| Summary: | SIGSEGV when using a NULL in 'title' field of SDL_MessageBoxData passing it to SDL_ShowMessageBox | ||
|---|---|---|---|
| Product: | SDL | Reporter: | vladius <vturbanov> |
| Component: | video | Assignee: | Ozkan Sezer <sezeroz> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | normal | ||
| Priority: | P2 | CC: | sezeroz |
| Version: | HG 2.0 | Keywords: | target-2.0.14 |
| Hardware: | All | ||
| OS: | All | ||
| Attachments: |
messagebox NULL patch
messagebox NULL patch, V2 |
||
|
Description
vladius
2020-08-11 07:49:12 UTC
This is now fixed in https://hg.libsdl.org/SDL/rev/4743e38c51cb --ryan. 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 Good point Ozkan, can you fix this for the other backends? Maybe this should be fixed at the higher level inside SDL_video.c? (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. 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.) 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. 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! (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. |