From 247667233dac4ce9d164f1a876f1ad32390f8e12 Mon Sep 17 00:00:00 2001 From: Marc-Antoine Lortie Date: Sat, 18 Jan 2020 14:58:19 -0500 Subject: [PATCH 1/2] Fixed memory leak in MDLLoader.cpp If one of the MDL importer implementations throw an exception, the memory allocated at mBuffer may never be freed. This fix should prevent further memory leaks. --- code/MDL/MDLLoader.cpp | 167 ++++++++++++++++++++++------------------- 1 file changed, 89 insertions(+), 78 deletions(-) diff --git a/code/MDL/MDLLoader.cpp b/code/MDL/MDLLoader.cpp index eb629d35a..78dc9d77f 100644 --- a/code/MDL/MDLLoader.cpp +++ b/code/MDL/MDLLoader.cpp @@ -184,88 +184,99 @@ void MDLImporter::InternReadFile( const std::string& pFile, if( iFileSize < sizeof(MDL::HalfLife::SequenceHeader_HL1)) { throw DeadlyImportError( "MDL File is too small."); } - - // Allocate storage and copy the contents of the file to a memory buffer - mBuffer =new unsigned char[iFileSize+1]; - file->Read( (void*)mBuffer, 1, iFileSize); - - // Append a binary zero to the end of the buffer. - // this is just for safety that string parsing routines - // find the end of the buffer ... - mBuffer[iFileSize] = '\0'; - const uint32_t iMagicWord = *((uint32_t*)mBuffer); - - // Determine the file subtype and call the appropriate member function - - // Original Quake1 format - if (AI_MDL_MAGIC_NUMBER_BE == iMagicWord || AI_MDL_MAGIC_NUMBER_LE == iMagicWord) { - ASSIMP_LOG_DEBUG("MDL subtype: Quake 1, magic word is IDPO"); - iGSFileVersion = 0; - InternReadFile_Quake1(); - } - // GameStudio A MDL2 format - used by some test models that come with 3DGS - else if (AI_MDL_MAGIC_NUMBER_BE_GS3 == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_GS3 == iMagicWord) { - ASSIMP_LOG_DEBUG("MDL subtype: 3D GameStudio A2, magic word is MDL2"); - iGSFileVersion = 2; - InternReadFile_Quake1(); - } - // GameStudio A4 MDL3 format - else if (AI_MDL_MAGIC_NUMBER_BE_GS4 == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_GS4 == iMagicWord) { - ASSIMP_LOG_DEBUG("MDL subtype: 3D GameStudio A4, magic word is MDL3"); - iGSFileVersion = 3; - InternReadFile_3DGS_MDL345(); - } - // GameStudio A5+ MDL4 format - else if (AI_MDL_MAGIC_NUMBER_BE_GS5a == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_GS5a == iMagicWord) { - ASSIMP_LOG_DEBUG("MDL subtype: 3D GameStudio A4, magic word is MDL4"); - iGSFileVersion = 4; - InternReadFile_3DGS_MDL345(); - } - // GameStudio A5+ MDL5 format - else if (AI_MDL_MAGIC_NUMBER_BE_GS5b == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_GS5b == iMagicWord) { - ASSIMP_LOG_DEBUG("MDL subtype: 3D GameStudio A5, magic word is MDL5"); - iGSFileVersion = 5; - InternReadFile_3DGS_MDL345(); - } - // GameStudio A7 MDL7 format - else if (AI_MDL_MAGIC_NUMBER_BE_GS7 == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_GS7 == iMagicWord) { - ASSIMP_LOG_DEBUG("MDL subtype: 3D GameStudio A7, magic word is MDL7"); - iGSFileVersion = 7; - InternReadFile_3DGS_MDL7(); - } - // IDST/IDSQ Format (CS:S/HL^2, etc ...) - else if (AI_MDL_MAGIC_NUMBER_BE_HL2a == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_HL2a == iMagicWord || - AI_MDL_MAGIC_NUMBER_BE_HL2b == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_HL2b == iMagicWord) - { - iGSFileVersion = 0; - - HalfLife::HalfLifeMDLBaseHeader *pHeader = (HalfLife::HalfLifeMDLBaseHeader *)mBuffer; - if (pHeader->version == AI_MDL_HL1_VERSION) - { - ASSIMP_LOG_DEBUG("MDL subtype: Half-Life 1/Goldsrc Engine, magic word is IDST/IDSQ"); - InternReadFile_HL1(pFile, iMagicWord); + + // delete the file buffer and cleanup. + auto DeleteBufferAndCleanup = [&]() { + if (mBuffer) { + delete mBuffer; + mBuffer = nullptr; } - else - { - ASSIMP_LOG_DEBUG("MDL subtype: Source(tm) Engine, magic word is IDST/IDSQ"); - InternReadFile_HL2(); + AI_DEBUG_INVALIDATE_PTR(pIOHandler); + AI_DEBUG_INVALIDATE_PTR(pScene); + }; + + try { + // Allocate storage and copy the contents of the file to a memory buffer + mBuffer = new unsigned char[iFileSize+1]; + file->Read( (void*)mBuffer, 1, iFileSize); + + // Append a binary zero to the end of the buffer. + // this is just for safety that string parsing routines + // find the end of the buffer ... + mBuffer[iFileSize] = '\0'; + const uint32_t iMagicWord = *((uint32_t*)mBuffer); + + // Determine the file subtype and call the appropriate member function + + // Original Quake1 format + if (AI_MDL_MAGIC_NUMBER_BE == iMagicWord || AI_MDL_MAGIC_NUMBER_LE == iMagicWord) { + ASSIMP_LOG_DEBUG("MDL subtype: Quake 1, magic word is IDPO"); + iGSFileVersion = 0; + InternReadFile_Quake1(); } - } - else { - // print the magic word to the log file - throw DeadlyImportError( "Unknown MDL subformat " + pFile + - ". Magic word (" + std::string((char*)&iMagicWord,4) + ") is not known"); - } + // GameStudio A MDL2 format - used by some test models that come with 3DGS + else if (AI_MDL_MAGIC_NUMBER_BE_GS3 == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_GS3 == iMagicWord) { + ASSIMP_LOG_DEBUG("MDL subtype: 3D GameStudio A2, magic word is MDL2"); + iGSFileVersion = 2; + InternReadFile_Quake1(); + } + // GameStudio A4 MDL3 format + else if (AI_MDL_MAGIC_NUMBER_BE_GS4 == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_GS4 == iMagicWord) { + ASSIMP_LOG_DEBUG("MDL subtype: 3D GameStudio A4, magic word is MDL3"); + iGSFileVersion = 3; + InternReadFile_3DGS_MDL345(); + } + // GameStudio A5+ MDL4 format + else if (AI_MDL_MAGIC_NUMBER_BE_GS5a == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_GS5a == iMagicWord) { + ASSIMP_LOG_DEBUG("MDL subtype: 3D GameStudio A4, magic word is MDL4"); + iGSFileVersion = 4; + InternReadFile_3DGS_MDL345(); + } + // GameStudio A5+ MDL5 format + else if (AI_MDL_MAGIC_NUMBER_BE_GS5b == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_GS5b == iMagicWord) { + ASSIMP_LOG_DEBUG("MDL subtype: 3D GameStudio A5, magic word is MDL5"); + iGSFileVersion = 5; + InternReadFile_3DGS_MDL345(); + } + // GameStudio A7 MDL7 format + else if (AI_MDL_MAGIC_NUMBER_BE_GS7 == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_GS7 == iMagicWord) { + ASSIMP_LOG_DEBUG("MDL subtype: 3D GameStudio A7, magic word is MDL7"); + iGSFileVersion = 7; + InternReadFile_3DGS_MDL7(); + } + // IDST/IDSQ Format (CS:S/HL^2, etc ...) + else if (AI_MDL_MAGIC_NUMBER_BE_HL2a == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_HL2a == iMagicWord || + AI_MDL_MAGIC_NUMBER_BE_HL2b == iMagicWord || AI_MDL_MAGIC_NUMBER_LE_HL2b == iMagicWord) + { + iGSFileVersion = 0; - // Now rotate the whole scene 90 degrees around the x axis to convert to internal coordinate system - pScene->mRootNode->mTransformation = aiMatrix4x4(1.f,0.f,0.f,0.f, - 0.f,0.f,1.f,0.f,0.f,-1.f,0.f,0.f,0.f,0.f,0.f,1.f); + HalfLife::HalfLifeMDLBaseHeader *pHeader = (HalfLife::HalfLifeMDLBaseHeader *)mBuffer; + if (pHeader->version == AI_MDL_HL1_VERSION) + { + ASSIMP_LOG_DEBUG("MDL subtype: Half-Life 1/Goldsrc Engine, magic word is IDST/IDSQ"); + InternReadFile_HL1(pFile, iMagicWord); + } + else + { + ASSIMP_LOG_DEBUG("MDL subtype: Source(tm) Engine, magic word is IDST/IDSQ"); + InternReadFile_HL2(); + } + } + else { + // print the magic word to the log file + throw DeadlyImportError( "Unknown MDL subformat " + pFile + + ". Magic word (" + std::string((char*)&iMagicWord,4) + ") is not known"); + } - // delete the file buffer and cleanup - delete [] mBuffer; - mBuffer= nullptr; - AI_DEBUG_INVALIDATE_PTR(pIOHandler); - AI_DEBUG_INVALIDATE_PTR(pScene); + // Now rotate the whole scene 90 degrees around the x axis to convert to internal coordinate system + pScene->mRootNode->mTransformation = aiMatrix4x4(1.f,0.f,0.f,0.f, + 0.f,0.f,1.f,0.f,0.f,-1.f,0.f,0.f,0.f,0.f,0.f,1.f); + + DeleteBufferAndCleanup(); + } catch(...) { + DeleteBufferAndCleanup(); + throw; + } } // ------------------------------------------------------------------------------------------------ From b74562f8a08ab889e10b5f37cfac3b957cce52f8 Mon Sep 17 00:00:00 2001 From: Marc-Antoine Lortie Date: Sat, 18 Jan 2020 15:16:03 -0500 Subject: [PATCH 2/2] Fixed delete operator. --- code/MDL/MDLLoader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/MDL/MDLLoader.cpp b/code/MDL/MDLLoader.cpp index 78dc9d77f..0c80abfc3 100644 --- a/code/MDL/MDLLoader.cpp +++ b/code/MDL/MDLLoader.cpp @@ -188,7 +188,7 @@ void MDLImporter::InternReadFile( const std::string& pFile, // delete the file buffer and cleanup. auto DeleteBufferAndCleanup = [&]() { if (mBuffer) { - delete mBuffer; + delete [] mBuffer; mBuffer = nullptr; } AI_DEBUG_INVALIDATE_PTR(pIOHandler);