Re: PATCH: GlobalAddAtomA check for invalid ptr
* 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. :-)
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
* 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. :-)
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
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 ); }
Marcus Meissner <marcus(a)jet.franken.de> 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. -- Alexandre Julliard julliard(a)winehq.org
On Thu, Aug 11, 2005 at 02:52:52PM +0200, Alexandre Julliard wrote:
Marcus Meissner <marcus(a)jet.franken.de> 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 ); }
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
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 }
participants (6)
-
Alexandre Julliard -
Andreas Mohr -
Felix Nawothnig -
Marcus Meissner -
Marcus Meissner -
Saulius Krasuckas