| Summary: | SDL_ShowMessageBox uses wrong index and accesses un-allocated memory | ||
|---|---|---|---|
| Product: | SDL | Reporter: | romain.lacroix |
| Component: | video | Assignee: | Sam Lantinga <slouken> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | normal | ||
| Priority: | P2 | CC: | wendelloni |
| Version: | 2.0.4 | ||
| Hardware: | x86 | ||
| OS: | Windows 10 | ||
| Attachments: | proposed patch to fix the bug | ||
I encountered this bug in 2.0.5. I applied the patch and it seems to improve the situation. However, I always get a value of 2 back (the value of SDL_MESSAGEBOX_BUTTON_ESCAPEKEY_DEFAULT) when I hit escape regardless of which button I assigned this flag to. I looked at the SDL code a bit more and it seems to use existing Windows APIs to make the dialog and handle the default/Enter key situation and I don't see any way to catch escape so I suppose this is simply a limitation of which I was previously unaware. At least I don't get garbage back in the one and two button cases now. Patch applied, thanks! https://hg.libsdl.org/SDL/rev/af4c3dc6b97f |
Created attachment 2455 [details] proposed patch to fix the bug For the windows implementation of SDL_ShowMessageBox() : ./src/video/windows/SDL_windowsmessagebox.c:345 WIN_ShowMessageBox() The implementation in 2.0.4 uses "button index" for parameter "id" of function AddDialogButton(). It then expects the value provided in param wParam of function MessageBoxDialogProc() to be a valid index of a button. It uses this value to index in the array of buttons when DialogBoxIndirect() returns (line 474 : *buttonid = buttons[which].buttonid;) However, when dismissing this box with Escape, the return value of DialogBoxIndirect will be SDL_MESSAGEBOX_BUTTON_ESCAPEKEY_DEFAULT (=2) which is not always a valid index of array buttons. When the array buttons has a length less or equal than 2, the memory access is invalid; I can see that the value written to *buttonId is uninitialized memory (random value). The fix I propose : use value "buttonid" (field of button) for parameter "id" of AddDialogButton(), then copy return value of DialogBoxIndirect() in *buttonid. This way, we will not use an out-of-bounds index in array buttons. Implemented in attached diff. Cheers