Re: PATCH: Fix LoadCursor() under win9x

[Marcus Meissner]

On Fri, Oct 12, 2001 at 03:16:04PM +0200, Johan Gill wrote:
> LoadCursor() should return 0 if we emulate win9x and the second parameter
> is an invalid pointer.
> - if (HIWORD(name))
> -
>   if (GetVersion() & 0x80000000)  /* win9x */
> -
>       if (IsBadReadPtr(name, 4))
> -
>           return 0;

This code is unclean:
    It should not check for versions.
    It should not use IsBadReadPtr(), but IsBadStringPtrA().
    It should not use IsBad* functions, but an exception handler.

I would suggest the patch below.

Ciao, Marcus

Changelog:
    Handle bad pointer arguments to LoadImageA() (and functions calling
    LoadImageA) with an exception handler.

Index: cursoricon.c
===================================================================
RCS file: /home/wine/wine/windows/cursoricon.c,v
retrieving revision 1.35
diff -u -r1.35 cursoricon.c
--- cursoricon.c	2001/10/05 19:38:14	1.35
+++ cursoricon.c	2001/10/12 16:50:03
@@ -36,6 +36,7 @@
 #include "wingdi.h"
 #include "wine/winbase16.h"
 #include "wine/winuser16.h"
+#include "wine/exception.h"
 #include "heap.h"
 #include "palette.h"
 #include "bitmap.h"
@@ -2174,14 +2175,29 @@
  * FIXME: implementation lacks some features, see LR_ defines in winuser.h
  */

+/* filter for page-fault exceptions */
+static WINE_EXCEPTION_FILTER(page_fault)
+{
+    if (GetExceptionCode() == EXCEPTION_ACCESS_VIOLATION)
+	return EXCEPTION_EXECUTE_HANDLER;
+    return EXCEPTION_CONTINUE_SEARCH;
+}
+
 HANDLE WINAPI LoadImageA( HINSTANCE hinst, LPCSTR name, UINT type,
                               INT desiredx, INT desiredy, UINT loadflags)
 {
     HANDLE res;
     LPWSTR u_name;

-    if (HIWORD(name)) u_name = HEAP_strdupAtoW(GetProcessHeap(), 0, name);
-    else u_name=(LPWSTR)name;
+    __TRY {
+	if (HIWORD(name)) u_name = HEAP_strdupAtoW(GetProcessHeap(), 0, name);
+	else u_name=(LPWSTR)name;
+    }
+    __EXCEPT(page_fault) {
+	SetLastError( ERROR_INVALID_PARAMETER );
+	return 0;
+    }
+    __ENDTRY
     res = LoadImageW(hinst, u_name, type, desiredx, desiredy, loadflags);
     if (HIWORD(name)) HeapFree(GetProcessHeap(), 0, u_name);
     return res;

PATCH: Fix LoadCursor() under win9x

[Andreas Mohr]

On Fri, Oct 12, 2001 at 08:13:20PM +0200, Marcus Meissner wrote:
> On Fri, Oct 12, 2001 at 03:16:04PM +0200, Johan Gill wrote:
> > LoadCursor() should return 0 if we emulate win9x and the second parameter
> > is an invalid pointer.
> > - if (HIWORD(name))
> > -
> >   if (GetVersion() & 0x80000000)  /* win9x */
> > -
> >       if (IsBadReadPtr(name, 4))
> > -
> >           return 0;
>
> This code is unclean:
>    It should not check for versions.

Hmm.
> It should not use IsBadReadPtr(), but IsBadStringPtrA().

Oops, yep.
> It should not use IsBad* functions, but an exception handler.

Hmm, probably.

Oh no, not the version difference discussion again ;-)

Excuse me, but our testing revealed that there *is* a difference between
Win9x and NT 4, namely that Win9x returns 0, whereas NT 4 crashes
(the program, that is) !

Using your exception handler, LoadCursor() *always* returns 0
on invalid parameter, which is WRONG in the NT 4 case
since it continues program execution without crashing.

--> GetVersion() is required, check was (nearly) right IMHO.
Probably the best solution would be to use both an exception handler and
call GetVersion() on exception.

Well, you'll probably now argue that the NT 4 crash case is unneeded,
however I'm not that convinced about it, since it's a real Wine
incompatibility versus NT if we don't check on winver.
Any reasons for doing it this way and *not* (more or less) our way ? ;-)

PATCH: Fix LoadCursor() under win9x

[Marcus Meissner]

> Using your exception handler, LoadCursor() *always* returns 0
> on invalid parameter, which is WRONG in the NT 4 case
> since it continues program execution without crashing.
>
> --> GetVersion() is required, check was (nearly) right IMHO.
> Probably the best solution would be to use both an exception handler and
> call GetVersion() on exception.
>
> Well, you'll probably now argue that the NT 4 crash case is unneeded,
> however I'm not that convinced about it, since it's a real Wine
> incompatibility versus NT if we don't check on winver.
> Any reasons for doing it this way and *not* (more or less) our way ? ;-)

The version check here is unnecessary. We should only add it if there really
is requirement for the different behaviour, otherwise we open ourselves to
if(version1) else if (version2) hell, which will just make the code unreadable.

Ciao, Marcus