implement OleLoadPictureFile

[Michael Karcher]

Am Montag, den 17.11.2008, 13:09 -0800 schrieb Jeremy Drake:
> > Be sure to write your findings like (this is hypothetical, as I did not
> > look at your patches and the OlePicture stuff till now): "Loads an image
> > from a File. This is just like OleLoadPictureStream, but with a file
> > name instead of a stream as source specification" and not like "To
> > implement the function, you must create a stream from the file (use the
> > standard OLE file stream object for that), pass the stream to
> > OleLoadPictureStream and finally release the stream. In the error case
> > you should do foo". The second form *is* just publishing what you saw in
> > disassembly, so everyone who reads this mail carefully is in my oppinion
> > everyone reading it carefully is in the same situation as you and
> > shouldn't implement the function.
> Hmm, looks like I don't really have to.  Your detailed description is
> about 90% accurate :).  But the simple version should read "This is just
> like OleLoadPicture"...  I'll let you extrapolate the change to the
> detailed version.
I just guessed from the text that accompanied your patch and the
function names.

> 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.

With wine, it will crash in olepicture.c:1787 if I pass a NULL stream
into OleLoadPicture. If it crashes in windows too, it does not matter
how we implement the crash in Wine. I don't even think that we have to
crash everything that crashes in Windows. And if it will crash, I might
just as well assume that the VARIANT is a VT_BSTR without any checks, as
no Win32 program is going to call with wrong parameters.

You seem to have looked at the disassembled code too much to see that
"there is more than one way to do it". But exactly this seperates a
clean-room implementation from a rip-off. The clean-room reimplementers
(which could be me) must not be provided with the way how to do it, so
they can find there own way (this is the creative power that makes wine
our own work!) instead of blindly following the way Microsoft decided to
use.

> 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.

> 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.

> 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.

> > Testcases on the other hand are fine. They just describe *what* to do,
> > but not *how*. Especially for the error case testcases might be
> > interesting.
> 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.
That is fair. Even different Windows versions are some times
inconsistent with their error codes. It is more important what
conditions exactly count as errors, and what happens to the data pointed
to by lplpdispPicture parameter (unchanged / set to NULL / set to a
valid object implementing IDispatch and IPicture). Most prominent error
codes like "File not found" and "File is not in any supported format"
should be checked, but that's it. 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 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.

> 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.

Regards,
  Michael Karcher