Re: PATCH: GlobalAddAtomA check for invalid ptr

[Saulius Krasuckas]

* On Tue, 9 Aug 2005, Marcus Meissner wrote:
> The Ikea kitchen planner passed 0xcccccccc for some reason and works 
> much better after this patch.

It if uninitialized memory in a case of MSVC and MSVCRTD, I guess.  
BUUUUG in the app. :-)

PATCH: GlobalAddAtomA check for invalid ptr

[Andreas Mohr]

Hi,

On Tue, Aug 09, 2005 at 12:13:22PM +0300, Saulius Krasuckas wrote:
> - On Tue, 9 Aug 2005, Marcus Meissner wrote:
> > The Ikea kitchen planner passed 0xcccccccc for some reason and works 
> > much better after this patch.
>
> It if uninitialized memory in a case of MSVC and MSVCRTD, I guess.  
> BUUUUG in the app. :-)

Yup, and AFAIR this gets initialized with 0x0c since that is a breakpoint
instruction. But you possibly knew that already...

And who says that it's an application bug?
It may just as well be something triggered by some unusual behaviour of Wine.
But then it might still be an application bug in that strangely triggered
code path after all...

Andreas

PATCH: GlobalAddAtomA check for invalid ptr

[Saulius Krasuckas]

* On Tue, 9 Aug 2005, Andreas Mohr wrote:
> - On Tue, Aug 09, 2005 at 12:13:22PM +0300, Saulius Krasuckas wrote:
> > It if uninitialized memory in a case of MSVC and MSVCRTD, I guess.  
> > BUUUUG in the app. :-)
>
> Yup, and AFAIR this gets initialized with 0x0c since that is a 
> breakpoint instruction. But you possibly knew that already...

No, I didn't.  Thanks.
> And who says that it's an application bug?
> It may just as well be something triggered by some unusual behaviour of Wine.
> But then it might still be an application bug in that strangely triggered
> code path after all...

Well, I am persuaded now. :-)

PATCH: GlobalAddAtomA check for invalid ptr

[Andreas Mohr]

Hi,

On Tue, Aug 09, 2005 at 01:32:51PM +0300, Saulius Krasuckas wrote:
> - On Tue, 9 Aug 2005, Andreas Mohr wrote:
> > - On Tue, Aug 09, 2005 at 12:13:22PM +0300, Saulius Krasuckas wrote:
> > > It if uninitialized memory in a case of MSVC and MSVCRTD, I guess.  
> > > BUUUUG in the app. :-)
> >
> > Yup, and AFAIR this gets initialized with 0x0c since that is a 
> > breakpoint instruction. But you possibly knew that already...
>
> No, I didn't.  Thanks.

Stupid me, despite my words it's still 0xcc, of course.
INT 3 (breakpoint interrupt) call, AFAIR.

Andreas

PATCH: GlobalAddAtomA check for invalid ptr

[Marcus Meissner]

On Tue, Aug 09, 2005 at 12:13:22PM +0300, Saulius Krasuckas wrote:
> - On Tue, 9 Aug 2005, Marcus Meissner wrote:
> > The Ikea kitchen planner passed 0xcccccccc for some reason and works 
> > much better after this patch.
>
> It if uninitialized memory in a case of MSVC and MSVCRTD, I guess.  
> BUUUUG in the app. :-)

(...RegisterClassExA calls GlobalAddAtomA...)

I had a look at the disassembly and yes, the program is buggy.

It has a function which goes like this:

func() {
    WNDCLASSA 	wca;
    WNDCLASSEXA	wcexa;

/* inserted by compiler most likely */
    memset(localstackspace, 0xCC, sizeoflocalstackspace);

... initialize fields of wca ...
    
    wcexa.cbSize = sizeof(wcexa);
    wcexa.hIconSm = LoadIconA(...);

RegisterClassA(&wca);
    RegisterClassExA(&wcexa);

...
}

So wondering why it does not initialize anything of WNDCLASSEXA except cbSize and
hIconSm ...

I think something in the function assumes that wca and wcexa overlap (since WNDCLASSEXA
has this layout: 
    UINT	cbSize;
    WNDCLASSA	wndclassastuff;
    HICON     hIconSm;

I really would like to see the sourcecode of this one and how it is broken. ;)
(Programmers of IKEA Kitchen Planner ... Please read.)

So ... now we can:

- fix this program by contacting the developers etc... which is difficult.
- fix WINE by adding a check.

I can only fix WINE.

Ciao, Marcus

Index: dlls/kernel/atom.c
===================================================================
RCS file: /home/wine/wine/dlls/kernel/atom.c,v
retrieving revision 1.8
diff -u -r1.8 atom.c
--- dlls/kernel/atom.c	10 May 2005 15:15:50 -0000	1.8
+++ dlls/kernel/atom.c	9 Aug 2005 06:55:47 -0000
@@ -183,6 +183,10 @@
  */
 ATOM WINAPI GlobalAddAtomA( LPCSTR str /* [in] String to add */ )
 {
+    if (HIWORD(str) && IsBadStringPtrA(str, MAX_ATOM_LEN)) {
+	SetLastError( ERROR_INVALID_PARAMETER );
+	return 0;
+    }
     return ATOM_AddAtomA( str, NULL );
 }

PATCH: GlobalAddAtomA check for invalid ptr

[Alexandre Julliard]

