Re: implement OleLoadPictureFile
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
Re: implement OleLoadPictureFile
Hello Jeremy, Juan and rest of wine-devel [sorry for shifting quotes around, this makes following my points easier and in this case shouldn't forge you to seem to have said something you didn't say] And now that I know that, I certainly won't be doing it again. Thank you for your understaning. Another Thank you from here. If I provided a patch to dlls/oleaut32/tests/olepicture.c adding tests that verify the behaviors of the functions, would that be accepted? In general, writing tests is okay, as they are an exercise in black-box reverse engineering, which is what's allowed. So yeah, tests would be great. Would it be acceptable for me to write up a description of what the function needs to do, so that someone else can do a clean-room implementation? I think that would be acceptable, yes. We have implemented things with hints from people who've studied disassembly before. Do you have a bug open? Sorry, I've forgotten. If not, please do open one. You can describe your findings there. The best way to do this is (in my oppinion) submitting the testcase again, but without the implementation, and marking the test todo_wine. A bug might be useful, but wouldn't a mail that contains the testcase as patch and the description in the comment part to wine-patches suffice? 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. 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. Is there anyone out there who would volunteer to do it if I were to write a description of the function? I would do so, if its just some minutes. Regards, Michael Karcher
Re: implement OleLoadPictureFile
On Thu, 13 Nov 2008, Juan Lang wrote: Any feedback on the spec file or the debug trace? I'm afraid I don't know what substantive difference there is between looking at one small portion of the disassembly (to verify a function is being called) and learning something more substantial. There are certain kinds of reverse engineering that are allowed, but looking at disassembly is not. And now that I know that, I certainly won't be doing it again. I don't think this is going to go in, sorry. Well, I still have an application that won't run under wine because this function is not implemented. So assuming *this* patch doesn't get in, what can I do to help get *a* patch in that implements this function? I can blow away my sandbox and start from scratch, but I suspect you would find that insufficient. Maybe I should find a hypnotist to make me forget what I saw in the debugger :) Would it be acceptable for me to write up a description of what the function needs to do, so that someone else can do a clean-room implementation? It would probably take a reasonably experienced C/COM developer all of about 5 minutes, since this function is really just a wrapper of an already-implemented function. Is there anyone out there who would volunteer to do it if I were to write a description of the function? If I provided a patch to dlls/oleaut32/tests/olepicture.c adding tests that verify the behaviors of the functions, would that be accepted? Or is writing tests for functions you've seen the disassembly for also prohibited? I realize now that I've made a mistake by looking at the function in the debugger, but hopefully there is still a way I can help get this function implemented. Thanks, Jeremy
Re: implement OleLoadPictureFile
On Sun, 16 Nov 2008, Juan Lang wrote: Do you have a bug open? Sorry, I've forgotten. If not, please do open one. You can describe your findings there. No, but a quick search on bugzilla for OleLoadPictureFile turned up bug 10156. A comment on that bug suggests changing the summary to OleLoadPictureFile not implemented, which would exactly sum up my situation. I posted a comment to that bug to that affect. Should that bug be renamed, I could post my findings there... If I provided a patch to dlls/oleaut32/tests/olepicture.c adding tests that verify the behaviors of the functions, would that be accepted? Or is writing tests for functions you've seen the disassembly for also prohibited? In general, writing tests is okay, as they are an exercise in black-box reverse engineering, which is what's allowed. So yeah, tests would be great. I've been looking at writing some tests (I've been compiling using make crosstest and running the exe on windows 2000), and I've found that there must be some sort of mapping between error codes from GetLastError when accessing the file and the HRESULT values returned from OleLoadPicutreFile (and not of the standard HRESULT_FROM_WIN32 variety that I'm familiar with). For instance, CTL_E_FILENOTFOUND is returned when the file does not exist. I'm trying to invoke as many error cases in the test as I can to flesh out this mapping, but I'm not sure I can find all of them in this manner... I think at some point in the next couple of days I'll decide I've got as many as I can and send in a patch to the tests.
Re: implement OleLoadPictureFile
On Mon, 17 Nov 2008, Michael Karcher wrote: The best way to do this is (in my oppinion) submitting the testcase again, but without the implementation, and marking the test todo_wine. A bug might be useful, but wouldn't a mail that contains the testcase as patch and the description in the comment part to wine-patches suffice? Sounds good to me. 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. Note that the IDL defines the VARIANT parameter as optional. If the filename is not specified, you should pass a NULL stream to OleLoadPicture. Also, there is an OleLoadPictureFileEx, which adds the same 3 additional paramters as OleLoadPictureEx adds over OleLoadPicture. You can probably guess how that works... 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. 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). I've always had to roll my own wrapper around the file APIs, or use a hack like the one in OleLoadPictureFilePath. 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. The tests I have come up with so far verify the error codes that I've been able to figure out how to trigger. Is there anyone out there who would volunteer to do it if I were to write a description of the function? I would do so, if its just some minutes. I think that, as long as you are familiar with the APIs for dealing with VARIANTs, this should be a very simple task. 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. http://msdn.microsoft.com/en-us/library/ms221673.aspx http://support.microsoft.com/kb/238981
Re: implement OleLoadPictureFile
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.
Re: implement OleLoadPictureFile
2008/11/17 Michael Karcher [EMAIL PROTECTED]: Am Montag, den 17.11.2008, 13:09 -0800 schrieb Jeremy Drake: 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. In shlwapi there is a SHCreateStreamFromFile with A, W and Ex versions when given a filename. 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. There should not be any mapping of error codes, except where tests clearly show what is returned and that does not match what is returned by the APIs being called to implement the function. Some of the errors - such as file not found can be handled by the called APIs like SHCreateStreamOnFile. There should still be tests for these codepaths as well. FWIW, applications are not usually concerned with the specifics of why an API failed, just that it did. This, in addition to Windows changing the return codes every release means that the error codes returned won't make that big a difference; the only error codes you can rely on will be those listed in the MSDN documentation. - Reece
Re: implement OleLoadPictureFile
And now that I know that, I certainly won't be doing it again. Thank you for your understaning. Well, I still have an application that won't run under wine because this function is not implemented. So assuming *this* patch doesn't get in, what can I do to help get *a* patch in that implements this function? I can blow away my sandbox and start from scratch, but I suspect you would find that insufficient. Maybe I should find a hypnotist to make me forget what I saw in the debugger :) :) Would it be acceptable for me to write up a description of what the function needs to do, so that someone else can do a clean-room implementation? It would probably take a reasonably experienced C/COM developer all of about 5 minutes, since this function is really just a wrapper of an already-implemented function. Is there anyone out there who would volunteer to do it if I were to write a description of the function? I think that would be acceptable, yes. We have implemented things with hints from people who've studied disassembly before. Do you have a bug open? Sorry, I've forgotten. If not, please do open one. You can describe your findings there. If I provided a patch to dlls/oleaut32/tests/olepicture.c adding tests that verify the behaviors of the functions, would that be accepted? Or is writing tests for functions you've seen the disassembly for also prohibited? In general, writing tests is okay, as they are an exercise in black-box reverse engineering, which is what's allowed. So yeah, tests would be great. --Juan
Re: OLEAUT32: implement OleLoadPictureFile(Ex) and add tests
On Mi, 2008-11-12 at 23:40 -0800, Jeremy Drake wrote: I have no Idea about that area, but the unicode API is not implemented on win9x (GetTempFileNameW, CreateFileW, DeleteFileW) if(!pOleLoadPictureFile || !pOleLoadPictureFileEx) Are there systems in the Wild that have OleLoadPictureFile, but are missing OleLoadPictureFile? In that case, you should use 2 seperate tests. skip(Skipping OleLoadPictureFile tests\n); Think, what you would see in the log before your text: filename.c:__LINE__: Tests skipped: Skipping is redundant and tests also. The usual way would be: skip(OleLoadPictureFile not found\n); When you want to make sure, that we never skip in Wine here, then use win_skip instead. -- By by ... Detlef
Re: OLEAUT32: implement OleLoadPictureFile(Ex) and add tests
On Fr, 2008-11-14 at 21:17 +0100, Detlef Riekenberg wrote: On Mi, 2008-11-12 at 23:40 -0800, Jeremy Drake wrote: if(!pOleLoadPictureFile || !pOleLoadPictureFileEx) Are there systems in the Wild that have OleLoadPictureFile, but are missing OleLoadPictureFile? In that case, you should use 2 seperate tests. skip(Skipping OleLoadPictureFile tests\n); Think, what you would see in the log before your text: filename.c:__LINE__: Tests skipped: Skipping is redundant and tests also. The usual way would be: skip(OleLoadPictureFile not found\n); When you want to make sure, that we never skip in Wine here, then use win_skip instead. Forget that. I just read the Mail about the disassembly usage. -- By by ... Detlef
Re: implement OleLoadPictureFile
On Wed, 12 Nov 2008, Austin English wrote: Can you add a testcase to show that this behavior is correct? Yeah. I'll do that for the next version of the patch, as well as implement OleLoadPictureFileEx. Any feedback on the spec file or the debug trace? Thanks, Jeremy -- If you cannot convince them, confuse them. -- Harry S Truman
Re: implement OleLoadPictureFile
Any feedback on the spec file or the debug trace? I'm afraid I don't know what substantive difference there is between looking at one small portion of the disassembly (to verify a function is being called) and learning something more substantial. There are certain kinds of reverse engineering that are allowed, but looking at disassembly is not. I don't think this is going to go in, sorry. --Juan
Re: implement OleLoadPictureFile
On Wed, Nov 12, 2008 at 2:16 AM, Jeremy Drake [EMAIL PROTECTED] wrote: I ran into a function that was not implemented when attempting to run a program I work on under wine. This patch implements OleLoadPictureFile based on the MSDN docs (http://msdn.microsoft.com/en-us/library/ms221680.aspx) and what I saw when stepping through in windbg on XP. A few notes: I'm not sure I got the spec file right. The first param is a VARIANT, not a VARIANT *, so I'm not sure ptr is the right thing. I'm not sure the right way to do the TRACE call with the VARIANT param. I did not implement OleLoadPictureFileEx. Technically, OleLoadPictureFile should call OleLoadPictureFileEx(varFilename, LP_DEFAULT, LP_DEFAULT, LP_DEFAULT, ppdispPicture) and OleLoadPictureEx should pass the sizeX, sizeY, and flags to OleLoadPictureEx. When stepping through the function in windbg, I saw it call a function OLEAUT32!CreateFileStream which does not exist in wine anywhere. Instead of using that, I used the same mechanism as was used in OleLoadPicturePath (GlobalAlloc the size of the file, read the whole file in, and CreateStreamOnHGlobal). Since this is my first wine patch, I hope you will forgive me these shortcomings. I'm not subscribed to any wine list, so please copy me on any replies. Thanks, Jeremy -- Personifiers Unite! You have nothing to lose but Mr. Dignity! Can you add a testcase to show that this behavior is correct? -- -Austin
Re: implement OleLoadPictureFile
This patch implements OleLoadPictureFile based on the MSDN docs (http://msdn.microsoft.com/en-us/library/ms221680.aspx) and what I saw when stepping through in windbg on XP. Stop right there. Implementing stuff based on looking at disassembly is expressly not allowed here, sorry. --Juan
