http://bugs.winehq.org/show_bug.cgi?id=10417
Summary: OLEAUT32: crash if >128 methods in an interface Product: Wine Version: 0.9.49. Platform: Macintosh OS/Version: Mac OS X 10.5 Status: UNCONFIRMED Severity: normal Priority: P1 Component: wine-ole AssignedTo: wine-bugs@winehq.org ReportedBy: mjk@cardbox.com
This bug was encountered in build cxoffice-6.2.0rc1-2-g024be42 of Wine (part of CrossOver Mac). The bug has been identified in the current source code at http://source.winehq.org/source/dlls/oleaut32/tmarshal.c.
Using any marshaled interface with more than 128 methods causes a crash within OLEAUT32 if any method at position >=128 is called. This was detected when using Cardbox (http://www.cardbox.com) and is a SHOW-STOPPER because it makes the use of VBScript macros impossible.
However, the bug is completely general and applies to any application at all that has interfaces with large number of methods. It is quite possible that many random OLE / COM - related bugs that have already been reported have this bug as their underlying cause.
The version of Cardbox on which the bug was found is more recent than the one currently available on the web site. If anyone wants to have a copy for testing, together with instructions for reproducing the crash, please contact me.
LOCATION OF THE BUG
The bug is in dlls/oleaut32/tmarshal.c. When constructing a proxy interface, PSFacBuf_CreateProxy at line #1712 constructs the following proxy code for each method:
popl %eax pushl <nr> pushl %eax call xCall lret <n> (+4)
where <nr> is the position of the method in the list of methods: 0, 1, 2, and so on.
The pushl <nr> instruction is defined by following code:
374 BYTE pushlval; // set to 0x6a by line #1712 375 BYTE nr;
The fact that the method position is a byte already limits the maximum size of an interface to 256 methods, which is less than the 512-method limit of Windows NT4.0 SP3, and the 1024-method limit of Windows 2000: see "MIDL2362" in http://msdn2.microsoft.com/en-us/library/aa366756.aspx for details. Thus this needs to be corrected in any case. The proxy code as it stands will call method 0 instead of method 256, method 1 instead of method 257, and so on, leading to random behaviour and possible stack corruption.
The crash when method 128 is called has a different cause. The proxy for method 128 contains the instruction 6A 80, because the programmer thought that this would push 00000080 onto the stack. In fact the PUSH instruction with opcode 6A SIGN-EXTENDS its operand and does not zero-extend it. Thus the proxy for the 128th method pushes FFFFFF80 onto the stack before calling xCall. xCall interprets this as a negative number (-128) and thus attempts to synthesize a call not to method 128 but to a non-existent method -128. In the same way it will call method -127 instead of method 129,... and so on.
SUGGESTED CORRECTION
The very simple correction to this bug, which is guaranteed to work, is to alter line 375 to
375 DWORD nr;
and line 1712 to
1712 xasm->pushlval = 0x68;
which expects a 32-bit operand rather than an 8-bit one.
This will result in every proxy using 15 bytes per method instead of 12 bytes. This does not seem an excessive price to pay for complete reliability in the future: there will then be no limit to the number of methods that can be supported.
ALTERNATIVE CORRECTIONS
If the 25% expansion in proxy size is considered unacceptable (it should not really be: proxies are small) then there are several ways round the problem. An increase to 256 methods could be achieved simply by adding a line at the very beginning of xCall:
method &= 0xff;
but this would HAVE to be accompanied by an explicit test for the method count limit (now 256) in PSFacBuf_CreateProxy so that the attempt to create a proxy with too methods would simply fail rather than (as now) generate a proxy that will randomly crash the application.
Another approach would be to create dummy functions (in assembler) that would add 128, 256, 384, 512, etc to the 'method' argument before forwarding it on to xCall. In that case, method numbers after 127 would generate proxies that called one of the variant xCalls instead of the original one. The programming in PSFacBuf_CreateProxy would be relatively straightforward, and the dummy functions would not need to do any stack manipulation: they would simply add an offset to the DWORD at [ESP+8] and then JMP straight to the start of xCall.
This would *still* give a finite limit to the number of methods, but the limit would be much larger. Again, good engineering practice dictates that PSFacBuf_CreateProxy should report an error if it encounters a number of methods beyond the number that it was designed to cope with.
http://bugs.winehq.org/show_bug.cgi?id=10417
--- Comment #1 from Vitaliy Margolen vitaliy@kievinfo.com 2007-11-11 14:10:45 --- Did you actually tried Wine or you just assumed that COX is the same?
Unless you use Wine and post backtrace of the crash, this bug is invalid.
http://bugs.winehq.org/show_bug.cgi?id=10417
--- Comment #2 from Martin Kochanski mjk@cardbox.com 2007-11-11 16:18:07 --- This bug was identified by reading the source code at http://source.winehq.org/source/dlls/oleaut32/tmarshal.c, which is part of the source code of Wine itself. It thus belongs to Wine directly rather than to CrossOver Mac.
A simple reading of the Wine source code is enough to show that (a) there is a bug, (b) the bug will ALWAYS lead to incorrect behaviour with any interface that has more than 128 methods. [Although admittedly someone ignorant of the Intel instruction set might think that the limit was 256 rather than 128]. If it is the aim of Wine to provide an environment for running Windows programs then the bug needs correcting whether or not it has ever been documented to cause an actual crash.
It is worth remembering how few bug reports come with actual identified erroneous lines in the Wine source code together with a two-line correction of the bug.
http://bugs.winehq.org/show_bug.cgi?id=10417
--- Comment #3 from Juan Lang juan_lang@yahoo.com 2007-11-11 17:02:47 --- Could you write a patch and mail it to wine-patches?
http://bugs.winehq.org/show_bug.cgi?id=10417
--- Comment #4 from Vitaliy Margolen vitaliy@kievinfo.com 2007-11-11 17:13:46 --- Then why don't you make a patch and send it in yourself? Otherwise some one will claim all the work you did finding this bug.
http://bugs.winehq.org/show_bug.cgi?id=10417
Dan Kegel dank@kegel.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dank@kegel.com Status|UNCONFIRMED |NEW Ever Confirmed|0 |1
--- Comment #5 from Dan Kegel dank@kegel.com 2007-11-11 19:14:18 --- Martin, thanks for finding and tracking down the bug! If you don't submit a patch, somebody else will eventually do it, but it might take a while.
http://bugs.winehq.org/show_bug.cgi?id=10417
--- Comment #6 from Martin Kochanski mjk@cardbox.com 2007-11-12 05:36:37 --- Thank you all for your kindness in commenting!
Juan, I'm only a poor innocent helpless Windows programmer and I've never done a diff in my life. I really wouldn't know where to begin with any of this, whereas I'm surrounded by experts who can do it in their sleep. Let's each do what we're good at. I've created the correction, though admittedly in the form of a piece of continuous prose rather than a proper patch. That was easy for me, though it still took a few hours. For someone else, it'll be easy to make a patch out of it.
Vitaliy, I don't mind about the glory: whoever submits the patch deserves the glory, as long as Wine is patched!
Dan, I know it'll take longer if I don't do it myself but it's a choice between me taking a day to learn how to do it all (and probably getting it wrong several times on the way) and someone else taking ten minutes to do the same thing.
All of you, I've posted a web page at http://www.cardbox.com/wine.htm that lets you download and install the Cardbox software. You can then reproduce the error in six mouse clicks (two of them are double-clicks).
I don't want to sound lazy or parasitic and I know this project works on voluntary contributions of time and effort. I hope you'll agree that identifying the correction for such a fundamental bug is a voluntary contribution in itself and not the act of a lazy parasite! I'll keep my fingers crossed and hope that someone turns this all into a patch in the not too distant future.
http://bugs.winehq.org/show_bug.cgi?id=10417
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution| |FIXED
--- Comment #7 from Alexandre Julliard julliard@winehq.org 2007-11-12 06:28:49 --- I've committed a fix. Thanks for the good report.
http://bugs.winehq.org/show_bug.cgi?id=10417
Dan Kegel dank@kegel.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #8 from Dan Kegel dank@kegel.com 2008-01-28 05:40:27 --- Closing all RESOLVED FIXED bugs older than four weeks.
http://bugs.winehq.org/show_bug.cgi?id=10417
Austin English austinenglish@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Platform|Macintosh |PC
http://bugs.winehq.org/show_bug.cgi?id=10417
Austin English austinenglish@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- OS/Version|Mac OS X 10.5 |Mac OS X
https://bugs.winehq.org/show_bug.cgi?id=10417
Nikolay Sivov bunglehead@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |2a2e303dc5598de6e97d4667b3a | |84a3c26b91c0e Component|ole |oleaut32