From 6ed1488e6e6e05b7424245548b32b7efdb125291 Mon Sep 17 00:00:00 2001 From: Max Vollmer Date: Mon, 30 Nov 2020 15:04:06 +0000 Subject: [PATCH 1/3] * Improved error messages * Throw instead of asserts on invalid file input --- code/AssetLib/glTF/glTFCommon.cpp | 4 +++- code/AssetLib/glTF/glTFCommon.h | 5 ++++- code/AssetLib/glTF2/glTF2Asset.h | 8 ++++++++ code/AssetLib/glTF2/glTF2Asset.inl | 22 +++++++++------------- code/AssetLib/glTF2/glTF2Importer.cpp | 5 ++++- 5 files changed, 28 insertions(+), 16 deletions(-) diff --git a/code/AssetLib/glTF/glTFCommon.cpp b/code/AssetLib/glTF/glTFCommon.cpp index 6bea18a0a..01ba31209 100644 --- a/code/AssetLib/glTF/glTFCommon.cpp +++ b/code/AssetLib/glTF/glTFCommon.cpp @@ -49,7 +49,9 @@ using namespace glTFCommon::Util; namespace Util { size_t DecodeBase64(const char *in, size_t inLength, uint8_t *&out) { - ai_assert(inLength % 4 == 0); + if (inLength % 4 != 0) { + throw DeadlyImportError("Invalid base64 encoded data: \"", std::string(in, std::min(size_t(32), inLength)), "\", length:", inLength); + } if (inLength < 4) { out = 0; diff --git a/code/AssetLib/glTF/glTFCommon.h b/code/AssetLib/glTF/glTFCommon.h index 977fc0da4..bcb319c47 100644 --- a/code/AssetLib/glTF/glTFCommon.h +++ b/code/AssetLib/glTF/glTFCommon.h @@ -249,7 +249,10 @@ inline char EncodeCharBase64(uint8_t b) { } inline uint8_t DecodeCharBase64(char c) { - return DATA::tableDecodeBase64[size_t(c)]; // TODO faster with lookup table or ifs? + if (c & 0xF0) { + throw DeadlyImportError("Invalid base64 char value: ", size_t(c)); + } + return DATA::tableDecodeBase64[size_t(c & 0x0F)]; // TODO faster with lookup table or ifs? } size_t DecodeBase64(const char *in, size_t inLength, uint8_t *&out); diff --git a/code/AssetLib/glTF2/glTF2Asset.h b/code/AssetLib/glTF2/glTF2Asset.h index 23c19cc58..be149f86e 100644 --- a/code/AssetLib/glTF2/glTF2Asset.h +++ b/code/AssetLib/glTF2/glTF2Asset.h @@ -1124,6 +1124,14 @@ private: IOStream *OpenFile(std::string path, const char *mode, bool absolute = false); }; +inline std::string getContextForErrorMessages(const std::string &id, const std::string &name) { + std::string context = id; + if (!name.empty()) { + context += " (\"" + name + "\")"; + } + return context; +} + } // namespace glTF2 // Include the implementation of the methods diff --git a/code/AssetLib/glTF2/glTF2Asset.inl b/code/AssetLib/glTF2/glTF2Asset.inl index c892e6697..0c6ce6e6d 100644 --- a/code/AssetLib/glTF2/glTF2Asset.inl +++ b/code/AssetLib/glTF2/glTF2Asset.inl @@ -273,17 +273,21 @@ Ref LazyDict::Retrieve(unsigned int i) { } if (!mDict->IsArray()) { - throw DeadlyImportError("GLTF: Field is not an array \"", mDictId, "\""); + throw DeadlyImportError("GLTF: Field \"", mDictId, "\" is not an array"); + } + + if (i >= mDict->Size()) { + throw DeadlyImportError("GLTF: Array index ", i, " is out of bounds (", mDict->Size(), ") for \"", mDictId, "\""); } Value &obj = (*mDict)[i]; if (!obj.IsObject()) { - throw DeadlyImportError("GLTF: Object at index \"", to_string(i), "\" is not a JSON object"); + throw DeadlyImportError("GLTF: Object at index ", to_string(i), " is not a JSON object"); } if (mRecursiveReferenceCheck.find(i) != mRecursiveReferenceCheck.end()) { - throw DeadlyImportError("GLTF: Object at index \"", to_string(i), "\" has recursive reference to itself"); + throw DeadlyImportError("GLTF: Object at index ", to_string(i), " has recursive reference to itself"); } mRecursiveReferenceCheck.insert(i); @@ -741,14 +745,6 @@ inline void CopyData(size_t count, } } -inline std::string getContextForErrorMessages(const std::string& id, const std::string& name) { - std::string context = id; - if (!name.empty()) { - context += " (\"" + name + "\")"; - } - return context; -} - } // namespace template @@ -766,11 +762,11 @@ void Accessor::ExtractData(T *&outData) { const size_t targetElemSize = sizeof(T); if (elemSize > targetElemSize) { - throw DeadlyImportError("GLTF: elemSize > targetElemSize"); + throw DeadlyImportError("GLTF: elemSize ", elemSize, " > targetElemSize ", targetElemSize, " in ", getContextForErrorMessages(id, name)); } if (count*stride > (bufferView ? bufferView->byteLength : sparse->data.size())) { - throw DeadlyImportError("GLTF: count*stride out of range"); + throw DeadlyImportError("GLTF: count*stride ", (count * stride), " > bufferView->byteLength ", bufferView->byteLength, " in ", getContextForErrorMessages(id, name)); } outData = new T[count]; diff --git a/code/AssetLib/glTF2/glTF2Importer.cpp b/code/AssetLib/glTF2/glTF2Importer.cpp index 3fb7889af..672bac52d 100644 --- a/code/AssetLib/glTF2/glTF2Importer.cpp +++ b/code/AssetLib/glTF2/glTF2Importer.cpp @@ -918,7 +918,10 @@ aiNode *ImportNode(aiScene *pScene, glTF2::Asset &r, std::vector & if (!node.meshes.empty()) { // GLTF files contain at most 1 mesh per node. - assert(node.meshes.size() == 1); + if (node.meshes.size() > 1) + { + throw DeadlyImportError("GLTF: Invalid input, found ", node.meshes.size(), " meshes in ", getContextForErrorMessages(node.id, node.name), ", but only 1 mesh per node allowed."); + } int mesh_idx = node.meshes[0].GetIndex(); int count = meshOffsets[mesh_idx + 1] - meshOffsets[mesh_idx]; From 7d72c78c8e2c8333a46094a4385c85752a06a491 Mon Sep 17 00:00:00 2001 From: Max Vollmer Date: Mon, 30 Nov 2020 15:20:51 +0000 Subject: [PATCH 2/3] Some improvements --- code/AssetLib/glTF2/glTF2Asset.inl | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/code/AssetLib/glTF2/glTF2Asset.inl b/code/AssetLib/glTF2/glTF2Asset.inl index 0c6ce6e6d..8fc8bf24e 100644 --- a/code/AssetLib/glTF2/glTF2Asset.inl +++ b/code/AssetLib/glTF2/glTF2Asset.inl @@ -283,11 +283,11 @@ Ref LazyDict::Retrieve(unsigned int i) { Value &obj = (*mDict)[i]; if (!obj.IsObject()) { - throw DeadlyImportError("GLTF: Object at index ", to_string(i), " is not a JSON object"); + throw DeadlyImportError("GLTF: Object at index ", i, " in array \"", mDictId, "\" is not a JSON object"); } if (mRecursiveReferenceCheck.find(i) != mRecursiveReferenceCheck.end()) { - throw DeadlyImportError("GLTF: Object at index ", to_string(i), " has recursive reference to itself"); + throw DeadlyImportError("GLTF: Object at index ", i, " in array \"", mDictId, "\" has recursive reference to itself"); } mRecursiveReferenceCheck.insert(i); @@ -765,8 +765,9 @@ void Accessor::ExtractData(T *&outData) { throw DeadlyImportError("GLTF: elemSize ", elemSize, " > targetElemSize ", targetElemSize, " in ", getContextForErrorMessages(id, name)); } - if (count*stride > (bufferView ? bufferView->byteLength : sparse->data.size())) { - throw DeadlyImportError("GLTF: count*stride ", (count * stride), " > bufferView->byteLength ", bufferView->byteLength, " in ", getContextForErrorMessages(id, name)); + const size_t maxSize = (bufferView ? bufferView->byteLength : sparse->data.size()); + if (count*stride > maxSize) { + throw DeadlyImportError("GLTF: count*stride ", (count * stride), " > maxSize ", maxSize, " in ", getContextForErrorMessages(id, name)); } outData = new T[count]; From 53ff0702ce08d0f9f63b885eebdf2cbf1b516490 Mon Sep 17 00:00:00 2001 From: Max Vollmer Date: Mon, 30 Nov 2020 16:21:29 +0000 Subject: [PATCH 3/3] Fixed check for base64 char values --- code/AssetLib/glTF/glTFCommon.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/code/AssetLib/glTF/glTFCommon.h b/code/AssetLib/glTF/glTFCommon.h index bcb319c47..6d402b0e3 100644 --- a/code/AssetLib/glTF/glTFCommon.h +++ b/code/AssetLib/glTF/glTFCommon.h @@ -249,10 +249,10 @@ inline char EncodeCharBase64(uint8_t b) { } inline uint8_t DecodeCharBase64(char c) { - if (c & 0xF0) { + if (c & 0x80) { throw DeadlyImportError("Invalid base64 char value: ", size_t(c)); } - return DATA::tableDecodeBase64[size_t(c & 0x0F)]; // TODO faster with lookup table or ifs? + return DATA::tableDecodeBase64[size_t(c & 0x7F)]; // TODO faster with lookup table or ifs? } size_t DecodeBase64(const char *in, size_t inLength, uint8_t *&out);