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 5253 - SIGSEGV when using a NULL in 'title' field of SDL_MessageBoxData passing it to SDL_ShowMessageBox
Summary: SIGSEGV when using a NULL in 'title' field of SDL_MessageBoxData passing it t...
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: video (show other bugs)
Version: HG 2.0
Hardware: All All
: P2 normal
Assignee: Ozkan Sezer
QA Contact: Sam Lantinga
URL:
Keywords: target-2.0.14
Depends on:
Blocks:
 
Reported: 2020-08-11 07:49 UTC by vladius
Modified: 2020-12-10 08:22 UTC (History)
1 user (show)

See Also:

Attachments
messagebox NULL patch (5.93 KB, patch)
2020-12-09 19:27 UTC, Ozkan Sezer 		Details | Diff
messagebox NULL patch, V2 (8.56 KB, patch)
2020-12-09 20:35 UTC, Ozkan Sezer 		Details | Diff
Show Obsolete (1) 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 vladius 2020-08-11 07:49:12 UTC 
Using SDL as part of Unreal Engine. The Epic's developer has asked me to forward this bug in here. It is as follows:

Using struct SDL_MessageBoxData information. Initializing its 'title' field with NULL.
Passing this struct data to the SDL_ShowMessageBox function triggers a SIGSEGV.
The NULL 'message' field, however, does not trigger the signal.

Thank you.
Comment 1 Ryan C. Gordon 2020-11-18 16:27:46 UTC 
This is now fixed in https://hg.libsdl.org/SDL/rev/4743e38c51cb

--ryan.
Comment 2 Ozkan Sezer 2020-11-20 09:29:22 UTC 
As far as I can see, this issue is present in other platforms too, for
strlen(NULL) if nothing else. See, e.g. the windows and winrt sources
(search for '>title' and '>message'). OSX side may also be affected.
I fixed the os/2 side as of https://hg.libsdl.org/SDL/rev/2cbc1bc6c207
Comment 3 Sam Lantinga 2020-12-09 14:21:10 UTC 
Good point
Comment 4 Sam Lantinga 2020-12-09 14:23:11 UTC 
Ozkan, can you fix this for the other backends? Maybe this should be fixed at the higher level inside SDL_video.c?
Comment 5 Ozkan Sezer 2020-12-09 14:31:07 UTC 
(In reply to Sam Lantinga from comment #4)
> Ozkan, can you fix this for the other backends? Maybe this should be fixed
> at the higher level inside SDL_video.c?

Will try and look today or tomorrow.
Comment 6 Ozkan Sezer 2020-12-09 19:27:44 UTC 
Created attachment 4559 [details]
messagebox NULL patch

(In reply to Sam Lantinga from comment #4)
> Ozkan, can you fix this for the other backends? Maybe this should be fixed
> at the higher level inside SDL_video.c?

See the attached patch which does the following:

- SDL_video.c (SDL_ShowMessageBox): replace messageboxdata if title or
  message field is NULL.
- SDL_video.c (SDL_ShowSimpleMessageBox): set title or message to "" if
  either of them is NULL.
- SDL_x11messagebox.c: revert commit 4743e38c51cb
- SDL_os2messagebox.c: revert commit 2cbc1bc6c207
- SDL_bmessagebox.cc: add empty string check along with NULL check for
  title and message fields.
- SDL_windowsmessagebox.c (AddDialogString): remove NULL string check
- SDL_windowsmessagebox.c (AddDialogControl): add empty string check
  along with the NULL check.

Please review. (It's easy to miss things across different backends.)
Comment 7 Ozkan Sezer 2020-12-09 20:35:21 UTC 
Created attachment 4561 [details]
messagebox NULL patch, V2

Created attachment 4559 [details]
messagebox NULL patch

(In reply to Sam Lantinga from comment #4)
> Ozkan, can you fix this for the other backends? Maybe this should be fixed
> at the higher level inside SDL_video.c?

Revised v2 patch.  Please review:

- SDL_video.c (SDL_ShowMessageBox): replace messageboxdata if title or
  message field is NULL.

- SDL_video.c (SDL_ShowSimpleMessageBox): set title or message to "" if
  either of them is NULL for EMSCRIPTEN builds.

- SDL_bmessagebox.cc: add empty string check along with NULL check for
  title and message fields.

- SDL_windowsmessagebox.c (AddDialogString): remove NULL string check

- SDL_windowsmessagebox.c (AddDialogControl): add empty string check
  along with the NULL check.

- SDL_x11messagebox.c: revert commit 4743e38c51cb
- SDL_os2messagebox.c: revert commit 2cbc1bc6c207

- test/testmessage.c: Add NULL Title and NULL Msg tests.
Comment 8 Sam Lantinga 2020-12-10 04:26:16 UTC 
I think you can unconditionally replace the messageboxdata. This isn't a time sensitive code path, so I think making it clearer about what's happening is better.

This test is a little awkward:
*aMessageBoxData->message != '\0'
... maybe replace with this?
aMessageBoxData->message[0]

Otherwise looks good!
Comment 9 Ozkan Sezer 2020-12-10 08:22:23 UTC 
(In reply to Sam Lantinga from comment #8)
> I think you can unconditionally replace the messageboxdata. This isn't a
> time sensitive code path, so I think making it clearer about what's
> happening is better.
> 
> This test is a little awkward:
> *aMessageBoxData->message != '\0'
> ... maybe replace with this?
> aMessageBoxData->message[0]
> 
> Otherwise looks good!

Pushed https://hg.libsdl.org/SDL/rev/155a6d4ff25d
Closing as fixed.