ValveSoftware / halflife

Half-Life 1 engine based games
Other
3.6k stars 598 forks source link

KeyValues::GetWString uses sizeof( wbuf ) instead of ARRAYSIZE( wbuf ) #1770

Open SamVanheer opened 7 years ago

SamVanheer commented 7 years ago

The KeyValues class used by the engine and Valve compiled games handles conversion to WString incorrectly. Instead of passing the size of the temporary buffer in characters, it passes the size in bytes. This can cause buffer overflows that can corrupt the stack. Newer GCC versions will trigger runtime check failures when these calls occur. I fixed these issues myself in Svengine since the server browser was using this method and triggering the checks.

This is what the code looks like:

 case TYPE_INT;
 case TYPE_PTR:
          swprintf(wbuf, sizeof( wbuf ), L"%d", dat->m_iValue);
          break;

TYPE_FLOAT seems to be using ARRAYSIZE, as expected.

Do note that the GetLocalizedFromANSI in this method correctly uses sizeof, since that method expects the size in bytes, not the size in characters.

Related to this, i also found another incorrect use of swprintf in Condition Zero's client code: CCSClientScoreBoardDialog::UpdateTeamInfo passes size 24 for buffer val, which is 6 characters large.

Additionally, CClientMOTD::Activate( const wchar_t* title, const wchar_t* msg ) writes the message to the temporary file as a byte array of wcslen( msg ), meaning it will write only half of the actual string. I'm not sure if it will even work if used like this, however this method doesn't appear to be called anywhere so it is probably not a problem.

SamVanheer commented 5 years ago

@mikela-valve Looks like these issues still exist in the engine and the games' code.