From 4d74a7f8be1216f9ea8ce0fea35f042c8975b315 Mon Sep 17 00:00:00 2001 From: "seer-by-sentry[bot]" <157164994+seer-by-sentry[bot]@users.noreply.github.com> Date: Sun, 11 Jan 2026 03:07:08 +0000 Subject: [PATCH] Robustify path handling and prevent buffer overflows --- .../Source/Common/System/MiniDumper.cpp | 7 +++- .../GameEngine/Source/Common/GlobalData.cpp | 39 ++++++++++++++++--- Generals/Code/Main/WinMain.cpp | 18 +++++++-- .../GameEngine/Source/Common/GlobalData.cpp | 37 +++++++++++------- GeneralsMD/Code/Main/WinMain.cpp | 18 +++++++-- 5 files changed, 88 insertions(+), 31 deletions(-) diff --git a/Core/GameEngine/Source/Common/System/MiniDumper.cpp b/Core/GameEngine/Source/Common/System/MiniDumper.cpp index 21ab80e578a..cccabab12c6 100644 --- a/Core/GameEngine/Source/Common/System/MiniDumper.cpp +++ b/Core/GameEngine/Source/Common/System/MiniDumper.cpp @@ -146,9 +146,12 @@ void MiniDumper::Initialize(const AsciiString& userDirPath) } DWORD executableSize = ::GetModuleFileNameW(NULL, m_executablePath, ARRAY_SIZE(m_executablePath)); - if (executableSize == 0 || executableSize == ARRAY_SIZE(m_executablePath)) + // GetModuleFileNameW returns 0 on failure, or buffer size if path was truncated + // In Windows Vista+, if buffer is too small, it returns the buffer size and sets ERROR_INSUFFICIENT_BUFFER + // We must check both conditions to ensure we have a valid, non-truncated path + if (executableSize == 0 || executableSize >= ARRAY_SIZE(m_executablePath)) { - DEBUG_LOG(("MiniDumper::Initialize: Could not get executable file name. Returned value=%u", executableSize)); + DEBUG_LOG(("MiniDumper::Initialize: Could not get executable file name or path was truncated. Returned value=%u, Buffer size=%u", executableSize, ARRAY_SIZE(m_executablePath))); return; } diff --git a/Generals/Code/GameEngine/Source/Common/GlobalData.cpp b/Generals/Code/GameEngine/Source/Common/GlobalData.cpp index 78e844ffbaf..c72245aa612 100644 --- a/Generals/Code/GameEngine/Source/Common/GlobalData.cpp +++ b/Generals/Code/GameEngine/Source/Common/GlobalData.cpp @@ -1169,12 +1169,39 @@ void GlobalData::parseGameDataDefinition( INI* ini ) char temp[_MAX_PATH]; if (::SHGetSpecialFolderPath(NULL, temp, CSIDL_PERSONAL, true)) { - if (temp[strlen(temp)-1] != '\\') - strcat(temp, "\\"); - strcat(temp, TheWritableGlobalData->m_userDataLeafName.str()); - strcat(temp, "\\"); - CreateDirectory(temp, NULL); - TheWritableGlobalData->m_userDataDir = temp; + size_t tempLen = strlen(temp); + + // Ensure we have enough space for backslash + user data leaf name + backslash + null terminator + if (tempLen > 0 && tempLen < _MAX_PATH - 1) + { + // Add trailing backslash if missing + if (temp[tempLen - 1] != '\\') + { + if (tempLen + 1 < _MAX_PATH) + { + temp[tempLen] = '\\'; + temp[tempLen + 1] = '\0'; + tempLen++; + } + else + { + // Path too long, cannot proceed + return; + } + } + + // Check if we have enough space to append the user data leaf name + const char* leafName = TheWritableGlobalData->m_userDataLeafName.str(); + size_t leafLen = strlen(leafName); + + if (tempLen + leafLen + 2 < _MAX_PATH) // +2 for backslash and null terminator + { + strcat(temp, leafName); + strcat(temp, "\\"); + CreateDirectory(temp, NULL); + TheWritableGlobalData->m_userDataDir = temp; + } + } } // override INI values with user preferences diff --git a/Generals/Code/Main/WinMain.cpp b/Generals/Code/Main/WinMain.cpp index 3c34a3e81cf..d810b31a5ab 100644 --- a/Generals/Code/Main/WinMain.cpp +++ b/Generals/Code/Main/WinMain.cpp @@ -795,12 +795,22 @@ Int APIENTRY WinMain( HINSTANCE hInstance, HINSTANCE hPrevInstance, /// @todo remove this force set of working directory later Char buffer[ _MAX_PATH ]; - GetModuleFileName( NULL, buffer, sizeof( buffer ) ); - if (Char *pEnd = strrchr(buffer, '\\')) + DWORD pathLen = GetModuleFileName( NULL, buffer, sizeof( buffer ) ); + + // Check if GetModuleFileName succeeded and path wasn't truncated + if (pathLen > 0 && pathLen < sizeof( buffer )) { - *pEnd = 0; + if (Char *pEnd = strrchr(buffer, '\\')) + { + *pEnd = 0; + + // Only set current directory if the resulting path is valid + if (strlen(buffer) > 0) + { + ::SetCurrentDirectory(buffer); + } + } } - ::SetCurrentDirectory(buffer); #ifdef RTS_DEBUG diff --git a/GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp b/GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp index 17cecf6bc09..283ba509020 100644 --- a/GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp +++ b/GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp @@ -1049,26 +1049,33 @@ GlobalData::GlobalData() char temp[_MAX_PATH + 1]; if (::SHGetSpecialFolderPath(NULL, temp, CSIDL_PERSONAL, true)) { - AsciiString myDocumentsDirectory = temp; + // Ensure null termination and validate the path wasn't truncated + temp[_MAX_PATH] = '\0'; + size_t tempLen = strlen(temp); + + if (tempLen > 0 && tempLen < _MAX_PATH) + { + AsciiString myDocumentsDirectory = temp; - if (myDocumentsDirectory.getCharAt(myDocumentsDirectory.getLength() -1) != '\\') - myDocumentsDirectory.concat( '\\' ); + if (myDocumentsDirectory.getCharAt(myDocumentsDirectory.getLength() -1) != '\\') + myDocumentsDirectory.concat( '\\' ); - AsciiString leafName; + AsciiString leafName; - if ( !GetStringFromRegistry( "", "UserDataLeafName", leafName ) ) - { - // Use something, anything - // [MH] had to remove this, otherwise mapcache build step won't run... DEBUG_CRASH( ( "Could not find registry key UserDataLeafName; defaulting to \"Command and Conquer Generals Zero Hour Data\" " ) ); - leafName = "Command and Conquer Generals Zero Hour Data"; - } + if ( !GetStringFromRegistry( "", "UserDataLeafName", leafName ) ) + { + // Use something, anything + // [MH] had to remove this, otherwise mapcache build step won't run... DEBUG_CRASH( ( "Could not find registry key UserDataLeafName; defaulting to \"Command and Conquer Generals Zero Hour Data\" " ) ); + leafName = "Command and Conquer Generals Zero Hour Data"; + } - myDocumentsDirectory.concat( leafName ); - if (myDocumentsDirectory.getCharAt( myDocumentsDirectory.getLength() - 1) != '\\') - myDocumentsDirectory.concat( '\\' ); + myDocumentsDirectory.concat( leafName ); + if (myDocumentsDirectory.getCharAt( myDocumentsDirectory.getLength() - 1) != '\\') + myDocumentsDirectory.concat( '\\' ); - CreateDirectory(myDocumentsDirectory.str(), NULL); - m_userDataDir = myDocumentsDirectory; + CreateDirectory(myDocumentsDirectory.str(), NULL); + m_userDataDir = myDocumentsDirectory; + } } //-allAdvice feature diff --git a/GeneralsMD/Code/Main/WinMain.cpp b/GeneralsMD/Code/Main/WinMain.cpp index 3277a00b4ff..d885acce808 100644 --- a/GeneralsMD/Code/Main/WinMain.cpp +++ b/GeneralsMD/Code/Main/WinMain.cpp @@ -856,12 +856,22 @@ Int APIENTRY WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, /// @todo remove this force set of working directory later Char buffer[_MAX_PATH]; - GetModuleFileName(NULL, buffer, sizeof(buffer)); - if (Char* pEnd = strrchr(buffer, '\\')) + DWORD pathLen = GetModuleFileName(NULL, buffer, sizeof(buffer)); + + // Check if GetModuleFileName succeeded and path wasn't truncated + if (pathLen > 0 && pathLen < sizeof(buffer)) { - *pEnd = 0; + if (Char* pEnd = strrchr(buffer, '\\')) + { + *pEnd = 0; + + // Only set current directory if the resulting path is valid + if (strlen(buffer) > 0) + { + ::SetCurrentDirectory(buffer); + } + } } - ::SetCurrentDirectory(buffer); #ifdef RTS_DEBUG