From b170370e5cd02b2f2e5abdf873537c97455a2ea7 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Wed, 8 Feb 2023 21:29:54 +0100 Subject: [PATCH 1/2] Fix: Fix memleak when exiting method by exception --- code/Common/Importer.cpp | 58 ++++++++++++++++++-------------- fuzz/assimp_fuzzer.cc | 7 ++-- include/assimp/MemoryIOWrapper.h | 12 +++---- 3 files changed, 40 insertions(+), 37 deletions(-) diff --git a/code/Common/Importer.cpp b/code/Common/Importer.cpp index df2895eefc..b66059397e 100644 --- a/code/Common/Importer.cpp +++ b/code/Common/Importer.cpp @@ -482,37 +482,43 @@ bool Importer::ValidateFlags(unsigned int pFlags) const { } // ------------------------------------------------------------------------------------------------ -const aiScene* Importer::ReadFileFromMemory( const void* pBuffer, - size_t pLength, - unsigned int pFlags, - const char* pHint /*= ""*/) { +const aiScene* Importer::ReadFileFromMemory(const void* pBuffer, size_t pLength, unsigned int pFlags, const char* pHint ) { ai_assert(nullptr != pimpl); - ASSIMP_BEGIN_EXCEPTION_REGION(); - if (!pHint) { - pHint = ""; - } - - if (!pBuffer || !pLength || strlen(pHint) > MaxLenHint ) { - pimpl->mErrorString = "Invalid parameters passed to ReadFileFromMemory()"; - return nullptr; - } - - // prevent deletion of the previous IOHandler IOSystem* io = pimpl->mIOHandler; - pimpl->mIOHandler = nullptr; - - SetIOHandler(new MemoryIOSystem((const uint8_t*)pBuffer,pLength,io)); - - // read the file and recover the previous IOSystem - static const size_t BufSize(Importer::MaxLenHint + 28); - char fbuff[BufSize]; - ai_snprintf(fbuff, BufSize, "%s.%s",AI_MEMORYIO_MAGIC_FILENAME,pHint); + try { + if (pHint == nullptr) { + pHint = ""; + } + if (!pBuffer || !pLength || strlen(pHint) > MaxLenHint ) { + pimpl->mErrorString = "Invalid parameters passed to ReadFileFromMemory()"; + return nullptr; + } + // prevent deletion of the previous IOHandler + pimpl->mIOHandler = nullptr; + + SetIOHandler(new MemoryIOSystem((const uint8_t*)pBuffer,pLength,io)); + + // read the file and recover the previous IOSystem + static const size_t BufSize(Importer::MaxLenHint + 28); + char fbuff[BufSize]; + ai_snprintf(fbuff, BufSize, "%s.%s",AI_MEMORYIO_MAGIC_FILENAME,pHint); + + ReadFile(fbuff,pFlags); + SetIOHandler(io); + } catch(const DeadlyImportError &e) { + pimpl->mErrorString = e.what(); + pimpl->mException = std::current_exception(); + SetIOHandler(io); + return ExceptionSwallower()(); \ + } catch(...) { + pimpl->mErrorString = "Unknown exception"; + pimpl->mException = std::current_exception(); + SetIOHandler(io); + return ExceptionSwallower()(); \ - ReadFile(fbuff,pFlags); - SetIOHandler(io); + } - ASSIMP_END_EXCEPTION_REGION_WITH_ERROR_STRING(const aiScene*, pimpl->mErrorString, pimpl->mException); return pimpl->mScene; } diff --git a/fuzz/assimp_fuzzer.cc b/fuzz/assimp_fuzzer.cc index 33748c10f1..edb5fdbb5d 100644 --- a/fuzz/assimp_fuzzer.cc +++ b/fuzz/assimp_fuzzer.cc @@ -3,7 +3,7 @@ Open Asset Import Library (assimp) --------------------------------------------------------------------------- -Copyright (c) 2006-2020, assimp team +Copyright (c) 2006-2023, assimp team All rights reserved. @@ -46,8 +46,8 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. using namespace Assimp; extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t dataSize) { - aiLogStream stream = aiGetPredefinedLogStream(aiDefaultLogStream_STDOUT,NULL); - aiAttachLogStream(&stream); + aiLogStream stream = aiGetPredefinedLogStream(aiDefaultLogStream_STDOUT,NULL); + aiAttachLogStream(&stream); Importer importer; const aiScene *sc = importer.ReadFileFromMemory(data, dataSize, @@ -57,3 +57,4 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t dataSize) { return 0; } + diff --git a/include/assimp/MemoryIOWrapper.h b/include/assimp/MemoryIOWrapper.h index 0bd3bc1081..d52c4384f7 100644 --- a/include/assimp/MemoryIOWrapper.h +++ b/include/assimp/MemoryIOWrapper.h @@ -4,7 +4,6 @@ Open Asset Import Library (assimp) Copyright (c) 2006-2022, assimp team - All rights reserved. Redistribution and use of this software in source and binary forms, @@ -153,11 +152,7 @@ class MemoryIOStream : public IOStream { class MemoryIOSystem : public IOSystem { public: /** Constructor. */ - MemoryIOSystem(const uint8_t* buff, size_t len, IOSystem* io) - : buffer(buff) - , length(len) - , existing_io(io) - , created_streams() { + MemoryIOSystem(const uint8_t* buff, size_t len, IOSystem* io) : buffer(buff), length(len), existing_io(io) { // empty } @@ -167,6 +162,7 @@ class MemoryIOSystem : public IOSystem { // ------------------------------------------------------------------- /** Tests for the existence of a file at the given path. */ bool Exists(const char* pFile) const override { + printf("Exists\n"); if (0 == strncmp( pFile, AI_MEMORYIO_MAGIC_FILENAME, AI_MEMORYIO_MAGIC_FILENAME_LENGTH ) ) { return true; } @@ -187,7 +183,7 @@ class MemoryIOSystem : public IOSystem { created_streams.emplace_back(new MemoryIOStream(buffer, length)); return created_streams.back(); } - return existing_io ? existing_io->Open(pFile, pMode) : NULL; + return existing_io ? existing_io->Open(pFile, pMode) : nullptr; } // ------------------------------------------------------------------- @@ -246,4 +242,4 @@ class MemoryIOSystem : public IOSystem { } // end namespace Assimp -#endif +#endif // AI_MEMORYIOSTREAM_H_INC From b7d08fc8f2f9250de3ecefef49a26367e8ad141a Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Wed, 8 Feb 2023 21:48:55 +0100 Subject: [PATCH 2/2] Fix review findings. --- include/assimp/MemoryIOWrapper.h | 61 +++++++++++++++----------------- 1 file changed, 28 insertions(+), 33 deletions(-) diff --git a/include/assimp/MemoryIOWrapper.h b/include/assimp/MemoryIOWrapper.h index d52c4384f7..720fd84243 100644 --- a/include/assimp/MemoryIOWrapper.h +++ b/include/assimp/MemoryIOWrapper.h @@ -65,23 +65,21 @@ namespace Assimp { // ---------------------------------------------------------------------------------- class MemoryIOStream : public IOStream { public: - MemoryIOStream (const uint8_t* buff, size_t len, bool own = false) - : buffer (buff) - , length(len) - , pos((size_t)0) - , own(own) { + MemoryIOStream (const uint8_t* buff, size_t len, bool own = false) : + buffer (buff), + length(len), + pos(static_cast(0)), + own(own) { // empty } - ~MemoryIOStream () { + ~MemoryIOStream() override { if(own) { delete[] buffer; } } - // ------------------------------------------------------------------- - // Read from stream - size_t Read(void* pvBuffer, size_t pSize, size_t pCount) { + size_t Read(void* pvBuffer, size_t pSize, size_t pCount) override { ai_assert(nullptr != pvBuffer); ai_assert(0 != pSize); @@ -94,16 +92,12 @@ class MemoryIOStream : public IOStream { return cnt; } - // ------------------------------------------------------------------- - // Write to stream - size_t Write(const void* /*pvBuffer*/, size_t /*pSize*/,size_t /*pCount*/) { + size_t Write(const void*, size_t, size_t ) override { ai_assert(false); // won't be needed return 0; } - // ------------------------------------------------------------------- - // Seek specific position - aiReturn Seek(size_t pOffset, aiOrigin pOrigin) { + aiReturn Seek(size_t pOffset, aiOrigin pOrigin) override { if (aiOrigin_SET == pOrigin) { if (pOffset > length) { return AI_FAILURE; @@ -123,20 +117,14 @@ class MemoryIOStream : public IOStream { return AI_SUCCESS; } - // ------------------------------------------------------------------- - // Get current seek position - size_t Tell() const { + size_t Tell() const override { return pos; } - // ------------------------------------------------------------------- - // Get size of file - size_t FileSize() const { + size_t FileSize() const override { return length; } - // ------------------------------------------------------------------- - // Flush file contents void Flush() { ai_assert(false); // won't be needed } @@ -148,19 +136,19 @@ class MemoryIOStream : public IOStream { }; // --------------------------------------------------------------------------- -/** Dummy IO system to read from a memory buffer */ +/// @brief Dummy IO system to read from a memory buffer. class MemoryIOSystem : public IOSystem { public: - /** Constructor. */ + /// @brief Constructor. MemoryIOSystem(const uint8_t* buff, size_t len, IOSystem* io) : buffer(buff), length(len), existing_io(io) { // empty } - /** Destructor. */ + /// @brief Destructor. ~MemoryIOSystem() = default; // ------------------------------------------------------------------- - /** Tests for the existence of a file at the given path. */ + /// @brief Tests for the existence of a file at the given path. bool Exists(const char* pFile) const override { printf("Exists\n"); if (0 == strncmp( pFile, AI_MEMORYIO_MAGIC_FILENAME, AI_MEMORYIO_MAGIC_FILENAME_LENGTH ) ) { @@ -170,14 +158,14 @@ class MemoryIOSystem : public IOSystem { } // ------------------------------------------------------------------- - /** Returns the directory separator. */ + /// @brief Returns the directory separator. char getOsSeparator() const override { return existing_io ? existing_io->getOsSeparator() : '/'; // why not? it doesn't care } // ------------------------------------------------------------------- - /** Open a new file with a given path. */ + /// @brief Open a new file with a given path. IOStream* Open(const char* pFile, const char* pMode = "rb") override { if ( 0 == strncmp( pFile, AI_MEMORYIO_MAGIC_FILENAME, AI_MEMORYIO_MAGIC_FILENAME_LENGTH ) ) { created_streams.emplace_back(new MemoryIOStream(buffer, length)); @@ -187,7 +175,7 @@ class MemoryIOSystem : public IOSystem { } // ------------------------------------------------------------------- - /** Closes the given file and releases all resources associated with it. */ + /// @brief Closes the given file and releases all resources associated with it. void Close( IOStream* pFile) override { auto it = std::find(created_streams.begin(), created_streams.end(), pFile); if (it != created_streams.end()) { @@ -199,36 +187,43 @@ class MemoryIOSystem : public IOSystem { } // ------------------------------------------------------------------- - /** Compare two paths */ + /// @brief Compare two paths bool ComparePaths(const char* one, const char* second) const override { return existing_io ? existing_io->ComparePaths(one, second) : false; } - + + /// @brief Will push the directory. bool PushDirectory( const std::string &path ) override { return existing_io ? existing_io->PushDirectory(path) : false; } + /// @brief Will return the current directory from the stack top. const std::string &CurrentDirectory() const override { static std::string empty; return existing_io ? existing_io->CurrentDirectory() : empty; } + /// @brief Returns the stack size. size_t StackSize() const override { return existing_io ? existing_io->StackSize() : 0; } + /// @brief Will pop the upper directory. bool PopDirectory() override { return existing_io ? existing_io->PopDirectory() : false; } + /// @brief Will create the directory. bool CreateDirectory( const std::string &path ) override { return existing_io ? existing_io->CreateDirectory(path) : false; } - + + /// @brief Will change the directory. bool ChangeDirectory( const std::string &path ) override { return existing_io ? existing_io->ChangeDirectory(path) : false; } + /// @brief Will delete the file. bool DeleteFile( const std::string &file ) override { return existing_io ? existing_io->DeleteFile(file) : false; }