Marcus Meissner  writes:
> So ... now we can:
> - fix this program by contacting the developers etc... which is difficult.
> - fix WINE by adding a check.
>
> I can only fix WINE.

Yes, but please don't use the IsBad* functions, use a proper exception
handler.

PATCH: GlobalAddAtomA check for invalid ptr

[Marcus Meissner]

On Thu, Aug 11, 2005 at 02:52:52PM +0200, Alexandre Julliard wrote:
> Marcus Meissner  writes:
> > So ... now we can:
> > - fix this program by contacting the developers etc... which is difficult.
> > - fix WINE by adding a check.
> >
> > I can only fix WINE.
>
> Yes, but please don't use the IsBad* functions, use a proper exception
> handler.

Next try, is this what you imagined.

I now do a strlen() in GlobalAddAtomA(), however the compiler might optimize it
away in the future (gcc 4.1 doesn't).

Ciao, Marcus

Changelog:
    Catch invalid strings in GlobalAddAtomA() using an
    exception handler.

Index: dlls/kernel/atom.c
===================================================================
RCS file: /home/wine/wine/dlls/kernel/atom.c,v
retrieving revision 1.8
diff -u -r1.8 atom.c
--- dlls/kernel/atom.c	10 May 2005 15:15:50 -0000	1.8
+++ dlls/kernel/atom.c	14 Aug 2005 10:24:46 -0000
@@ -38,6 +38,8 @@
 #include "winbase.h"
 #include "winerror.h"

+#include "wine/exception.h"
+#include "excpt.h"
 #include "wine/server.h"
 #include "wine/unicode.h"
 #include "kernel_private.h"
@@ -48,6 +50,14 @@

#define MAX_ATOM_LEN 255

+/* filter for page-fault exceptions */
+static WINE_EXCEPTION_FILTER(page_fault)
+{
+    if (GetExceptionCode() == EXCEPTION_ACCESS_VIOLATION)
+        return EXCEPTION_EXECUTE_HANDLER;
+    return EXCEPTION_CONTINUE_SEARCH;
+}
+
 static struct atom_table* get_local_table(DWORD entries)
 {
     static struct atom_table*   local_table;
@@ -183,6 +193,20 @@
  */
 ATOM WINAPI GlobalAddAtomA( LPCSTR str /* [in] String to add */ )
 {
+    if (HIWORD(str)) {
+	__TRY {
+		char *s = (char*)str;
+		while (*s) s++;
+		/* FIXME: Should make sure the compiler does
+		 * not throw away the above.
+		 */
+	}
+	__EXCEPT(page_fault) {
+	    SetLastError( ERROR_INVALID_PARAMETER );
+	    return 0;
+	}
+	__ENDTRY
+    }
     return ATOM_AddAtomA( str, NULL );
 }

PATCH: GlobalAddAtomA check for invalid ptr

[Felix Nawothnig]

Marcus Meissner wrote:
> > Yes, but please don't use the IsBad* functions, use a proper exception
> > handler.
>
> Next try, is this what you imagined.

The reason for using an exception handler is thread safety - to achieve 
that you have to put it around the ATOM_AddAtomA call.

Felix

PATCH: GlobalAddAtomA check for invalid ptr

[Marcus Meissner]

On Sun, Aug 14, 2005 at 10:27:20PM +0200, Felix Nawothnig wrote:
> Marcus Meissner wrote:
> > > Yes, but please don't use the IsBad* functions, use a proper exception
> > > handler.
> >
> > Next try, is this what you imagined.
>
> The reason for using an exception handler is thread safety - to achieve 
> that you have to put it around the ATOM_AddAtomA call.

Next try...

Ciao, Marcus

Changelog:
    Wrap GlobalAddAtomA() internal call with exception handler.

Index: dlls/kernel/atom.c
===================================================================
RCS file: /home/wine/wine/dlls/kernel/atom.c,v
retrieving revision 1.8
diff -u -r1.8 atom.c
--- dlls/kernel/atom.c	10 May 2005 15:15:50 -0000	1.8
+++ dlls/kernel/atom.c	15 Aug 2005 05:35:29 -0000
@@ -38,6 +38,8 @@
 #include "winbase.h"
 #include "winerror.h"

+#include "wine/exception.h"
+#include "excpt.h"
 #include "wine/server.h"
 #include "wine/unicode.h"
 #include "kernel_private.h"
@@ -48,6 +50,14 @@

#define MAX_ATOM_LEN 255

+/* filter for page-fault exceptions */
+static WINE_EXCEPTION_FILTER(page_fault)
+{
+    if (GetExceptionCode() == EXCEPTION_ACCESS_VIOLATION)
+        return EXCEPTION_EXECUTE_HANDLER;
+    return EXCEPTION_CONTINUE_SEARCH;
+}
+
 static struct atom_table* get_local_table(DWORD entries)
 {
     static struct atom_table*   local_table;
@@ -183,7 +193,14 @@
  */
 ATOM WINAPI GlobalAddAtomA( LPCSTR str /* [in] String to add */ )
 {
-    return ATOM_AddAtomA( str, NULL );
+    __TRY {
+	return ATOM_AddAtomA( str, NULL );
+    }
+    __EXCEPT(page_fault) {
+	SetLastError( ERROR_INVALID_PARAMETER );
+	return 0;
+    }
+    __ENDTRY
 }