Hi,
On Mon, Aug 29, 2005 at 12:37:31PM -0500, Alex Villacís Lasso wrote:
An old copy of Print Shop Deluxe Companion manages to supply a NULL as a name to be loaded by LoadLibrary16 --> LoadModule16 --> SIGSEGV. This patch adds a check to return ERROR_READ_FAULT instead of crashing.
Alex Villacís Lasso
Changelog:
- Add a NULL check in LoadModule16
Are you sure that Windows does that thing?
Somehow I really cannot imagine that during all these years this one has gone unnoticed...
(and is ERROR_READ_FAULT the correct error to return here?)
Thanks,
Andreas Mohr
Andreas Mohr wrote:
Hi,
On Mon, Aug 29, 2005 at 12:37:31PM -0500, Alex Villacís Lasso wrote:
An old copy of Print Shop Deluxe Companion manages to supply a NULL as a name to be loaded by LoadLibrary16 --> LoadModule16 --> SIGSEGV. This patch adds a check to return ERROR_READ_FAULT instead of crashing.
Alex Villacís Lasso
Changelog:
- Add a NULL check in LoadModule16
Are you sure that Windows does that thing?
Somehow I really cannot imagine that during all these years this one has gone unnoticed...
(and is ERROR_READ_FAULT the correct error to return here?)
Thanks,
Andreas Mohr
I could not find any MSDN reference on any documented behavior for LoadLibrary16 or LoadModule16 when libname == NULL.
Even if Windows does not check for NULL in this function, I do not think that the bug-for-bug compatibility should extend to the point where Wine crashes on the same invalid parameters as Windows does. This only leaves the exact value of the return value - I don't have the tools to generate a 16-bit executable under Windows, or else I don't know of any freely available ones, so I could only check the 32-bit LoadLibrary. Maybe somebody could point me to an URL to download a 16-bit compiler for Windows?
The specific value of ERROR_READ_FAULT was chosen because a) it is less than 31, as the MSDN documents, b) it is the closest description of what happens when the function attempts to use the parameter without checking.
Alex Villacís Lasso
Hi,
On Mon, Aug 29, 2005 at 01:43:04PM -0500, Alex Villacís Lasso wrote:
I could not find any MSDN reference on any documented behavior for LoadLibrary16 or LoadModule16 when libname == NULL.
Even if Windows does not check for NULL in this function, I do not think that the bug-for-bug compatibility should extend to the point where Wine crashes on the same invalid parameters as Windows does. This only leaves
That's what you think, but we will DEFINITELY want bug-for-bug compatibility (well, except for fringe cases, which that one isn't, IMHO). (no criticism)
We really want that, even if it's only used to be sure that an error is an error (in order to realize immediately that something is badly wrong since the program shouldn't behave that way). Simply returning an *illegal* return code instead of crashing (like Windows!) is NOT an option.
All that assuming that Windows LoadModule16 does crash, of course...
the exact value of the return value - I don't have the tools to generate a 16-bit executable under Windows, or else I don't know of any freely available ones, so I could only check the 32-bit LoadLibrary. Maybe somebody could point me to an URL to download a 16-bit compiler for Windows?
You want OpenWatcom, believe me. You want it. Badly! :)
(I have an old NonopenWatcom installation here, so I'll write a short test, but maybe you'll even beat me to it)
The specific value of ERROR_READ_FAULT was chosen because a) it is less than 31, as the MSDN documents, b) it is the closest description of what happens when the function attempts to use the parameter without checking.
Wine cannot choose or guess. Wine has to know. Even a single bit can be different and cause as much harm as making a whole Windows program fail... (which is why so many people say that "Wine is crap, it doesn't work": one single "completely irrelevant" difference can cause a terrible amount of trouble, no matter how absolutely incredibly perfect all the remaining parts of Wine are)
Andreas
Hi,
On Mon, Aug 29, 2005 at 01:43:04PM -0500, Alex Villacís Lasso wrote:
I could not find any MSDN reference on any documented behavior for LoadLibrary16 or LoadModule16 when libname == NULL.
I've checked it now (Watcom, Win98 SE):
(forgot to zero LOADPARAMS, sorry! But it hopefully didn't matter...)
a) NULL, &lp 0 "system out of mem/executable corrupt/relocs invalid" b) "kernel", NULL kernel handle c) NULL, NULL 0 d) (char *)0x1, &lp 2 "file not found" e) (char *)0x1256, NULL 2 f) NULL, (void *)0x1257 0
NO CRASHES whatsoever, IOW it fully intercepts any invalid pointers.
a), c), f) vs. d) indicates that it does an explicit check against a NULL name, since probably otherwise LoadModule16 will continue due to the non-NULL name (0x1) and notice somewhat later (in some file API) that file name 0x1 is invalid.
So yes, you were right after all - but only semi-right, since the return code is wrong.
IMHO this incident strongly indicates that we should add a Win16 source to the Wine tree which can be compiled with Watcom and run similar to the Wine test suite. I don't think such a glaring omission could have happened with Win32 API due to our test suite (well, sure, somewhere it can happen, but the chances are much lower).
There probably is no good way to do Win16 calls in Win32 programs with thunking, so we should just have a separate Win16 source instead, to be tested as time permits, since it needs a Watcom compile.
What about a directory dlls/kernel/tests/win16/ ? (and adding a README mentioning OpenWatcom) Or should it be dlls/kernel/tests16/ instead?
I've got lots of Win16 tests in a Watcom file on my system, I should reorganize it into a common format and add them to CVS, I guess...
Andreas
Andreas Mohr wrote:
What about a directory dlls/kernel/tests/win16/ ? (and adding a README mentioning OpenWatcom) Or should it be dlls/kernel/tests16/ instead?
Why not a binary win16 checked into CVS to be run by winetest? We only want to test win16 loading, right?
regards, Jakob
Andreas Mohr wrote:
Hi,
On Mon, Aug 29, 2005 at 01:43:04PM -0500, Alex Villacís Lasso wrote:
I could not find any MSDN reference on any documented behavior for LoadLibrary16 or LoadModule16 when libname == NULL.
I've checked it now (Watcom, Win98 SE):
(forgot to zero LOADPARAMS, sorry! But it hopefully didn't matter...)
a) NULL, &lp 0 "system out of mem/executable corrupt/relocs invalid" b) "kernel", NULL kernel handle c) NULL, NULL 0 d) (char *)0x1, &lp 2 "file not found" e) (char *)0x1256, NULL 2 f) NULL, (void *)0x1257 0
NO CRASHES whatsoever, IOW it fully intercepts any invalid pointers.
a), c), f) vs. d) indicates that it does an explicit check against a NULL name, since probably otherwise LoadModule16 will continue due to the non-NULL name (0x1) and notice somewhat later (in some file API) that file name 0x1 is invalid.
The attached patch should implement the observed behavior in Win98SE.
Alex Villacís Lasso
Changelog: - Add NULL and exception handler to LoadModule16 to check against NULL or invalid libname
-
Alex Villacís Lasso -
Andreas Mohr -
Jakob Eriksson