From 2d372b302ffdaeb7ac629ca105b8a13e6de584e8 Mon Sep 17 00:00:00 2001 From: Umesh Rajesh Ramchandani Date: Fri, 2 Dec 2022 13:59:08 +0100 Subject: [PATCH 1/8] Fixed bug when exporting binary FBX Fixed vector subscript out of range bug when NULL_RECORD is passed to PutString and is actually null --- code/AssetLib/FBX/FBXCommon.h | 1 + code/AssetLib/FBX/FBXExportNode.cpp | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/code/AssetLib/FBX/FBXCommon.h b/code/AssetLib/FBX/FBXCommon.h index c592c5649..c3d715892 100644 --- a/code/AssetLib/FBX/FBXCommon.h +++ b/code/AssetLib/FBX/FBXCommon.h @@ -55,6 +55,7 @@ const char NULL_RECORD[NumNullRecords] = { // 25 null bytes in 64-bit and 13 nul '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0' }; // who knows why, it looks like two integers 32/64 bit (compressed and uncompressed sizes?) + 1 byte (might be compression type?) +static std::string NULL_RECORD_STRING(NumNullRecords, '\0'); const std::string SEPARATOR = { '\x00', '\x01' }; // for use inside strings const std::string MAGIC_NODE_TAG = "_$AssimpFbx$"; // from import const int64_t SECOND = 46186158000; // FBX's kTime unit diff --git a/code/AssetLib/FBX/FBXExportNode.cpp b/code/AssetLib/FBX/FBXExportNode.cpp index 21c61b257..21e591425 100644 --- a/code/AssetLib/FBX/FBXExportNode.cpp +++ b/code/AssetLib/FBX/FBXExportNode.cpp @@ -360,7 +360,7 @@ void FBX::Node::EndBinary( bool has_children ) { // if there were children, add a null record - if (has_children) { s.PutString(Assimp::FBX::NULL_RECORD); } + if (has_children) { s.PutString(Assimp::FBX::NULL_RECORD_STRING); } // now go back and write initial pos this->end_pos = s.Tell(); From d5294be00b0f89486b3459d5835045dbf26444b5 Mon Sep 17 00:00:00 2001 From: Alex Date: Sat, 10 Dec 2022 01:22:00 +0000 Subject: [PATCH 2/8] Fixes Heap-buffer-overflow READ 4 in Assimp::ScenePreprocessor::ProcessMesh https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=49797 --- code/AssetLib/OFF/OFFLoader.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/code/AssetLib/OFF/OFFLoader.cpp b/code/AssetLib/OFF/OFFLoader.cpp index a366d7463..cb265029a 100644 --- a/code/AssetLib/OFF/OFFLoader.cpp +++ b/code/AssetLib/OFF/OFFLoader.cpp @@ -290,11 +290,12 @@ void OFFImporter::InternReadFile( const std::string& pFile, aiScene* pScene, IOS sz = line; SkipSpaces(&sz); idx = strtoul10(sz,&sz); if(!idx || idx > 9) { - ASSIMP_LOG_ERROR("OFF: Faces with zero indices aren't allowed"); + ASSIMP_LOG_ERROR("OFF: Faces with zero indices aren't allowed"); --mesh->mNumFaces; + ++i; continue; - } - faces->mNumIndices = idx; + } + faces->mNumIndices = idx; faces->mIndices = new unsigned int[faces->mNumIndices]; for (unsigned int m = 0; m < faces->mNumIndices;++m) { SkipSpaces(&sz); From 90769ef3e6a6d05691e46024b7415aafda4b9c7d Mon Sep 17 00:00:00 2001 From: Alex Date: Sun, 11 Dec 2022 00:02:09 +0000 Subject: [PATCH 3/8] Fixes Heap-buffer-overflow READ 1 in Assimp::MD5::MD5Parser::ParseHeader https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=49422 When it reaches the `SkipSpacesAndLineEnd`, `in` already points past `bufferEnd` and it leads to out of bounds memory read. --- code/AssetLib/MD5/MD5Parser.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/code/AssetLib/MD5/MD5Parser.cpp b/code/AssetLib/MD5/MD5Parser.cpp index 2cd738581..02b34fe4b 100644 --- a/code/AssetLib/MD5/MD5Parser.cpp +++ b/code/AssetLib/MD5/MD5Parser.cpp @@ -117,6 +117,8 @@ void MD5Parser::ParseHeader() { ReportError("MD5 version tag is unknown (10 is expected)"); } SkipLine(); + if (buffer == bufferEnd) + return; // print the command line options to the console // FIX: can break the log length limit, so we need to be careful From 917352dd8b44292c5c3490de8365932edb24c9a1 Mon Sep 17 00:00:00 2001 From: sashashura Date: Sun, 11 Dec 2022 01:54:57 +0100 Subject: [PATCH 4/8] Fixes Heap-buffer-overflow READ 1 in Assimp::ObjFileParser::getFace https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=49274 --- code/AssetLib/Obj/ObjFileParser.cpp | 2 +- code/AssetLib/Obj/ObjTools.h | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/code/AssetLib/Obj/ObjFileParser.cpp b/code/AssetLib/Obj/ObjFileParser.cpp index 42bd23689..360c1d0e9 100644 --- a/code/AssetLib/Obj/ObjFileParser.cpp +++ b/code/AssetLib/Obj/ObjFileParser.cpp @@ -440,7 +440,7 @@ void ObjFileParser::getFace(aiPrimitiveType type) { const bool vt = (!m_pModel->mTextureCoord.empty()); const bool vn = (!m_pModel->mNormals.empty()); int iPos = 0; - while (m_DataIt != m_DataItEnd) { + while (m_DataIt < m_DataItEnd) { int iStep = 1; if (IsLineEnd(*m_DataIt)) { diff --git a/code/AssetLib/Obj/ObjTools.h b/code/AssetLib/Obj/ObjTools.h index a24bfd5a2..99d2bc5e3 100644 --- a/code/AssetLib/Obj/ObjTools.h +++ b/code/AssetLib/Obj/ObjTools.h @@ -111,6 +111,9 @@ inline Char_T getNextToken(Char_T pBuffer, Char_T pEnd) { */ template inline char_t skipLine(char_t it, char_t end, unsigned int &uiLine) { + if (it >= end) + return it; + while (!isEndOfBuffer(it, end) && !IsLineEnd(*it)) { ++it; } From db8ff416799bbbed7195c9b7e0c98292e8631ce1 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Thu, 15 Dec 2022 14:06:57 +0100 Subject: [PATCH 5/8] Update MD5Parser.cpp --- code/AssetLib/MD5/MD5Parser.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/code/AssetLib/MD5/MD5Parser.cpp b/code/AssetLib/MD5/MD5Parser.cpp index 02b34fe4b..dce4c5732 100644 --- a/code/AssetLib/MD5/MD5Parser.cpp +++ b/code/AssetLib/MD5/MD5Parser.cpp @@ -5,8 +5,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, @@ -117,8 +115,9 @@ void MD5Parser::ParseHeader() { ReportError("MD5 version tag is unknown (10 is expected)"); } SkipLine(); - if (buffer == bufferEnd) + if (buffer == bufferEnd) { return; + } // print the command line options to the console // FIX: can break the log length limit, so we need to be careful @@ -137,8 +136,9 @@ bool MD5Parser::ParseSection(Section &out) { // first parse the name of the section char *sz = buffer; - while (!IsSpaceOrNewLine(*buffer)) - buffer++; + while (!IsSpaceOrNewLine(*buffer)) { + ++buffer; + } out.mName = std::string(sz, (uintptr_t)(buffer - sz)); SkipSpaces(); @@ -146,14 +146,14 @@ bool MD5Parser::ParseSection(Section &out) { while (running) { if ('{' == *buffer) { // it is a normal section so read all lines - buffer++; + ++buffer; bool run = true; while (run) { if (!SkipSpacesAndLineEnd()) { return false; // seems this was the last section } if ('}' == *buffer) { - buffer++; + ++buffer; break; } @@ -165,7 +165,7 @@ bool MD5Parser::ParseSection(Section &out) { // terminate the line with zero while (!IsLineEnd(*buffer)) - buffer++; + ++buffer; if (*buffer) { ++lineNumber; *buffer++ = '\0'; From c3d15a3f5166d295544cd16243f4c8e3d352d4a6 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Fri, 16 Dec 2022 09:03:40 +0100 Subject: [PATCH 6/8] Fix minor review findings. --- code/AssetLib/Obj/ObjTools.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/code/AssetLib/Obj/ObjTools.h b/code/AssetLib/Obj/ObjTools.h index 99d2bc5e3..a76054825 100644 --- a/code/AssetLib/Obj/ObjTools.h +++ b/code/AssetLib/Obj/ObjTools.h @@ -2,7 +2,7 @@ Open Asset Import Library (assimp) ---------------------------------------------------------------------- -Copyright (c) 2006-2021, assimp team +Copyright (c) 2006-2022, assimp team All rights reserved. @@ -111,8 +111,9 @@ inline Char_T getNextToken(Char_T pBuffer, Char_T pEnd) { */ template inline char_t skipLine(char_t it, char_t end, unsigned int &uiLine) { - if (it >= end) + if (it >= end) { return it; + } while (!isEndOfBuffer(it, end) && !IsLineEnd(*it)) { ++it; @@ -132,7 +133,7 @@ inline char_t skipLine(char_t it, char_t end, unsigned int &uiLine) { /** * @brief Get a name from the current line. Preserve space in the middle, - * but trim it at the end. + * but trim it at the end. * @param[in] it set to current position * @param[in] end set to end of scratch buffer for readout * @param[out] name Separated name From 6743274b3182093dbee9a41836a14971331f5a3d Mon Sep 17 00:00:00 2001 From: RKJ <37873142+rohit-kumar-j@users.noreply.github.com> Date: Mon, 19 Dec 2022 04:06:36 +0530 Subject: [PATCH 7/8] illegal token on right-side-of ::Windows Error without: ``` assimp\material.inl(101,47): message : error recovery skipped: ') ?' ``` Reference : https://stackoverflow.com/questions/2561368/illegal-token-on-right-side-of --- include/assimp/material.inl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/assimp/material.inl b/include/assimp/material.inl index cdf272201..0f80fdecc 100644 --- a/include/assimp/material.inl +++ b/include/assimp/material.inl @@ -98,8 +98,10 @@ AI_FORCE_INLINE aiReturn aiMaterial::Get(const char* pKey,unsigned int type, return AI_FAILURE; } +#undef max +#undef min iNum = static_cast(std::min(static_cast(iNum),prop->mDataLength / sizeof(Type))); - ::memcpy(pOut,prop->mData,iNum * sizeof(Type)); + std::memcpy(pOut,prop->mData,iNum * sizeof(Type)); if (pMax) { *pMax = iNum; } From 76de8ba1f4163828c7a29efe92d326a865158b7c Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Mon, 19 Dec 2022 08:58:45 +0100 Subject: [PATCH 8/8] Update material.inl --- include/assimp/material.inl | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/include/assimp/material.inl b/include/assimp/material.inl index 0f80fdecc..342c26646 100644 --- a/include/assimp/material.inl +++ b/include/assimp/material.inl @@ -97,9 +97,10 @@ AI_FORCE_INLINE aiReturn aiMaterial::Get(const char* pKey,unsigned int type, if (prop->mType != aiPTI_Buffer) { return AI_FAILURE; } - -#undef max -#undef min +// std::min has in some cases a conflict with a defined min +#ifdef min +# undef min +#endif iNum = static_cast(std::min(static_cast(iNum),prop->mDataLength / sizeof(Type))); std::memcpy(pOut,prop->mData,iNum * sizeof(Type)); if (pMax) {