From 3224eac75e07dd198d60fb754d408fa1861ec3f0 Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Tue, 3 May 2016 16:33:49 +0200 Subject: + Fixed bounds checking in SFileSetFilePointer --- src/SFileReadFile.cpp | 98 ++++++++++++++++++++++--------------------- test/StormTest.cpp | 113 ++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 161 insertions(+), 50 deletions(-) diff --git a/src/SFileReadFile.cpp b/src/SFileReadFile.cpp index 30dc587..485b361 100644 --- a/src/SFileReadFile.cpp +++ b/src/SFileReadFile.cpp @@ -801,9 +801,10 @@ DWORD WINAPI SFileGetFileSize(HANDLE hFile, LPDWORD pdwFileSizeHigh) DWORD WINAPI SFileSetFilePointer(HANDLE hFile, LONG lFilePos, LONG * plFilePosHigh, DWORD dwMoveMethod) { TMPQFile * hf = (TMPQFile *)hFile; - ULONGLONG FilePosition; - ULONGLONG MoveOffset; - DWORD dwFilePosHi; + ULONGLONG OldPosition; + ULONGLONG NewPosition; + ULONGLONG FileSize; + ULONGLONG DeltaPos; // If the hFile is not a valid file handle, return an error. if(!IsValidFileHandle(hFile)) @@ -812,33 +813,47 @@ DWORD WINAPI SFileSetFilePointer(HANDLE hFile, LONG lFilePos, LONG * plFilePosHi return SFILE_INVALID_POS; } + // Retrieve the file size for handling the limits + if(hf->pStream != NULL) + { + FileStream_GetSize(hf->pStream, &FileSize); + } + else + { + FileSize = SFileGetFileSize(hFile, NULL); + } + + // Handle the NULL and non-NULL values of plFilePosHigh + // Non-NULL: The DeltaPos is combined from lFilePos and *lpFilePosHigh + // NULL: The DeltaPos is sign-extended value of lFilePos + DeltaPos = (plFilePosHigh != NULL) ? MAKE_OFFSET64(plFilePosHigh[0], lFilePos) : (ULONGLONG)(LONGLONG)lFilePos; + // Get the relative point where to move from switch(dwMoveMethod) { case FILE_BEGIN: - FilePosition = 0; + + // Move relative to the file begin. + OldPosition = 0; break; case FILE_CURRENT: + + // Retrieve the current file position if(hf->pStream != NULL) { - FileStream_GetPos(hf->pStream, &FilePosition); + FileStream_GetPos(hf->pStream, &OldPosition); } else { - FilePosition = hf->dwFilePos; + OldPosition = hf->dwFilePos; } break; case FILE_END: - if(hf->pStream != NULL) - { - FileStream_GetSize(hf->pStream, &FilePosition); - } - else - { - FilePosition = SFileGetFileSize(hFile, NULL); - } + + // Move relative to the end of the file + OldPosition = FileSize; break; default: @@ -846,47 +861,36 @@ DWORD WINAPI SFileSetFilePointer(HANDLE hFile, LONG lFilePos, LONG * plFilePosHi return SFILE_INVALID_POS; } - // Now get the move offset. Note that both values form - // a signed 64-bit value (a file pointer can be moved backwards) - if(plFilePosHigh != NULL) - dwFilePosHi = *plFilePosHigh; - else - dwFilePosHi = (lFilePos & 0x80000000) ? 0xFFFFFFFF : 0; - MoveOffset = MAKE_OFFSET64(dwFilePosHi, lFilePos); + // Calculate the new position + NewPosition = OldPosition + DeltaPos; - // Now calculate the new file pointer - // Do not allow the file pointer to overflow - FilePosition = ((FilePosition + MoveOffset) >= FilePosition) ? (FilePosition + MoveOffset) : 0; + // If moving backward, don't allow the new position go negative + if((LONGLONG)DeltaPos < 0) + { + if(NewPosition > FileSize) + NewPosition = 0; + } + + // If moving forward, don't allow the new position go past the end of the file + else + { + if(NewPosition > FileSize) + NewPosition = FileSize; + } // Now apply the file pointer to the file if(hf->pStream != NULL) { - // Apply the new file position - if(!FileStream_Read(hf->pStream, &FilePosition, NULL, 0)) + if(!FileStream_Read(hf->pStream, &NewPosition, NULL, 0)) return SFILE_INVALID_POS; - - // Return the new file position - if(plFilePosHigh != NULL) - *plFilePosHigh = (LONG)(FilePosition >> 32); - return (DWORD)FilePosition; } else { - // Files in MPQ can't be bigger than 4 GB. - // We don't allow to go past 4 GB - if(FilePosition >> 32) - { - SetLastError(ERROR_INVALID_PARAMETER); - return SFILE_INVALID_POS; - } - - // Change the file position - hf->dwFilePos = (DWORD)FilePosition; - - // Return the new file position - if(plFilePosHigh != NULL) - *plFilePosHigh = 0; - return (DWORD)FilePosition; + hf->dwFilePos = (DWORD)NewPosition; } -} + // Return the new file position + if(plFilePosHigh != NULL) + *plFilePosHigh = (LONG)(NewPosition >> 32); + return (DWORD)NewPosition; +} diff --git a/test/StormTest.cpp b/test/StormTest.cpp index 3b92fd1..d513b7b 100644 --- a/test/StormTest.cpp +++ b/test/StormTest.cpp @@ -2079,9 +2079,71 @@ static int RemoveMpqFile(TLogHelper * pLogger, HANDLE hMpq, const char * szFileN return ERROR_SUCCESS; } +static ULONGLONG SFileGetFilePointer(HANDLE hFile) +{ + LONG FilePosHi = 0; + DWORD FilePosLo; + + FilePosLo = SFileSetFilePointer(hFile, 0, &FilePosHi, FILE_CURRENT); + return MAKE_OFFSET64(FilePosHi, FilePosLo); +} + //----------------------------------------------------------------------------- // Tests +static int TestSetFilePointer( + HANDLE hFile, + LONGLONG DeltaPos, + ULONGLONG ExpectedPos, + DWORD dwMoveMethod, + bool bUseFilePosHigh, + int nError) +{ + ULONGLONG NewPos = 0; + LONG DeltaPosHi = (LONG)(DeltaPos >> 32); + LONG DeltaPosLo = (LONG)(DeltaPos); + + // If there was an error before, do nothing + if(nError == ERROR_SUCCESS) + { + SFileSetFilePointer(hFile, DeltaPosLo, bUseFilePosHigh ? &DeltaPosHi : NULL, dwMoveMethod); + NewPos = SFileGetFilePointer(hFile); + if(NewPos != ExpectedPos) + nError = ERROR_HANDLE_EOF; + } + + return nError; +} + +static int TestSetFilePointers(HANDLE hFile, bool bUseFilePosHigh) +{ + LONGLONG FileSize; + int nError = ERROR_SUCCESS; + + // We expect the file to be at least 2 pages long + FileSize = SFileGetFileSize(hFile, NULL); + if(FileSize < 0x2000) + return ERROR_NOT_SUPPORTED; + + // Move 0x20 bytes from the beginning. Expected new pos is 0x20 + nError = TestSetFilePointer(hFile, 0x20, 0x20, FILE_BEGIN, bUseFilePosHigh, nError); + + // Move 0x20 bytes from the current position. Expected new pos is 0x20 + nError = TestSetFilePointer(hFile, 0x20, 0x40, FILE_CURRENT, bUseFilePosHigh, nError); + + // Move 0x40 bytes back. Because the offset can't be moved to negative position, it will be zero + nError = TestSetFilePointer(hFile, -64, 0x00, FILE_CURRENT, bUseFilePosHigh, nError); + + // Move 0x40 bytes before the end of the file + nError = TestSetFilePointer(hFile, -64, FileSize-64, FILE_END, bUseFilePosHigh, nError); + + // Move 0x80 bytes forward. Should be at end of file + nError = TestSetFilePointer(hFile, 0x80, FileSize, FILE_CURRENT, bUseFilePosHigh, nError); + + return nError; +} + + static void TestGetFileInfo( TLogHelper * pLogger, HANDLE hMpqOrFile, @@ -2530,6 +2592,47 @@ static int TestOpenArchive(const char * szPlainName, const char * szListFile = N return nError; } +static int TestOpenArchive_SetPos(const char * szPlainName, const char * szFileName) +{ + TLogHelper Logger("SetPosTest", szPlainName); + HANDLE hFile = NULL; + HANDLE hMpq = NULL; + TCHAR szMpqName[MAX_PATH]; + char szFullPath[MAX_PATH]; + int nError = ERROR_SUCCESS; + + // Create the full path name for the archive + CreateFullPathName(szFullPath, szMpqSubDir, szPlainName); + CopyFileName(szMpqName, szFullPath, strlen(szFullPath)); + + // Try to open the archive. It is expected to fail + Logger.PrintProgress("Opening archive %s", szPlainName); + if(SFileOpenArchive(szMpqName, 0, MPQ_OPEN_READ_ONLY, &hMpq)) + { + if(SFileOpenFileEx(hMpq, szFileName, 0, &hFile)) + { + // First, use the SFileSetFilePointer WITHOUT the high-dword position + if(nError == ERROR_SUCCESS) + nError = TestSetFilePointers(hFile, false); + + // First, use the SFileSetFilePointer WITH the high-dword position + if(nError == ERROR_SUCCESS) + nError = TestSetFilePointers(hFile, false); + + // Close the file + SFileCloseFile(hFile); + } + else + nError = GetLastError(); + + // Close the archive + SFileCloseArchive(hMpq); + } + else + nError = GetLastError(); + + return nError; +} // Open an empty archive (found in WoW cache - it's just a header) static int TestOpenArchive_WillFail(const char * szPlainName) @@ -4248,7 +4351,11 @@ int main(int argc, char * argv[]) // Open a file whose archive's (signature) file has flags = 0x90000000 if(nError == ERROR_SUCCESS) nError = TestOpenArchive("MPQ_1997_v1_Diablo1_STANDARD.SNP", "ListFile_Blizzard.txt"); - +*/ + // Test the SFileSetFilePointer operations + if(nError == ERROR_SUCCESS) + nError = TestOpenArchive_SetPos("MPQ_1997_v1_Diablo1_DIABDAT.MPQ", "music\\dtowne.wav"); +/* // Open an empty archive (found in WoW cache - it's just a header) if(nError == ERROR_SUCCESS) nError = TestOpenArchive("MPQ_2012_v2_EmptyMpq.MPQ"); @@ -4343,11 +4450,11 @@ int main(int argc, char * argv[]) // Open another protected map if(nError == ERROR_SUCCESS) nError = TestOpenArchive("MPQ_2016_v1_ProtectedMap_TableSizeOverflow.w3x"); -*/ + // Open another protected map if(nError == ERROR_SUCCESS) nError = TestOpenArchive("MPQ_2016_v1_ProtectedMap_HashOffsIsZero.w3x"); -/* + // Open the multi-file archive with wrong prefix to see how StormLib deals with it if(nError == ERROR_SUCCESS) nError = TestOpenArchive_WillFail("flat-file://streaming/model.MPQ.0"); -- cgit v1.2.3