From 824f38907c9eaae9057ea68cc76a522896a3e2f1 Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Fri, 9 Jan 2026 07:55:45 +0100 Subject: [PATCH 1/3] Fixing Use After Free bugs --- src/SBaseCommon.cpp | 207 +++++++++++++++++++++++------------- src/SFileCompactArchive.cpp | 12 ++- src/SFileCreateArchive.cpp | 3 +- src/SFileOpenArchive.cpp | 33 +++--- src/StormCommon.h | 4 +- src/StormLib.h | 3 + test/StormTest.cpp | 27 ++--- 7 files changed, 185 insertions(+), 104 deletions(-) diff --git a/src/SBaseCommon.cpp b/src/SBaseCommon.cpp index 4d56502..6f48ca8 100644 --- a/src/SBaseCommon.cpp +++ b/src/SBaseCommon.cpp @@ -932,15 +932,22 @@ TMPQFile * CreateFileHandle(TMPQArchive * ha, TFileEntry * pFileEntry) hf->ha = ha; // If the called entered a file entry, we also copy informations from the file entry - if(ha != NULL && pFileEntry != NULL) + if(ha != NULL) { - // Set the raw position and MPQ position - hf->RawFilePos = FileOffsetFromMpqOffset(ha, pFileEntry->ByteOffset); - hf->MpqFilePos = pFileEntry->ByteOffset; + // Increment number of open files in the archive handle + ha->dwFileCount++; - // Set the data size - hf->dwDataSize = pFileEntry->dwFileSize; - hf->pFileEntry = pFileEntry; + // Add the file entry to the handle + if(pFileEntry != NULL) + { + // Set the raw position and MPQ position + hf->RawFilePos = FileOffsetFromMpqOffset(ha, pFileEntry->ByteOffset); + hf->MpqFilePos = pFileEntry->ByteOffset; + + // Set the data size + hf->dwDataSize = pFileEntry->dwFileSize; + hf->pFileEntry = pFileEntry; + } } } @@ -1691,75 +1698,6 @@ DWORD WriteMpqDataMD5( return dwErrCode; } -// Frees the structure for MPQ file -void FreeFileHandle(TMPQFile *& hf) -{ - if(hf != NULL) - { - // If we have patch file attached to this one, free it first - if(hf->hfPatch != NULL) - FreeFileHandle(hf->hfPatch); - - // Then free all buffers allocated in the file structure - if(hf->pbFileData != NULL) - STORM_FREE(hf->pbFileData); - if(hf->pPatchInfo != NULL) - STORM_FREE(hf->pPatchInfo); - if(hf->SectorOffsets != NULL) - STORM_FREE(hf->SectorOffsets); - if(hf->SectorChksums != NULL) - STORM_FREE(hf->SectorChksums); - if(hf->hctx != NULL) - STORM_FREE(hf->hctx); - if(hf->pbFileSector != NULL) - STORM_FREE(hf->pbFileSector); - if(hf->pStream != NULL) - FileStream_Close(hf->pStream); - STORM_FREE(hf); - hf = NULL; - } -} - -// Frees the MPQ archive -void FreeArchiveHandle(TMPQArchive *& ha) -{ - if(ha != NULL) - { - // First of all, free the patch archive, if any - if(ha->haPatch != NULL) - FreeArchiveHandle(ha->haPatch); - - // Free the patch prefix, if any - if(ha->pPatchPrefix != NULL) - STORM_FREE(ha->pPatchPrefix); - - // Close the file stream - FileStream_Close(ha->pStream); - ha->pStream = NULL; - - // Free the file names from the file table - if(ha->pFileTable != NULL) - { - for(DWORD i = 0; i < ha->dwFileTableSize; i++) - { - if(ha->pFileTable[i].szFileName != NULL) - STORM_FREE(ha->pFileTable[i].szFileName); - ha->pFileTable[i].szFileName = NULL; - } - - // Then free all buffers allocated in the archive structure - STORM_FREE(ha->pFileTable); - } - - if(ha->pHashTable != NULL) - STORM_FREE(ha->pHashTable); - if(ha->pHetTable != NULL) - FreeHetTable(ha->pHetTable); - STORM_FREE(ha); - ha = NULL; - } -} - bool IsInternalMpqFileName(const char * szFileName) { if(szFileName != NULL && szFileName[0] == '(') @@ -1859,6 +1797,123 @@ void CalculateDataBlockHash(void * pvDataBlock, DWORD cbDataBlock, LPBYTE md5_ha md5_done(&md5_state, md5_hash); } +//----------------------------------------------------------------------------- +// Free the handle structures + +static void DeleteArchiveHandle(TMPQArchive * ha) +{ + // Sanity check + assert(ha->dwFileCount == 0 && ha->dwRefCount == 0); + + // Invalidate the add file callback so it won't be called + // when saving (listfile) and (attributes) + ha->pfnAddFileCB = NULL; + ha->pvAddFileUserData = NULL; + + // Flush all unsaved data to the storage + SFileFlushArchive((HANDLE)(ha)); + assert(ha->dwFileCount == 0 && ha->dwRefCount == 0); + + // First of all, free the patch archive, if any + if(ha->haPatch != NULL) + DereferenceArchive(ha->haPatch); + ha->haPatch = NULL; + + // Free the patch prefix, if any + if(ha->pPatchPrefix != NULL) + STORM_FREE(ha->pPatchPrefix); + + // Close the file stream + FileStream_Close(ha->pStream); + ha->pStream = NULL; + + // Free the file names from the file table + if(ha->pFileTable != NULL) + { + for(DWORD i = 0; i < ha->dwFileTableSize; i++) + { + if(ha->pFileTable[i].szFileName != NULL) + STORM_FREE(ha->pFileTable[i].szFileName); + ha->pFileTable[i].szFileName = NULL; + } + + // Then free all buffers allocated in the archive structure + STORM_FREE(ha->pFileTable); + } + + if(ha->pHashTable != NULL) + STORM_FREE(ha->pHashTable); + if(ha->pHetTable != NULL) + FreeHetTable(ha->pHetTable); + STORM_FREE(ha); +} + +void FreeFileHandle(TMPQFile *& hf) +{ + TMPQArchive * ha; + + if(hf != NULL) + { + // If we have patch file attached to this one, free it first + if(hf->hfPatch != NULL) + FreeFileHandle(hf->hfPatch); + + // Then free all buffers allocated in the file structure + if(hf->pbFileData != NULL) + STORM_FREE(hf->pbFileData); + if(hf->pPatchInfo != NULL) + STORM_FREE(hf->pPatchInfo); + if(hf->SectorOffsets != NULL) + STORM_FREE(hf->SectorOffsets); + if(hf->SectorChksums != NULL) + STORM_FREE(hf->SectorChksums); + if(hf->hctx != NULL) + STORM_FREE(hf->hctx); + if(hf->pbFileSector != NULL) + STORM_FREE(hf->pbFileSector); + if(hf->pStream != NULL) + FileStream_Close(hf->pStream); + + // Dereference file count in the archive handle + if((ha = hf->ha) != NULL) + DereferenceArchiveFiles(ha); + + // Free the file handle + STORM_FREE(hf); + hf = NULL; + } +} + +bool DereferenceArchiveFiles(TMPQArchive * ha) +{ + // There must be at least one reference + if(ha == NULL || ha->dwFileCount == 0) + return false; + + // Decrement the file count + ha->dwFileCount--; + + // If we reached zero, free the archive + if(ha->dwRefCount == 0 && ha->dwFileCount == 0) + DeleteArchiveHandle(ha); + return true; +} + +bool DereferenceArchive(TMPQArchive * ha) +{ + // There must be at least one reference + if(ha == NULL || ha->dwRefCount == 0) + return false; + + // Decrement the file count + ha->dwRefCount--; + + // If we reached zero, free the archive + if(ha->dwRefCount == 0 && ha->dwFileCount == 0) + DeleteArchiveHandle(ha); + return true; +} + //----------------------------------------------------------------------------- // Swapping functions diff --git a/src/SFileCompactArchive.cpp b/src/SFileCompactArchive.cpp index 63a6de2..6040a3c 100644 --- a/src/SFileCompactArchive.cpp +++ b/src/SFileCompactArchive.cpp @@ -462,20 +462,24 @@ DWORD WINAPI SFileGetMaxFileCount(HANDLE hMpq) bool WINAPI SFileSetMaxFileCount(HANDLE hMpq, DWORD dwMaxFileCount) { - TMPQArchive * ha = (TMPQArchive *)hMpq; + TMPQArchive * ha; DWORD dwErrCode = ERROR_SUCCESS; // Calculate the hash table size for the new file limit DWORD dwNewHashTableSize = GetNearestPowerOfTwo(dwMaxFileCount); // Test the valid parameters - if(!IsValidMpqHandle(hMpq)) + if((ha = IsValidMpqHandle(hMpq)) == NULL) dwErrCode = ERROR_INVALID_HANDLE; if(ha->dwFlags & MPQ_FLAG_READ_ONLY) dwErrCode = ERROR_ACCESS_DENIED; if(dwNewHashTableSize < ha->dwFileTableSize) dwErrCode = ERROR_DISK_FULL; + // Are there some files open? + if(ha->dwFileCount) + dwErrCode = ERROR_ACCESS_DENIED; + // ALL file names must be known in order to be able to rebuild hash table if(dwErrCode == ERROR_SUCCESS && ha->pHashTable != NULL) { @@ -538,6 +542,10 @@ bool WINAPI SFileCompactArchive(HANDLE hMpq, const TCHAR * szListFile, bool /* b if(ha->dwFlags & MPQ_FLAG_READ_ONLY) dwErrCode = ERROR_ACCESS_DENIED; + // Are there some files open? + if(ha->dwFileCount) + dwErrCode = ERROR_ACCESS_DENIED; + // If the MPQ is changed at this moment, we have to flush the archive if(dwErrCode == ERROR_SUCCESS && (ha->dwFlags & MPQ_FLAG_CHANGED)) { diff --git a/src/SFileCreateArchive.cpp b/src/SFileCreateArchive.cpp index 9a311e0..13d5713 100644 --- a/src/SFileCreateArchive.cpp +++ b/src/SFileCreateArchive.cpp @@ -224,6 +224,7 @@ bool WINAPI SFileCreateArchive2(const TCHAR * szMpqName, PSFILE_CREATE_MPQ pCrea ha->dwFileFlags3 = pCreateInfo->dwFileFlags3 ? MPQ_FILE_EXISTS : 0; ha->dwAttrFlags = pCreateInfo->dwAttrFlags; ha->dwFlags = dwMpqFlags | MPQ_FLAG_CHANGED; + ha->dwRefCount = 1; pStream = NULL; // Fill the MPQ header @@ -274,7 +275,7 @@ bool WINAPI SFileCreateArchive2(const TCHAR * szMpqName, PSFILE_CREATE_MPQ pCrea if(dwErrCode != ERROR_SUCCESS) { FileStream_Close(pStream); - FreeArchiveHandle(ha); + DereferenceArchive(ha); SErrSetLastError(dwErrCode); ha = NULL; } diff --git a/src/SFileOpenArchive.cpp b/src/SFileOpenArchive.cpp index d07d325..efa5821 100644 --- a/src/SFileOpenArchive.cpp +++ b/src/SFileOpenArchive.cpp @@ -354,6 +354,9 @@ bool WINAPI SFileOpenArchive( // Also remember if this MPQ is a patch ha->dwFlags |= (dwFlags & MPQ_OPEN_PATCH) ? MPQ_FLAG_PATCH : 0; + // Set the reference count to 1 + ha->dwRefCount = 1; + // Limit the header searching to about 130 MB of data if(EndOfSearch > 0x08000000) EndOfSearch = 0x08000000; @@ -626,7 +629,7 @@ bool WINAPI SFileOpenArchive( if(dwErrCode != ERROR_SUCCESS) { FileStream_Close(pStream); - FreeArchiveHandle(ha); + DereferenceArchive(ha); SErrSetLastError(dwErrCode); ha = NULL; } @@ -695,6 +698,10 @@ bool WINAPI SFileFlushArchive(HANDLE hMpq) // Note that the (signature) file is usually before (listfile) in the file table // + // Up the number of references to prevent it from going away during archive cleanup + ha->dwRefCount++; + + // Add the (signature) file if(ha->dwFlags & MPQ_FLAG_SIGNATURE_NEW) { dwErrCode = SSignFileCreate(ha); @@ -702,6 +709,7 @@ bool WINAPI SFileFlushArchive(HANDLE hMpq) dwResultError = dwErrCode; } + // Add the (listfile) file if(ha->dwFlags & (MPQ_FLAG_LISTFILE_NEW | MPQ_FLAG_LISTFILE_FORCE)) { dwErrCode = SListFileSaveToMpq(ha); @@ -709,6 +717,7 @@ bool WINAPI SFileFlushArchive(HANDLE hMpq) dwResultError = dwErrCode; } + // Add the (attributes) file if(ha->dwFlags & MPQ_FLAG_ATTRIBUTES_NEW) { dwErrCode = SAttrFileSaveToMpq(ha); @@ -737,6 +746,10 @@ bool WINAPI SFileFlushArchive(HANDLE hMpq) } } + // Drop the reference count. Do NOT call DereferenceHandle() to prevent recursion + assert(ha->dwRefCount > 0); + ha->dwRefCount--; + // We are no longer saving internal MPQ structures ha->dwFlags &= ~MPQ_FLAG_SAVING_TABLES; } @@ -754,7 +767,6 @@ bool WINAPI SFileFlushArchive(HANDLE hMpq) bool WINAPI SFileCloseArchive(HANDLE hMpq) { TMPQArchive * ha = IsValidMpqHandle(hMpq); - bool bResult = false; // Only if the handle is valid if(ha == NULL) @@ -763,15 +775,12 @@ bool WINAPI SFileCloseArchive(HANDLE hMpq) return false; } - // Invalidate the add file callback so it won't be called - // when saving (listfile) and (attributes) - ha->pfnAddFileCB = NULL; - ha->pvAddFileUserData = NULL; - - // Flush all unsaved data to the storage - bResult = SFileFlushArchive(hMpq); + // Dereference the archive + if(!DereferenceArchive(ha)) + { + SErrSetLastError(ERROR_ACCESS_DENIED); + return false; + } - // Free all memory used by MPQ archive - FreeArchiveHandle(ha); - return bResult; + return true; } diff --git a/src/StormCommon.h b/src/StormCommon.h index cefb92c..037b17d 100644 --- a/src/StormCommon.h +++ b/src/StormCommon.h @@ -410,8 +410,10 @@ DWORD WriteSectorOffsets(TMPQFile * hf); DWORD WriteSectorChecksums(TMPQFile * hf); DWORD WriteMemDataMD5(TFileStream * pStream, ULONGLONG RawDataOffs, void * pvRawData, DWORD dwRawDataSize, DWORD dwChunkSize, LPDWORD pcbTotalSize); DWORD WriteMpqDataMD5(TFileStream * pStream, ULONGLONG RawDataOffs, DWORD dwRawDataSize, DWORD dwChunkSize); + +bool DereferenceArchiveFiles(TMPQArchive * ha); +bool DereferenceArchive(TMPQArchive * ha); void FreeFileHandle(TMPQFile *& hf); -void FreeArchiveHandle(TMPQArchive *& ha); //----------------------------------------------------------------------------- // Patch functions diff --git a/src/StormLib.h b/src/StormLib.h index 005cc46..55fcaeb 100644 --- a/src/StormLib.h +++ b/src/StormLib.h @@ -864,6 +864,9 @@ typedef struct _TMPQArchive DWORD dwFlags; // See MPQ_FLAG_XXXXX DWORD dwSubType; // See MPQ_SUBTYPE_XXX + DWORD dwFileCount; // Number of open files + DWORD dwRefCount; // Number of references + SFILE_ADDFILE_CALLBACK pfnAddFileCB; // Callback function for adding files void * pvAddFileUserData; // User data thats passed to the callback diff --git a/test/StormTest.cpp b/test/StormTest.cpp index bd07c88..1eed7cd 100755 --- a/test/StormTest.cpp +++ b/test/StormTest.cpp @@ -3955,27 +3955,30 @@ static DWORD TestUtf8Conversions(const BYTE * szTestString, const TCHAR * szList static void Test_PlayingSpace() { -/* + LPCTSTR szMpqName = _T("e:\\Test-UAF.mpq"); HANDLE hMpq; HANDLE hFile; - LPBYTE pbData; - DWORD dwFileSize = 529298; - DWORD dwBytesRead = 0; + char nameBuf[260]; - if(SFileOpenArchive(_T("e:\\Ladik\\Incoming\\31525686D3A39C6B5CA4B2979367B809.w3x"), 0, 0, &hMpq)) + DeleteFile(szMpqName); + if(SFileCreateArchive(szMpqName, 0, 16, &hMpq)) { - if(SFileOpenFileEx(hMpq, "(listfile)", 0, &hFile)) + SFileCreateFile(hMpq, "foo", 0ULL, 8, 0, 0, &hFile); + SFileCloseArchive(hMpq); + SFileCloseFile(hFile); + } + + DeleteFile(szMpqName); + if(SFileCreateArchive(szMpqName, 0, 16, &hMpq)) + { + if(SFileCreateFile(hMpq, "foo", 0ULL, 8, 0, 0, &hFile)) { - if((pbData = STORM_ALLOC(BYTE, dwFileSize)) != NULL) - { - SFileReadFile(hFile, pbData, dwFileSize, &dwBytesRead, NULL); - STORM_FREE(pbData); - } + SFileSetMaxFileCount(hMpq, 64); + SFileGetFileName(hFile, nameBuf); SFileCloseFile(hFile); } SFileCloseArchive(hMpq); } -*/ } //----------------------------------------------------------------------------- From f32cb611a427690db959c16d45bdfb5aa839c1b9 Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Fri, 9 Jan 2026 21:46:59 +0100 Subject: [PATCH 2/3] Fixed use-after-free scenarios --- test/StormTest.cpp | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/test/StormTest.cpp b/test/StormTest.cpp index 1eed7cd..774da3f 100755 --- a/test/StormTest.cpp +++ b/test/StormTest.cpp @@ -3953,32 +3953,40 @@ static DWORD TestUtf8Conversions(const BYTE * szTestString, const TCHAR * szList return ERROR_SUCCESS; } -static void Test_PlayingSpace() +static DWORD TestUseAfterFree(LPCTSTR szPlainName) { - LPCTSTR szMpqName = _T("e:\\Test-UAF.mpq"); + TLogHelper Logger("TestUseAfterFree", szPlainName); HANDLE hMpq; HANDLE hFile; - char nameBuf[260]; + DWORD dwErrCode; - DeleteFile(szMpqName); - if(SFileCreateArchive(szMpqName, 0, 16, &hMpq)) + // Create new archive + dwErrCode = CreateNewArchive(&Logger, szPlainName, MPQ_CREATE_ARCHIVE_V1 | MPQ_CREATE_LISTFILE | MPQ_CREATE_ATTRIBUTES, 32, &hMpq); + if(dwErrCode == ERROR_SUCCESS) { SFileCreateFile(hMpq, "foo", 0ULL, 8, 0, 0, &hFile); SFileCloseArchive(hMpq); SFileCloseFile(hFile); } - DeleteFile(szMpqName); - if(SFileCreateArchive(szMpqName, 0, 16, &hMpq)) + dwErrCode = CreateNewArchive(&Logger, szPlainName, MPQ_CREATE_ARCHIVE_V1 | MPQ_CREATE_LISTFILE | MPQ_CREATE_ATTRIBUTES, 32, &hMpq); + if(dwErrCode == ERROR_SUCCESS) { if(SFileCreateFile(hMpq, "foo", 0ULL, 8, 0, 0, &hFile)) { + char nameBuf[260]; + SFileSetMaxFileCount(hMpq, 64); SFileGetFileName(hFile, nameBuf); SFileCloseFile(hFile); } SFileCloseArchive(hMpq); } + return dwErrCode; +} + +static void Test_PlayingSpace() +{ } //----------------------------------------------------------------------------- @@ -4388,7 +4396,7 @@ static const TEST_INFO1 Test_ReopenMpqs[] = {_T("MPQ_2016_v1_00000.pak"), NULL, "76c5c4dffee8a9e3568e22216b5f0b94", 2072 | TFLG_COMPACT | TFLG_HAS_LISTFILE}, {_T("MPQ_2013_v4_SC2_EmptyMap.SC2Map"), NULL, "88e1b9a88d56688c9c24037782b7bb68", 33 | TFLG_COMPACT | TFLG_ADD_USER_DATA | TFLG_HAS_LISTFILE | TFLG_HAS_ATTRIBUTES}, {_T("MPQ_2013_v4_expansion1.MPQ"), NULL, "c97d2b4e2561d3eb3a728d72a74d86c2", 15633 | TFLG_COMPACT | TFLG_ADD_USER_DATA | TFLG_HAS_LISTFILE | TFLG_HAS_ATTRIBUTES}, - + // Adding a file to MPQ that had size of the file table equal // or greater than hash table, but has free entries {_T("MPQ_2014_v1_out1.w3x"), NULL, "222e685bd76e1af6d267ea1e0c27371f", 39 | TFLG_MODIFY | TFLG_HAS_LISTFILE}, @@ -4480,6 +4488,9 @@ int _tmain(int argc, TCHAR * argv[]) // Test the UTF-8 conversions TestUtf8Conversions(FileNameInvalidUTF8, LfBad1.szFile); + // Test the use-after-free scenarios + TestUseAfterFree(_T("Test-UAF.mpq")); + #ifdef TEST_COMMAND_LINE // Test-open MPQs from the command line. They must be plain name // and must be placed in the Test-MPQs folder From fa6c3c43ff94cfbf123f778a077712914f0ed2bc Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Fri, 9 Jan 2026 21:48:35 +0100 Subject: [PATCH 3/3] Typo --- src/SBaseCommon.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SBaseCommon.cpp b/src/SBaseCommon.cpp index 6f48ca8..0f9f2c9 100644 --- a/src/SBaseCommon.cpp +++ b/src/SBaseCommon.cpp @@ -931,7 +931,7 @@ TMPQFile * CreateFileHandle(TMPQArchive * ha, TFileEntry * pFileEntry) hf->pStream = NULL; hf->ha = ha; - // If the called entered a file entry, we also copy informations from the file entry + // If the caller entered a file entry, we also copy informations from the file entry if(ha != NULL) { // Increment number of open files in the archive handle