Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Use after return for stack allocated object #7926

Closed
wants to merge 3 commits into from

Conversation

@nielsmh
Copy link
Contributor

nielsmh commented Jan 12, 2020

GetCurrentSong() returns a copy of an object containing a string buffer. A pointer to this buffer can get stored into the global string formatting parameters, in which it can escape the function that called GetCurrentSong(). This means a pointer to an invalid object is stuck in a global variable. Solve it by returning a reference instead, and make a fixed no-song object that a reference to will stay valid.

nielsmh added 3 commits Jan 12, 2020
GetCurrentSong() returns a copy of an object containing a string buffer. A pointer to this buffer can get stored into the global string formatting parameters, in which it can escape the function that called GetCurrentSong(). This means a pointer to an invalid object is stuck in a global variable. Solve it by returning a reference instead, and make a fixed no-song object that a reference to will stay valid.
Pointer to string buffer on the stack could escape through the global string parameter buffer. Clear out the parameter after drawing, before return.
@nielsmh
Copy link
Contributor Author

nielsmh commented Jan 12, 2020

Made another, shorter fix, that can theoretically still let a stack object pointer escape, but in practice shouldn't. (I don't think we have more than one thread accessing the string formatting data.)

Really, I think this problem is entirely theoretical. While a pointer to a stack object does escape and become invalid, it should never be accessed, unless code somewhere else forgets to fill out its string parameters.

@Milek7
Copy link
Contributor

Milek7 commented Jan 12, 2020

No, if the pointer escapes but is never accessed it doesn't matter, but there is different issue. As GetCurrentSong returns by copy, struct returned by it becomes invalid as soon as SetDParamStr returns. But SetDParamStr stores pointer which is used inside DrawString below, at which point this pointer is invalid.

This is alternative fix:

diff --git a/src/music_gui.cpp b/src/music_gui.cpp
index 7bae6dc1e..d15025ea3 100644
--- a/src/music_gui.cpp
+++ b/src/music_gui.cpp
@@ -732,11 +732,12 @@ struct MusicWindow : public Window {
                        case WID_M_TRACK_NAME: {
                                GfxFillRect(r.left, r.top + 1, r.right - 1, r.bottom, PC_BLACK);
                                StringID str = STR_MUSIC_TITLE_NONE;
+                               MusicSystem::PlaylistEntry entry(_music.GetCurrentSong());
                                if (BaseMusic::GetUsedSet()->num_available == 0) {
                                        str = STR_MUSIC_TITLE_NOMUSIC;
                                } else if (_music.IsPlaying()) {
                                        str = STR_MUSIC_TITLE_NAME;
-                                       SetDParamStr(0, _music.GetCurrentSong().songname);
+                                       SetDParamStr(0, entry.songname);
                                }
                                DrawString(r.left + WD_FRAMERECT_LEFT, r.right - WD_FRAMERECT_RIGHT, r.top + WD_FRAMERECT_TOP, str, TC_FROMSTRING, SA_HOR_CENTER);
                                break;
@nielsmh
Copy link
Contributor Author

nielsmh commented Jan 12, 2020

Let's use #7928 instead

@nielsmh nielsmh closed this Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.