search

libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
9.91k stars 1.83k forks source link

Crash with long IME compositions on Windows #5729

Closed Guldoman closed 2 years ago

Guldoman commented 2 years ago

While comparing the IME behavior on Windows (#5398) against my PR #5617, I noticed that when doing long compositions (>64 bytes) a crash happened in SDL (2.0.22):

warning: Heap block at 000001F13D92F9E0 modified at 000001F13D92FA30 past requested size of 40

Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
0x00007ffda1f4a313 in ntdll!RtlRegisterSecureMemoryCacheCallback () from /c/Windows/SYSTEM32/ntdll.dll
(gdb) bt
#0  0x00007ffda1f4a313 in ntdll!RtlRegisterSecureMemoryCacheCallback () from /c/Windows/SYSTEM32/ntdll.dll
#1  0x00007ffda1f4699f in ntdll!RtlZeroHeap () from /c/Windows/SYSTEM32/ntdll.dll
#2  0x00007ffda1f0ccc2 in ntdll!memset () from /c/Windows/SYSTEM32/ntdll.dll                                                                    #3  0x00007ffda1f491c1 in ntdll!RtlRegisterSecureMemoryCacheCallback () from /c/Windows/SYSTEM32/ntdll.dll
#4  0x00007ffda1e75cc1 in ntdll!RtlGetCurrentServiceSessionId () from /c/Windows/SYSTEM32/ntdll.dll                                             #5  0x00007ffda1e75b74 in ntdll!RtlGetCurrentServiceSessionId () from /c/Windows/SYSTEM32/ntdll.dll
#6  0x00007ffda1e747b1 in ntdll!RtlFreeHeap () from /c/Windows/SYSTEM32/ntdll.dll                                                               #7  0x00007ffda1199c9c in msvcrt!free () from /c/Windows/System32/msvcrt.dll
#8  0x00007ffd64b51d30 in SDL_free_REAL (ptr=0x1f13d92f9f0)
    at .../SDL2-2.0.22/src/stdlib/SDL_malloc.c:5432
#9  0x00007ffd64cd5d15 in IME_SendEditingEvent (videodata=0x1f1371e5820)
    at .../SDL2-2.0.22/src/video/windows/SDL_windowskeyboard.c:882
#10 0x00007ffd64cd63cd in IME_HandleMessage (hwnd=0x804a4, msg=271, wParam=12354, lParam=0x1001fe1b8, videodata=0x1f1371e5820)
    at .../SDL2-2.0.22/src/video/windows/SDL_windowskeyboard.c:1036
#11 0x00007ffd64cd0aea in WIN_WindowProc (hwnd=0x804a4, msg=271, wParam=12354, lParam=441)
    at .../SDL2-2.0.22/src/video/windows/SDL_windowsevents.c:666
#12 0x00007ffda1a8e858 in USER32!CallWindowProcW () from /c/Windows/System32/USER32.dll
#13 0x00007ffda1a8e299 in USER32!DispatchMessageW () from /c/Windows/System32/USER32.dll
#14 0x00007ffd64cd30f8 in WIN_PumpEvents (_this=0x1f1371e5200)
    at .../SDL2-2.0.22/src/video/windows/SDL_windowsevents.c:1535
#15 0x00007ffd64acd85a in SDL_PumpEventsInternal (push_sentinel=SDL_TRUE)
    at .../SDL2-2.0.22/src/events/SDL_events.c:847
#16 0x00007ffd64acdc69 in SDL_WaitEventTimeout_REAL (event=0x1001fe630, timeout=0)
    at .../SDL2-2.0.22/src/events/SDL_events.c:1024
#17 0x00007ffd64acd8fd in SDL_PollEvent_REAL (event=0x1001fe630)
    at .../SDL2-2.0.22/src/events/SDL_events.c:886
#18 0x00007ffd64abfb78 in SDL_PollEvent (a=0x1001fe630)
...

As this looked like a null terminator issue I added a bit of space for it, but a different crash happened:

warning: Heap block at 00000295F8725D20 modified at 00000295F8725D70 past requested size of 40

Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
0x00007ffda1f4a313 in ntdll!RtlRegisterSecureMemoryCacheCallback () from /c/Windows/SYSTEM32/ntdll.dll
(gdb) bt
#0  0x00007ffda1f4a313 in ntdll!RtlRegisterSecureMemoryCacheCallback () from /c/Windows/SYSTEM32/ntdll.dll
#1  0x00007ffda1f4699f in ntdll!RtlZeroHeap () from /c/Windows/SYSTEM32/ntdll.dll
#2  0x00007ffda1f0ccc2 in ntdll!memset () from /c/Windows/SYSTEM32/ntdll.dll
#3  0x00007ffda1f491c1 in ntdll!RtlRegisterSecureMemoryCacheCallback () from /c/Windows/SYSTEM32/ntdll.dll                                      #4  0x00007ffda1e75cc1 in ntdll!RtlGetCurrentServiceSessionId () from /c/Windows/SYSTEM32/ntdll.dll
#5  0x00007ffda1e75b74 in ntdll!RtlGetCurrentServiceSessionId () from /c/Windows/SYSTEM32/ntdll.dll
#6  0x00007ffda1e747b1 in ntdll!RtlFreeHeap () from /c/Windows/SYSTEM32/ntdll.dll
#7  0x00007ffda1199c9c in msvcrt!free () from /c/Windows/System32/msvcrt.dll
#8  0x00007ffd64b51d30 in SDL_free_REAL (ptr=0x295f8725d30)
    at .../SDL2-2.0.22/src/stdlib/SDL_malloc.c:5432
#9  0x00007ffd64cd5816 in IME_GetCompositionString (videodata=0x295f8725820, himc=0x38b02b1, string=8)
    at .../SDL2-2.0.22/src/video/windows/SDL_windowskeyboard.c:776
#10 0x00007ffd64cd63c9 in IME_HandleMessage (hwnd=0x310768, msg=271, wParam=12354, lParam=0x9a165fe8a8, videodata=0x295f8725820)
    at .../SDL2-2.0.22/src/video/windows/SDL_windowskeyboard.c:1035
#11 0x00007ffd64cd0aea in WIN_WindowProc (hwnd=0x310768, msg=271, wParam=12354, lParam=441)
    at .../SDL2-2.0.22/src/video/windows/SDL_windowsevents.c:666
#12 0x00007ffda1a8e858 in USER32!CallWindowProcW () from /c/Windows/System32/USER32.dll
#13 0x00007ffda1a8e299 in USER32!DispatchMessageW () from /c/Windows/System32/USER32.dll
#14 0x00007ffd64cd30f8 in WIN_PumpEvents (_this=0x295f8725200)
    at .../SDL2-2.0.22/src/video/windows/SDL_windowsevents.c:1535
#15 0x00007ffd64acd85a in SDL_PumpEventsInternal (push_sentinel=SDL_TRUE)
    at .../SDL2-2.0.22/src/events/SDL_events.c:847
#16 0x00007ffd64acdc69 in SDL_WaitEventTimeout_REAL (event=0x0, timeout=379)
    at .../SDL2-2.0.22/src/events/SDL_events.c:1024
#17 0x00007ffd64abfbc3 in SDL_WaitEventTimeout (a=0x0, b=379)
    at .../SDL2-2.0.22/src/dynapi/SDL_dynapi_procs.h:155
...

As this one also looked like a null terminator problem (ImmGetCompositionStringW returns the length without it), I added space for that too.

So this seems to fix the crashes:

diff --git a/src/video/windows/SDL_windowskeyboard.c b/src/video/windows/SDL_windowskeyboard.c
index b9874a543..c71798fa2 100644
--- a/src/video/windows/SDL_windowskeyboard.c
+++ b/src/video/windows/SDL_windowskeyboard.c
@@ -65,7 +65,7 @@ WIN_InitKeyboard(_THIS)
     data->ime_hwnd_current = 0;
     data->ime_himc = 0;
     data->ime_composition_length = 32 * sizeof(WCHAR);
-    data->ime_composition = (WCHAR*)SDL_malloc(data->ime_composition_length);
+    data->ime_composition = (WCHAR*)SDL_malloc(data->ime_composition_length + sizeof(WCHAR));
     data->ime_composition[0] = 0;
     data->ime_readingstring[0] = 0;
     data->ime_cursor = 0;
@@ -813,7 +813,7 @@ IME_GetCompositionString(SDL_VideoData *videodata, HIMC himc, DWORD string)

         length = ImmGetCompositionStringW(himc, GCS_COMPATTR, NULL, 0);
         if (length > 0) {
-            Uint8* attributes = (Uint8*)SDL_malloc(length);
+            Uint8* attributes = (Uint8*)SDL_malloc(length + sizeof(WCHAR));
             ImmGetCompositionString(himc, GCS_COMPATTR, attributes, length);

             for (start = 0; start < length; ++start) {
@@ -863,7 +863,7 @@ IME_SendEditingEvent(SDL_VideoData *videodata)
         size_t len = SDL_min(SDL_wcslen(videodata->ime_composition), (size_t)videodata->ime_cursor);

         size += sizeof(videodata->ime_readingstring);
-        buffer = (WCHAR*)SDL_malloc(size);
+        buffer = (WCHAR*)SDL_malloc(size + sizeof(WCHAR));
         buffer[0] = 0;

         SDL_wcslcpy(buffer, videodata->ime_composition, len + 1);
@@ -871,7 +871,7 @@ IME_SendEditingEvent(SDL_VideoData *videodata)
         SDL_wcslcat(buffer, &videodata->ime_composition[len], size);
     }
     else {
-        buffer = (WCHAR*)SDL_malloc(size);
+        buffer = (WCHAR*)SDL_malloc(size + sizeof(WCHAR));
         buffer[0] = 0;
         SDL_wcslcpy(buffer, videodata->ime_composition, size);
     }

Let me know if that looks reasonable and I'll PR it.

slouken commented 2 years ago

Yup, that looks great.