From d7dbdd6c3bc24497131b649291c80110dcd6a988 Mon Sep 17 00:00:00 2001 From: henrikbuchholz Date: Thu, 15 Aug 2013 16:38:28 +0200 Subject: [PATCH 1/3] SortByPType Posprocessing crashed for crappy models with degenerated geometry Which models crashed before the fix?: The crash was observed for files with the following properties: 1. They contain >=1 meshes 2. They were loaded with SortByPType option 3. They only contained degenerated meshes, so that these were skipped SortByPType What is improved by the fix?: Obviously, the affected models were crappy anyway and will still produce empty output after the fix. However, the fix avoids the heap-corruption, which couldn't be solved by try/catch from outside and had the annoying effect that a whole scene with hundreds of individual models could crash due to a single crappy one. Why did it crash before? The SortByPType deleted some exluded meshes, but didn't reset the pointers in pScene. After throwing the DeadlyImportException (no remaining meshes), remaining meshes were deleted => Excluded meshes were deleted twice. --- code/SortByPTypeProcess.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/code/SortByPTypeProcess.cpp b/code/SortByPTypeProcess.cpp index 50edf29a5..c38bdd9cb 100644 --- a/code/SortByPTypeProcess.cpp +++ b/code/SortByPTypeProcess.cpp @@ -151,7 +151,7 @@ void SortByPTypeProcess::Execute( aiScene* pScene) std::vector::iterator meshIdx = replaceMeshIndex.begin(); for (unsigned int i = 0; i < pScene->mNumMeshes;++i) { - aiMesh* mesh = pScene->mMeshes[i]; + aiMesh* const mesh = pScene->mMeshes[i]; ai_assert(0 != mesh->mPrimitiveTypes); // if there's just one primitive type in the mesh there's nothing to do for us @@ -367,6 +367,9 @@ void SortByPTypeProcess::Execute( aiScene* pScene) // delete the input mesh delete mesh; + + // avoid invalid pointer + pScene->mMeshes[i] = NULL; } if (outMeshes.empty()) From 35fa7cb441ec594a04f21f4bc8dc71bac619f561 Mon Sep 17 00:00:00 2001 From: henrikbuchholz Date: Wed, 14 Aug 2013 15:53:33 +0200 Subject: [PATCH 2/3] 3DSLoader: Skipped chunks of size 0 --- code/3DSLoader.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/code/3DSLoader.cpp b/code/3DSLoader.cpp index 336a1390b..f16f3a27d 100644 --- a/code/3DSLoader.cpp +++ b/code/3DSLoader.cpp @@ -79,6 +79,8 @@ static const aiImporterDesc desc = { Discreet3DS::Chunk chunk; \ ReadChunk(&chunk); \ int chunkSize = chunk.Size-sizeof(Discreet3DS::Chunk); \ + if(chunkSize <= 0) \ + continue; \ const int oldReadLimit = stream->GetReadLimit(); \ stream->SetReadLimit(stream->GetCurrentPos() + chunkSize); \ From 5983300422509e6a3465905bd916763b29b99041 Mon Sep 17 00:00:00 2001 From: henrikbuchholz Date: Tue, 26 Nov 2013 14:52:53 +0100 Subject: [PATCH 3/3] Updated some code comments in DefaultIOStream.cpp There was a misleading TODO comment that encouraged to use fseek/ftell instead of fstat. However, fstat has been used intionally because fseek/ftell is potentially unsafe. So I replaced the TODO and added some explanation why fstat is being used instead. --- code/DefaultIOStream.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/code/DefaultIOStream.cpp b/code/DefaultIOStream.cpp index 9f9a3de78..1553bd8c1 100644 --- a/code/DefaultIOStream.cpp +++ b/code/DefaultIOStream.cpp @@ -110,7 +110,14 @@ size_t DefaultIOStream::FileSize() const if (SIZE_MAX == cachedSize) { - // TODO: Is that really faster if we're already owning a handle to the file? + // Although fseek/ftell would allow us to reuse the exising file handle here, + // it is generally unsafe because: + // - For binary streams, it is not technically well-defined + // - For text files the results are meaningless + // That's why we use the safer variant fstat here. + // + // See here for details: + // https://www.securecoding.cert.org/confluence/display/seccode/FIO19-C.+Do+not+use+fseek()+and+ftell()+to+compute+the+size+of+a+regular+file #if defined _WIN32 && !defined __GNUC__ struct __stat64 fileStat; int err = _stat64( mFilename.c_str(), &fileStat );