ValveSoftware
[HL][TFC] Crash while parsing command menu (menu item length)
Not related to https://github.com/ValveSoftware/halflife/issues/1730 apart from that it also happens in the command menu.
There have been reports by French TFC players experiencing a crash when trying to join a server, with the proposed work-around being to delete the "tfc_french" folder. I tried to reproduce the crash and find out what's causing it, with success.
It seems to be HL25 specific, so not happening when using the steam_legacy branch.
Quoting myself:
I fixed the "tfc_french" folder. The culprit was the "commandmenu.txt". There is a bug in the current version of TFC which causes it to crash if a menu item is longer than 31 characters. The old version of TFC (steam_legacy) is not affected. So this is a bug introduced with the 25th anniversary update of Half-Life.
On a side note, the French command menu file uses ANSI instead of UTF-8. Diacritics show up, so I guess it's not an issue.
Related discussion: https://steamcommunity.com/app/20/discussions/0/601891815092683482/
Diff: french_commandmenu.diff.txt I only removed the single quotes to fit more letters into the maximum of 31 characters. The command string can be longer.
Test case 1
"8" "You finish building the dispenser." "say OK"
A buffer overflow occurs if the original string is too long in the button text.
Test case 2
"8" "#Dispenser_finish" "say OK"
A buffer overflow occurs if the substituted string is too long in the button text.
These are triggered in two different code segments of "CHudTextMessage::LocaliseTextString" in "cl_dll/text_message.cpp".
I'm looking at cl_dll/vgui_TeamFortressViewport.cpp#L817-L821 and cl_dll/text_message.cpp#L47-L85 . This is the client DLL for Half-Life.
At CHudTextMessage::LocaliseTextString , line 85 it adds a zero behind the current position.
dst++, src++;
*dst = 0;
If the source string is longer than the destination, then this means a zero is added behind the buffer (buffer overflow).
Though I don't know if this is actually causing the crash, and also this is the HL client DLL, not the one for TFC.
vgui_TeamFortressViewport.cpp#L817-L821
cText[31] = '\0';
// Get the button text
pfile = gEngfuncs.COM_ParseFile(pfile, token);
CHudTextMessage::LocaliseTextString( token, cText, sizeof( cText ) );
As a work-around, "sizeof( cText )" could be decreased by one. But the actual bug is in "CHudTextMessage::LocaliseTextString" as described above.
text_message.cpp#L47-L87
char *CHudTextMessage::LocaliseTextString( const char *msg, char *dst_buffer, int buffer_size )
{
int len = buffer_size;
char *dst = dst_buffer;
for ( char *src = (char*)msg; *src != 0 && buffer_size > 0; buffer_size-- )
{
if ( *src == '#' )
[...]
else
{
*dst = *src;
dst++, src++;
*dst = 0;
}
}
The "if" part looks dangerous as well. If a "#" is found, and a corresponding entry in "titles.txt", it copies to the destination without checking if the buffer is long enough.
// copy string into message over the msg name
for ( char *wsrc = (char*)clmsg->pMessage; *wsrc != 0; wsrc++, dst++ )
{
*dst = *wsrc;
}
*dst = 0;
The call to "CHudTextMessage::LocaliseTextString" was added to "vgui_TeamFortressViewport.cpp" with the HL25 SDK Update.
Test case 1
"8" "You finish building the dispenser." "say OK"
A buffer overflow occurs if the original string is too long in the button text.
Test case 2
"8" "#Dispenser_finish" "say OK"
A buffer overflow occurs if the substituted string is too long in the button text.
These are triggered in two different code segments of "CHudTextMessage::LocaliseTextString" in "cl_dll/text_message.cpp".
Proposing a fix for "CHudTextMessage::LocaliseTextString" in "cl_dll/text_message.cpp". It's untested, maybe someone could check if it works? @SamVanheer @kisak-valve @shawns-valve
https://github.com/ValveSoftware/halflife/compare/b1b5cf5892918535619b2937bb927e46cb097ba1...pizzahut2:halflife:patch-1
In the latest English build version 10210 of Half-Life, if a command menu button text exceeds 32 characters (including the null terminator), the game writes past the end of the fixed size cText[32] buffer, causing a crash. This does not appear to be caused by using non-English characters, but some locale specific builds of TFC, such as German and French, will immediately crash to desktop upon creating or connecting to a server on the Windows build, because the button text simply exceeds the buffer size. I was unable to reproduce this issue on Linux or in Counter-Strike 1.6 on Windows. I have tested the proposed patch to CHudTextMessage::LocaliseTextString, and it appears to fix the crash reliably.
