From 7b59cc297e26f3e94b7c8a91b779fe21e355288f Mon Sep 17 00:00:00 2001 From: Neil Clifford Date: Thu, 19 Nov 2020 12:20:06 +0000 Subject: [PATCH 1/4] FBXParser.cpp - handle buffer over-read cases correctly --- code/AssetLib/FBX/FBXParser.cpp | 48 +++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/code/AssetLib/FBX/FBXParser.cpp b/code/AssetLib/FBX/FBXParser.cpp index f93f69d4d..8d4bbd866 100644 --- a/code/AssetLib/FBX/FBXParser.cpp +++ b/code/AssetLib/FBX/FBXParser.cpp @@ -641,7 +641,11 @@ void ParseVectorDataArray(std::vector& out, const Element& el) ReadBinaryDataArray(type, count, data, end, buff, el); ai_assert(data == end); - ai_assert(buff.size() == count * (type == 'd' ? 8 : 4)); + uint64_t dataToRead = static_cast(count) * (type == 'd' ? 8 : 4); + ai_assert(buff.size() == dataToRead); + if (dataToRead > buff.size()) { + ParseError("Invalid read size (binary)",&el); + } const uint32_t count3 = count / 3; out.reserve(count3); @@ -728,7 +732,11 @@ void ParseVectorDataArray(std::vector& out, const Element& el) ReadBinaryDataArray(type, count, data, end, buff, el); ai_assert(data == end); - ai_assert(buff.size() == count * (type == 'd' ? 8 : 4)); + uint64_t dataToRead = static_cast(count) * (type == 'd' ? 8 : 4); + ai_assert(buff.size() == dataToRead); + if (dataToRead > buff.size()) { + ParseError("Invalid read size (binary)",&el); + } const uint32_t count4 = count / 4; out.reserve(count4); @@ -807,7 +815,11 @@ void ParseVectorDataArray(std::vector& out, const Element& el) ReadBinaryDataArray(type, count, data, end, buff, el); ai_assert(data == end); - ai_assert(buff.size() == count * (type == 'd' ? 8 : 4)); + uint64_t dataToRead = static_cast(count) * (type == 'd' ? 8 : 4); + ai_assert(buff.size() == dataToRead); + if (dataToRead > buff.size()) { + ParseError("Invalid read size (binary)",&el); + } const uint32_t count2 = count / 2; out.reserve(count2); @@ -879,7 +891,11 @@ void ParseVectorDataArray(std::vector& out, const Element& el) ReadBinaryDataArray(type, count, data, end, buff, el); ai_assert(data == end); - ai_assert(buff.size() == count * 4); + uint64_t dataToRead = static_cast(count) * 4; + ai_assert(buff.size() == dataToRead); + if (dataToRead > buff.size()) { + ParseError("Invalid read size (binary)",&el); + } out.reserve(count); @@ -937,7 +953,11 @@ void ParseVectorDataArray(std::vector& out, const Element& el) ReadBinaryDataArray(type, count, data, end, buff, el); ai_assert(data == end); - ai_assert(buff.size() == count * (type == 'd' ? 8 : 4)); + uint64_t dataToRead = static_cast(count) * (type == 'd' ? 8 : 4); + ai_assert(buff.size() == dataToRead); + if (dataToRead > buff.size()) { + ParseError("Invalid read size (binary)",&el); + } if (type == 'd') { const double* d = reinterpret_cast(&buff[0]); @@ -998,7 +1018,11 @@ void ParseVectorDataArray(std::vector& out, const Element& el) ReadBinaryDataArray(type, count, data, end, buff, el); ai_assert(data == end); - ai_assert(buff.size() == count * 4); + uint64_t dataToRead = static_cast(count) * 4; + ai_assert(buff.size() == dataToRead); + if (dataToRead > buff.size()) { + ParseError("Invalid read size (binary)",&el); + } out.reserve(count); @@ -1063,7 +1087,11 @@ void ParseVectorDataArray(std::vector& out, const Element& el) ReadBinaryDataArray(type, count, data, end, buff, el); ai_assert(data == end); - ai_assert(buff.size() == count * 8); + uint64_t dataToRead = static_cast(count) * 8; + ai_assert(buff.size() == dataToRead); + if (dataToRead > buff.size()) { + ParseError("Invalid read size (binary)",&el); + } out.reserve(count); @@ -1121,7 +1149,11 @@ void ParseVectorDataArray(std::vector& out, const Element& el) ReadBinaryDataArray(type, count, data, end, buff, el); ai_assert(data == end); - ai_assert(buff.size() == count * 8); + uint64_t dataToRead = static_cast(count) * 8; + ai_assert(buff.size() == dataToRead); + if (dataToRead > buff.size()) { + ParseError("Invalid read size (binary)",&el); + } out.reserve(count); From 67abcb10ba19e793dacb9dae408b0c816abd25df Mon Sep 17 00:00:00 2001 From: Evangel Date: Sun, 22 Nov 2020 15:10:36 +1000 Subject: [PATCH 2/4] Added mName to aiScene. Primarily to provide access to the "name" member of glTF2 scenes. --- code/AssetLib/glTF2/glTF2Asset.h | 1 + code/AssetLib/glTF2/glTF2Asset.inl | 5 +++++ code/AssetLib/glTF2/glTF2Importer.cpp | 1 + include/assimp/scene.h | 7 +++++-- 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/code/AssetLib/glTF2/glTF2Asset.h b/code/AssetLib/glTF2/glTF2Asset.h index ead8ca1dd..23c19cc58 100644 --- a/code/AssetLib/glTF2/glTF2Asset.h +++ b/code/AssetLib/glTF2/glTF2Asset.h @@ -863,6 +863,7 @@ struct Sampler : public Object { }; struct Scene : public Object { + std::string name; std::vector> nodes; Scene() {} diff --git a/code/AssetLib/glTF2/glTF2Asset.inl b/code/AssetLib/glTF2/glTF2Asset.inl index 2c7e9fc5a..af9a4e94e 100644 --- a/code/AssetLib/glTF2/glTF2Asset.inl +++ b/code/AssetLib/glTF2/glTF2Asset.inl @@ -1400,6 +1400,11 @@ inline void Node::Read(Value &obj, Asset &r) { } inline void Scene::Read(Value &obj, Asset &r) { + if (Value *name = FindString(obj, "name")) { + if (name->IsString()) { + this->name = name->GetString(); + } + } if (Value *array = FindArray(obj, "nodes")) { for (unsigned int i = 0; i < array->Size(); ++i) { if (!(*array)[i].IsUint()) continue; diff --git a/code/AssetLib/glTF2/glTF2Importer.cpp b/code/AssetLib/glTF2/glTF2Importer.cpp index b1ba5b67b..ad0cea221 100644 --- a/code/AssetLib/glTF2/glTF2Importer.cpp +++ b/code/AssetLib/glTF2/glTF2Importer.cpp @@ -1386,6 +1386,7 @@ void glTF2Importer::InternReadFile(const std::string &pFile, aiScene *pScene, IO // read the asset file glTF2::Asset asset(pIOHandler); asset.Load(pFile, GetExtension(pFile) == "glb"); + pScene->mName = asset.scene->name; // // Copy the data out diff --git a/include/assimp/scene.h b/include/assimp/scene.h index a189f5700..2a9a77b02 100644 --- a/include/assimp/scene.h +++ b/include/assimp/scene.h @@ -335,12 +335,15 @@ struct aiScene /** * @brief The global metadata assigned to the scene itself. * - * This data contains global metadata which belongs to the scene like - * unit-conversions, versions, vendors or other model-specific data. This + * This data contains global metadata which belongs to the scene like + * unit-conversions, versions, vendors or other model-specific data. This * can be used to store format-specific metadata as well. */ C_STRUCT aiMetadata* mMetaData; + /** The name of the scene itself. + */ + C_STRUCT aiString mName; #ifdef __cplusplus From 30584c1ec178d9756be551287d9ab2e9eca63a7e Mon Sep 17 00:00:00 2001 From: Evangel Date: Sun, 22 Nov 2020 15:36:08 +1000 Subject: [PATCH 3/4] Replaced name with scene_name to avoid shadowing. --- code/AssetLib/glTF2/glTF2Asset.inl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/code/AssetLib/glTF2/glTF2Asset.inl b/code/AssetLib/glTF2/glTF2Asset.inl index af9a4e94e..c892e6697 100644 --- a/code/AssetLib/glTF2/glTF2Asset.inl +++ b/code/AssetLib/glTF2/glTF2Asset.inl @@ -1400,9 +1400,9 @@ inline void Node::Read(Value &obj, Asset &r) { } inline void Scene::Read(Value &obj, Asset &r) { - if (Value *name = FindString(obj, "name")) { - if (name->IsString()) { - this->name = name->GetString(); + if (Value *scene_name = FindString(obj, "name")) { + if (scene_name->IsString()) { + this->name = scene_name->GetString(); } } if (Value *array = FindArray(obj, "nodes")) { From 98e42e22b8a922f96d5088db889d98bdd02129d9 Mon Sep 17 00:00:00 2001 From: Evangel Date: Sun, 22 Nov 2020 15:49:41 +1000 Subject: [PATCH 4/4] Added check around setting pScene->mName from asset.scene. --- code/AssetLib/glTF2/glTF2Importer.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/code/AssetLib/glTF2/glTF2Importer.cpp b/code/AssetLib/glTF2/glTF2Importer.cpp index ad0cea221..738186414 100644 --- a/code/AssetLib/glTF2/glTF2Importer.cpp +++ b/code/AssetLib/glTF2/glTF2Importer.cpp @@ -1386,7 +1386,9 @@ void glTF2Importer::InternReadFile(const std::string &pFile, aiScene *pScene, IO // read the asset file glTF2::Asset asset(pIOHandler); asset.Load(pFile, GetExtension(pFile) == "glb"); - pScene->mName = asset.scene->name; + if (asset.scene) { + pScene->mName = asset.scene->name; + } // // Copy the data out