implement OleLoadPictureFile

[Jeremy Drake]

On Tue, 18 Nov 2008, Michael Karcher wrote:

> > Note that the IDL defines the VARIANT parameter as "optional".  If the
> > filename is not specified, you should pass a NULL stream to
> > OleLoadPicture.
> How do I know? I think you shouldn't have told these detail. I might
> know what happens if I pass in VARIANTs of type VT_ERROR or VT_NULL,
> like lplpdispPicture is unchanged, set to NULL or Windows crashes. But
> there seems no legal way for me to find out what happens inside
> Microsoft's oleaut32.dll. If it were an inter-dll call, relay or snoop
> might bring me to the conclusion that you are right, but I just pretend
> I never read that. Just write a couple of tests for different variant
> types (VT_NULL, VT_ERROR, VT_BSTR). If they crash on Windows, put them
> in an if(0) block with an appropriate comment.

Right.  Oops.  I may actually have mis-remembered that, so it's good you
didn't read that :P.  I thought I had read on the MSDN that NULL stream
meant that it would create an empty IPicture, but checking again it
doesn't.  I should have said that the unspecified parameter would result
in an empty picture, and let you go from there.  BTW, this case is tested
in my tests...  It may have been the " This is equivalent to calling
OleCreatePictureIndirect(NULL, ...)followed by IPersistStream::Load" from
the MSDN page that I was misremembering.

> > Also, there is an OleLoadPictureFileEx, which adds the same 3 additional
> > paramters as OleLoadPictureEx adds over OleLoadPicture.  You can probably
> > guess how that works...
> Do a three-way-merge of OleLoadPicture to OleLoadPictureEx and
> OleLoadPicture to OleLoadPictureFile and you arrive at
> OleLoadPictureFileEx, probably.

Or call one from the other and save the code duplication.

> > For the non-Ex versions, it may help you to know about the constant
> > LP_DEFAULT in olectl.h, which means "do the default thing" to the
> > extra params of the Ex version.
> MSDN documents LP_DEFAULT only for the palette in OleLoadPictureFileEx,
> but follows your points in the documentation for OleLoadPictureEx. It
> might have been that the the desired size should be just zero (which
> LP_DEFAULT happens to be) to get the default.
>
> > As far as the "standard OLE file stream object", if you know of one please
> > let me know.  AFAIK, it is a pretty major oversight in the OLE IStream
> > APIs that there is not a function to create an IStream given a file (name
> > or handle).
> Oops. I just assumed there was one, but you seem to be right. One could
> try whether the Windows FileMoniker works on objects that don't
> implement IPersistFile but just IPersistStream and thus
> IMoniker::BindToObject from the FileMoniker does what we want. But this
> is a very dangerous approach, as you can't tell BindToObject the CLSID
> to use, but just the IID you want, and the Wine implementation will
> happily activate and load *any* file type you pass in, and notice after
> loading that the object loaded does not support the IPicture interface.
> I don't know what ugly things can happen on IPersistStream::Load given
> some CLSIDs no one thinks about in the moment (maybe XML documents that
> cause an internet access to fetch the DTD), but I think some Media
> Player/Outlook security vulnerabilities we had some years ago are
> related to implementing things like this (that was IIRC attachments of
> the name ".WAV" that are associated with windows media player; windows
> media player checked magic numbers in that file, completely forgetting
> about the file a wave file finding "MZ" in the beginning and acting
> "appropriately). You will see whether OLELoadPictureFile does that if
> you do a relay trace with native oleaut32 and builtin ole32, as the
> CreateFileMoniker call would cross DLL boundaries. Please don't tell me
> that you know that Microsoft did it another way, even if you probably
> do.

OK, I won't.

> > I've always had to roll my own wrapper around the file APIs,
> > or use a hack like the one in OleLoadPictureFilePath.
> You probably mean OleLoadPictureFile. I am not going to look at your
> patch, so I don't know what hack you meant. If the hack was your idea,
> feel free to describe it. If the hack is inspired by the disassembly,
> please don't tell any Wine developer how that hack worked.

No, I meant a hack in OleLoadPicturePath, as implemented in wine.  I
surely would not mention a hack I derived from the disassembly.

> > Quite.  Also in olectl.h are the error codes that these functions
> > could return.  Unfortunately, I think it will be necessary to make some
> > sort of manual translation between the GetLastError returns from the
> > file APIs and these errors.  Without spelling out the mappings from the
> > disassembly, it will probably be nearly impossible to get all of the cases
> > exactly right.
> You shouldn't even have told that the mapping is done directly in
> OleLoadPictureFile and not some lower layer invoked by
> OleLoadPictureFile, except if you are sure from a relay trace. Of
> course, if Wine developers find that *somewhere* error codes seem to be
> mapped, they are free to do the mapping in OleLoadPictureFile.

I'm pretty sure I didn't tell you that.  I meant that there must be
somewhere the mapping is done, and that I didn't see any obvious numerical
relationships betweeen the codes that would imply some sort of
mathematical transformation (hence, manual).

> > I think that, as long as you are familiar with the APIs for dealing with
> > VARIANTs, this should be a very simple task.
> You might want to do the following test, if possible: Put the image into
> a file named "33" (without extension or anything) and call
> OleLoadPictureFile with a variant containing "33" as string value to
> verify that OleLoadPictureFile is able to load images without extension
> based on their magic bytes. If that works, retry with a variant
> containing the integer "33". If it still works, chances are *VERY* high
> that we need VariantChangeType. If it fails, we know that
> VariantChangeType is not the way to go.

Right.  I was planning to do this.

> > I would provide details
> > about how to deal with the VARIANT, but it may be misinterpreted as
> > knowledge gained from the disassembler.  So I'll just leave you with a
> > couple of links to MS docs.
> If you tell me whether VariantChangeType should be used here or not, it
> *is* knowledge gained from the disassembler, so it is exactly right you
> don't tell me that. Write good testcases instead that make me infer how
> to implement it. Thanks for the pointers to MSDN. I didn't know about
> the VT_ERROR/DISP_E_PARAMNOTFOUND case yet.

The VT_ERROR/DISP_E_PARAMNOTFOUND is one of the test cases I have
written...