Repository navigation
fix overflow when control binding text exceeds 40 chars - #7987
Goober5000 wants to merge 2 commits into
Conversation
Goober5000
added
fix
Goober5000
added
the
ui
| static unsigned int Defaults_cycle_pos = 0; // the controls preset that was last selected | ||
| static UI_TIMESTAMP bound_timestamp; | ||
| static char bound_string[40]; | ||
| static char bound_string[256]; |
There was a problem hiding this comment.
Erm, why are we still using a magic number here? Maybe use a constant.
| if (done) { | ||
| // done with binding mode, clean up and prepare for display | ||
| font::force_fit_string(bound_string, 39, Conflict_wnd_coords[gr_screen.res][CONTROL_W_COORD]); | ||
| font::force_fit_string(bound_string, 255, Conflict_wnd_coords[gr_screen.res][CONTROL_W_COORD]); |
7e8f8a4 to
50f5c4a
Compare
dustinspace217
left a comment
dustinspace217
left a comment
There was a problem hiding this comment.
The new constants cover z64555's notes, and the SCP_string change fixes the crash: I reproduced it on master with a long Joy-0 translation, and this build gets through the same mission.
One thing to change: translate_key now returns a pointer into a static SCP_string. If a script's dialog hook calls ba.replaceTokens while the unbound-key popup is open, the inner call can reallocate that string and free the memory the outer pointer still uses before buf += ptr. Returning SCP_string by value, or copying ptr into a local before the popup, fixes it.
Two smaller notes, fine to leave for later: the directive key line still copies into a 256-byte buffer (missiontraining.cpp:299), and the controls screen still copies into 256-byte buffers before force_fit_string. Both only matter for very long translations. Also, this isn't translation-only: by my count from the code, "Alt-Shift-Backspace or Joy-0 Hat Forward" is exactly 40 characters, so a player who binds that can hit the old crash with no mod at all.
Goober5000
changed the title
A mod that translates the "Joy-N " prefix (or other binding text) to something longer could overflow the 40-byte buffer in translate_key(), which strcpy_s treats as a fatal error. Build the text in an SCP_string instead. Also enlarge bound_string in the control config screen (it is width-fitted for display anyway) and convert the gameplay help binding line to SCP_string for consistency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
50f5c4a to
5e59e82
Compare
|
@dustinspace217 updated again |
dustinspace217
left a comment
dustinspace217
left a comment
There was a problem hiding this comment.
Thanks for the update. Given the scope of the change I didn't re-run anything, but the change looks good!
translate_key()built the text for a$token$in training messages, e.g. "Alt-Shift-Backspace or Joy-0 Hat Forward", in a 40-byte static buffer, andstrcpy_streats an overflow as a fatal error. This can happen without any mod: the example above is exactly 40 characters, so it doesn't fit with the terminator. A mod that translates theJoy-Nprefix or other binding text to something longer makes it much easier to hit.Changes:
translate_key()now takes anSCP_string &outand returnsbool, instead of returning a pointer into a static buffer. This removes the length limit and also avoids a reentrancy problem: the unbound-key popup can run script hooks that callba.replaceTokens, which would overwrite (or, with anSCP_string, reallocate) the static buffer while the caller still holds a pointer into it.bound_stringin the control config screen is enlarged to 256 bytes. It is fitted to the display width anyway.force_fit_stringin the control config screen are now named constants.SCP_stringfor consistency.Not addressed here: the directive key line in
HudGaugeDirectives::render()still copies into a 256-byte buffer forsplit_str_once(), which will be handled separately.