[PATCH] WIP/krnl386: When freeing task, also reset internal variable
This is a WIP patch fixing crashes in the win16 program Nota Bene. The crash is caused by alloc_win16_thunk calling GlobalAlloc16, GlobalLock16 and keeping the pinned address as "thunk_array". Later we have the following chain: TASK_DeleteTask -> GlobalFreeAll16 -> GlobalFree16 This deletes the buffer behind "thunk_array", but since it's not NULL wine will continue to use it, causing funny heap corruption issues. This patch fixes the issue and seems logically correct to me, though it's very ugly architecturally. Any hints on how we should handle this case properly? Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=12732 --- dlls/krnl386.exe16/krnl386.exe16.spec | 2 ++ dlls/krnl386.exe16/task.c | 9 +++++++++ dlls/user.exe16/message.c | 6 ++++++ include/wine/winbase16.h | 3 +++ 4 files changed, 20 insertions(+) diff --git a/dlls/krnl386.exe16/krnl386.exe16.spec b/dlls/krnl386.exe16/krnl386.exe16.spec index d98e136c77e..a1e7db46c0f 100644 --- a/dlls/krnl386.exe16/krnl386.exe16.spec +++ b/dlls/krnl386.exe16/krnl386.exe16.spec @@ -751,3 +751,5 @@ # Snoop support 2001 pascal -register __wine_snoop_entry() 2002 pascal -register __wine_snoop_return() + +@ cdecl -arch=win32 __wine_set_taskkill_func(ptr) diff --git a/dlls/krnl386.exe16/task.c b/dlls/krnl386.exe16/task.c index 30a40328c86..140b9190666 100644 --- a/dlls/krnl386.exe16/task.c +++ b/dlls/krnl386.exe16/task.c @@ -351,6 +351,12 @@ static TDB *TASK_Create( NE_MODULE *pModule, UINT16 cmdShow, LPCSTR cmdline, BYT return pTask; } +static wine_taskkill_func taskkill; + +void __wine_set_taskkill_func(wine_taskkill_func func) +{ + taskkill = func; +} /*********************************************************************** * TASK_DeleteTask @@ -382,6 +388,9 @@ static void TASK_DeleteTask( HTASK16 hTask ) /* the environment block and the thunk segments). */ GlobalFreeAll16( hPDB ); + + if (taskkill) + taskkill(); } diff --git a/dlls/user.exe16/message.c b/dlls/user.exe16/message.c index b79b0cb7751..f5cadc5e387 100644 --- a/dlls/user.exe16/message.c +++ b/dlls/user.exe16/message.c @@ -144,6 +144,11 @@ static int winproc_to_index( WNDPROC16 handle ) return index; } +static void taskkill(void) +{ + thunk_array = 0; +} + /* allocate a 16-bit thunk for an existing window proc */ static WNDPROC16 alloc_win16_thunk( WNDPROC handle ) { @@ -156,6 +161,7 @@ static WNDPROC16 alloc_win16_thunk( WNDPROC handle ) if (!thunk_array) /* allocate the array and its selector */ { + __wine_set_taskkill_func(taskkill); assert( MAX_WINPROCS16 * sizeof(WINPROC_THUNK) <= 0x10000 ); if (!(thunk_selector = GlobalAlloc16( GMEM_FIXED | GMEM_ZEROINIT, diff --git a/include/wine/winbase16.h b/include/wine/winbase16.h index 1d052130e33..338a2af4e9e 100644 --- a/include/wine/winbase16.h +++ b/include/wine/winbase16.h @@ -567,4 +567,7 @@ BOOL16 WINAPI WriteProfileSection16(LPCSTR,LPCSTR); #define CURRENT_SP (((WORD *)NtCurrentTeb()->SystemReserved1)[0]) #define CURRENT_SS (((WORD *)NtCurrentTeb()->SystemReserved1)[1]) +typedef void (*wine_taskkill_func)(void); +extern void __wine_set_taskkill_func(wine_taskkill_func func); + #endif /* __WINE_WINE_WINBASE16_H */ -- 2.34.1
Any news on this? After a bit more thinking this doesn't seem too correct anymore to me, but I still don't know how to do it properly. We somehow must prevent the thunks from being freed as long as we still use them. Regards, Fabian Maurer On Montag, 17. Januar 2022 22:26:33 CET you wrote:
This is a WIP patch fixing crashes in the win16 program Nota Bene. The crash is caused by alloc_win16_thunk calling GlobalAlloc16, GlobalLock16 and keeping the pinned address as "thunk_array".
Later we have the following chain: TASK_DeleteTask -> GlobalFreeAll16 -> GlobalFree16 This deletes the buffer behind "thunk_array", but since it's not NULL wine will continue to use it, causing funny heap corruption issues.
This patch fixes the issue and seems logically correct to me, though it's very ugly architecturally. Any hints on how we should handle this case properly?
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=12732 --- dlls/krnl386.exe16/krnl386.exe16.spec | 2 ++ dlls/krnl386.exe16/task.c | 9 +++++++++ dlls/user.exe16/message.c | 6 ++++++ include/wine/winbase16.h | 3 +++ 4 files changed, 20 insertions(+)
diff --git a/dlls/krnl386.exe16/krnl386.exe16.spec b/dlls/krnl386.exe16/krnl386.exe16.spec index d98e136c77e..a1e7db46c0f 100644 --- a/dlls/krnl386.exe16/krnl386.exe16.spec +++ b/dlls/krnl386.exe16/krnl386.exe16.spec @@ -751,3 +751,5 @@ # Snoop support 2001 pascal -register __wine_snoop_entry() 2002 pascal -register __wine_snoop_return() + +@ cdecl -arch=win32 __wine_set_taskkill_func(ptr) diff --git a/dlls/krnl386.exe16/task.c b/dlls/krnl386.exe16/task.c index 30a40328c86..140b9190666 100644 --- a/dlls/krnl386.exe16/task.c +++ b/dlls/krnl386.exe16/task.c @@ -351,6 +351,12 @@ static TDB *TASK_Create( NE_MODULE *pModule, UINT16 cmdShow, LPCSTR cmdline, BYT return pTask; }
+static wine_taskkill_func taskkill; + +void __wine_set_taskkill_func(wine_taskkill_func func) +{ + taskkill = func; +}
/*********************************************************************** * TASK_DeleteTask @@ -382,6 +388,9 @@ static void TASK_DeleteTask( HTASK16 hTask ) /* the environment block and the thunk segments). */
GlobalFreeAll16( hPDB ); + + if (taskkill) + taskkill(); }
diff --git a/dlls/user.exe16/message.c b/dlls/user.exe16/message.c index b79b0cb7751..f5cadc5e387 100644 --- a/dlls/user.exe16/message.c +++ b/dlls/user.exe16/message.c @@ -144,6 +144,11 @@ static int winproc_to_index( WNDPROC16 handle ) return index; }
+static void taskkill(void) +{ + thunk_array = 0; +} + /* allocate a 16-bit thunk for an existing window proc */ static WNDPROC16 alloc_win16_thunk( WNDPROC handle ) { @@ -156,6 +161,7 @@ static WNDPROC16 alloc_win16_thunk( WNDPROC handle )
if (!thunk_array) /* allocate the array and its selector */ { + __wine_set_taskkill_func(taskkill); assert( MAX_WINPROCS16 * sizeof(WINPROC_THUNK) <= 0x10000 );
if (!(thunk_selector = GlobalAlloc16( GMEM_FIXED | GMEM_ZEROINIT, diff --git a/include/wine/winbase16.h b/include/wine/winbase16.h index 1d052130e33..338a2af4e9e 100644 --- a/include/wine/winbase16.h +++ b/include/wine/winbase16.h @@ -567,4 +567,7 @@ BOOL16 WINAPI WriteProfileSection16(LPCSTR,LPCSTR); #define CURRENT_SP (((WORD *)NtCurrentTeb()->SystemReserved1)[0]) #define CURRENT_SS (((WORD *)NtCurrentTeb()->SystemReserved1)[1])
+typedef void (*wine_taskkill_func)(void); +extern void __wine_set_taskkill_func(wine_taskkill_func func); + #endif /* __WINE_WINE_WINBASE16_H */ -- 2.34.1
Le 30/01/2022 à 18:33, Fabian Maurer a écrit :
Any news on this? After a bit more thinking this doesn't seem too correct anymore to me, but I still don't know how to do it properly. We somehow must prevent the thunks from being freed as long as we still use them.
I thought https://source.winehq.org/git/wine.git/?a=commit;h=879ccd33572170e9bcb00d5a4... was suppose to fix it, didn't it? A+
participants (2)
-
Eric Pouech -
Fabian Maurer