We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 3334 - SDL_ShowMessageBox uses wrong index and accesses un-allocated memory
Summary: SDL_ShowMessageBox uses wrong index and accesses un-allocated memory
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: video (show other bugs)
Version: 2.0.4
Hardware: x86 Windows 10
: P2 normal
Assignee: Sam Lantinga
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-14 19:08 UTC by romain.lacroix
Modified: 2017-08-12 02:42 UTC (History)
1 user (show)

See Also:

Attachments
proposed patch to fix the bug (629 bytes, text/plain)
2016-05-14 19:08 UTC, romain.lacroix 		Details
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description romain.lacroix 2016-05-14 19:08:41 UTC 
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
Comment 1 JR 2017-06-19 02:54:11 UTC 
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.
Comment 2 Sam Lantinga 2017-08-12 02:42:55 UTC 
Patch applied, thanks!
https://hg.libsdl.org/SDL/rev/af4c3dc6b97f