Skip to content

Commit

Permalink
Merge pull request assimp#4943 from assimp/kimkulling/fix_leak_issue-…
Browse files Browse the repository at this point in the history 
…3416

Fix: Fix memleak when exiting method by exception
  • Loading branch information
@kimkulling
kimkulling authored Feb 8, 2023
2 parents dd6bcec + b7d08fc commit 4cf1ee9
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 70 deletions.
58 changes: 32 additions & 26 deletions code/Common/Importer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<const aiScene*>()(); \
} catch(...) {
pimpl->mErrorString = "Unknown exception";
pimpl->mException = std::current_exception();
SetIOHandler(io);
return ExceptionSwallower<const aiScene*>()(); \

ReadFile(fbuff,pFlags);
SetIOHandler(io);
}

ASSIMP_END_EXCEPTION_REGION_WITH_ERROR_STRING(const aiScene*, pimpl->mErrorString, pimpl->mException);
return pimpl->mScene;
}

Expand Down
7 changes: 4 additions & 3 deletions fuzz/assimp_fuzzer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
Open Asset Import Library (assimp)
---------------------------------------------------------------------------
Copyright (c) 2006-2020, assimp team
Copyright (c) 2006-2023, assimp team
All rights reserved.
Expand Down Expand Up @@ -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,
Expand All @@ -57,3 +57,4 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t dataSize) {

return 0;
}

73 changes: 32 additions & 41 deletions include/assimp/MemoryIOWrapper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -66,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<size_t>(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);

Expand All @@ -95,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;
Expand All @@ -124,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
}
Expand All @@ -149,49 +136,46 @@ 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. */
MemoryIOSystem(const uint8_t* buff, size_t len, IOSystem* io)
: buffer(buff)
, length(len)
, existing_io(io)
, created_streams() {
/// @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 ) ) {
return true;
}
return existing_io ? existing_io->Exists(pFile) : false;
}

// -------------------------------------------------------------------
/** 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));
return created_streams.back();
}
return existing_io ? existing_io->Open(pFile, pMode) : NULL;
return existing_io ? existing_io->Open(pFile, pMode) : nullptr;
}

// -------------------------------------------------------------------
/** 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()) {
Expand All @@ -203,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;
}
Expand All @@ -246,4 +237,4 @@ class MemoryIOSystem : public IOSystem {

} // end namespace Assimp

#endif
#endif // AI_MEMORYIOSTREAM_H_INC

0 comments on commit 4cf1ee9

Please sign in to comment.