From 746d5cf964120967a72322ad98723390707f15f4 Mon Sep 17 00:00:00 2001 From: "Max Vollmer (Microsoft Havok)" <60260460+ms-maxvollmer@users.noreply.github.com> Date: Wed, 21 Apr 2021 16:17:03 +0100 Subject: [PATCH 1/3] * Throw instead of assert on invalid file input * Check JSON object type before accessing members * Ensure samplers input and output references are set before accessing them --- code/AssetLib/glTF2/glTF2Asset.inl | 27 ++++++++++++++++++++-- code/AssetLib/glTF2/glTF2Importer.cpp | 6 ++--- code/PostProcessing/SortByPTypeProcess.cpp | 4 +++- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/code/AssetLib/glTF2/glTF2Asset.inl b/code/AssetLib/glTF2/glTF2Asset.inl index 77537028f..8a793c144 100644 --- a/code/AssetLib/glTF2/glTF2Asset.inl +++ b/code/AssetLib/glTF2/glTF2Asset.inl @@ -162,6 +162,9 @@ inline static bool ReadValue(Value &val, T &out) { template inline static bool ReadMember(Value &obj, const char *id, T &out) { + if (!obj.IsObject()) { + return false; + } Value::MemberIterator it = obj.FindMember(id); if (it != obj.MemberEnd()) { return ReadHelper::Read(it->value, out); @@ -176,6 +179,9 @@ inline static T MemberOrDefault(Value &obj, const char *id, T defaultValue) { } inline Value *FindMember(Value &val, const char *id) { + if (!val.IsObject()) { + return nullptr; + } Value::MemberIterator it = val.FindMember(id); return (it != val.MemberEnd()) ? &it->value : nullptr; } @@ -193,6 +199,9 @@ inline void throwUnexpectedTypeError(const char (&expectedTypeName)[N], const ch // Look-up functions with type checks. Context and extra context help the user identify the problem if there's an error. inline Value *FindStringInContext(Value &val, const char *memberId, const char* context, const char* extraContext = nullptr) { + if (!val.IsObject()) { + return nullptr; + } Value::MemberIterator it = val.FindMember(memberId); if (it == val.MemberEnd()) { return nullptr; @@ -204,6 +213,9 @@ inline Value *FindStringInContext(Value &val, const char *memberId, const char* } inline Value *FindNumberInContext(Value &val, const char *memberId, const char* context, const char* extraContext = nullptr) { + if (!val.IsObject()) { + return nullptr; + } Value::MemberIterator it = val.FindMember(memberId); if (it == val.MemberEnd()) { return nullptr; @@ -215,6 +227,9 @@ inline Value *FindNumberInContext(Value &val, const char *memberId, const char* } inline Value *FindUIntInContext(Value &val, const char *memberId, const char* context, const char* extraContext = nullptr) { + if (!val.IsObject()) { + return nullptr; + } Value::MemberIterator it = val.FindMember(memberId); if (it == val.MemberEnd()) { return nullptr; @@ -226,6 +241,9 @@ inline Value *FindUIntInContext(Value &val, const char *memberId, const char* co } inline Value *FindArrayInContext(Value &val, const char *memberId, const char* context, const char* extraContext = nullptr) { + if (!val.IsObject()) { + return nullptr; + } Value::MemberIterator it = val.FindMember(memberId); if (it == val.MemberEnd()) { return nullptr; @@ -237,6 +255,9 @@ inline Value *FindArrayInContext(Value &val, const char *memberId, const char* c } inline Value *FindObjectInContext(Value &val, const char *memberId, const char* context, const char* extraContext = nullptr) { + if (!val.IsObject()) { + return nullptr; + } Value::MemberIterator it = val.FindMember(memberId); if (it == val.MemberEnd()) { return nullptr; @@ -886,7 +907,7 @@ inline void Accessor::Read(Value &obj, Asset &r) { componentType = MemberOrDefault(obj, "componentType", ComponentType_BYTE); { const Value* countValue = FindUInt(obj, "count"); - if (!countValue || countValue->GetInt() < 1) + if (!countValue || countValue->GetUint() < 1) { throw DeadlyImportError("A strictly positive count value is required, when reading ", id.c_str(), name.empty() ? "" : " (" + name + ")"); } @@ -1105,7 +1126,9 @@ inline Accessor::Indexer::Indexer(Accessor &acc) : template T Accessor::Indexer::GetValue(int i) { ai_assert(data); - ai_assert(i * stride < accessor.GetMaxByteSize()); + if (i * stride >= accessor.GetMaxByteSize()) { + throw DeadlyImportError("GLTF: Invalid index ", i, ", count out of range for buffer with stride ", stride, " and size ", accessor.GetMaxByteSize(), "."); + } // Ensure that the memcpy doesn't overwrite the local. const size_t sizeToCopy = std::min(elemSize, sizeof(T)); T value = T(); diff --git a/code/AssetLib/glTF2/glTF2Importer.cpp b/code/AssetLib/glTF2/glTF2Importer.cpp index dca6fcb2d..b6b6a364c 100644 --- a/code/AssetLib/glTF2/glTF2Importer.cpp +++ b/code/AssetLib/glTF2/glTF2Importer.cpp @@ -1170,7 +1170,7 @@ aiNodeAnim *CreateNodeAnim(glTF2::Asset&, Node &node, AnimationSamplers &sampler static const float kMillisecondsFromSeconds = 1000.f; - if (samplers.translation) { + if (samplers.translation && samplers.translation->input && samplers.translation->output) { float *times = nullptr; samplers.translation->input->ExtractData(times); aiVector3D *values = nullptr; @@ -1194,7 +1194,7 @@ aiNodeAnim *CreateNodeAnim(glTF2::Asset&, Node &node, AnimationSamplers &sampler anim->mPositionKeys->mValue.z = node.translation.value[2]; } - if (samplers.rotation) { + if (samplers.rotation && samplers.rotation->input && samplers.rotation->output) { float *times = nullptr; samplers.rotation->input->ExtractData(times); aiQuaternion *values = nullptr; @@ -1222,7 +1222,7 @@ aiNodeAnim *CreateNodeAnim(glTF2::Asset&, Node &node, AnimationSamplers &sampler anim->mRotationKeys->mValue.w = node.rotation.value[3]; } - if (samplers.scale) { + if (samplers.scale && samplers.scale->input && samplers.scale->output) { float *times = nullptr; samplers.scale->input->ExtractData(times); aiVector3D *values = nullptr; diff --git a/code/PostProcessing/SortByPTypeProcess.cpp b/code/PostProcessing/SortByPTypeProcess.cpp index dd6902692..38549b9a0 100644 --- a/code/PostProcessing/SortByPTypeProcess.cpp +++ b/code/PostProcessing/SortByPTypeProcess.cpp @@ -135,7 +135,9 @@ void SortByPTypeProcess::Execute(aiScene *pScene) { std::vector::iterator meshIdx = replaceMeshIndex.begin(); for (unsigned int i = 0; i < pScene->mNumMeshes; ++i) { aiMesh *const mesh = pScene->mMeshes[i]; - ai_assert(0 != mesh->mPrimitiveTypes); + if (mesh->mPrimitiveTypes == 0) { + throw DeadlyImportError("GLTF: Mesh with invalid primitive type: ", mesh->mName.C_Str()); + } // if there's just one primitive type in the mesh there's nothing to do for us unsigned int num = 0; From 44dc08f128c97667104a5dc6a85bf1ea645ce1b5 Mon Sep 17 00:00:00 2001 From: "Max Vollmer (Microsoft Havok)" <60260460+ms-maxvollmer@users.noreply.github.com> Date: Wed, 21 Apr 2021 16:20:58 +0100 Subject: [PATCH 2/3] Remove GLTF tag, postprocessing is format independent --- code/PostProcessing/SortByPTypeProcess.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/PostProcessing/SortByPTypeProcess.cpp b/code/PostProcessing/SortByPTypeProcess.cpp index 38549b9a0..20ab63249 100644 --- a/code/PostProcessing/SortByPTypeProcess.cpp +++ b/code/PostProcessing/SortByPTypeProcess.cpp @@ -136,7 +136,7 @@ void SortByPTypeProcess::Execute(aiScene *pScene) { for (unsigned int i = 0; i < pScene->mNumMeshes; ++i) { aiMesh *const mesh = pScene->mMeshes[i]; if (mesh->mPrimitiveTypes == 0) { - throw DeadlyImportError("GLTF: Mesh with invalid primitive type: ", mesh->mName.C_Str()); + throw DeadlyImportError("Mesh with invalid primitive type: ", mesh->mName.C_Str()); } // if there's just one primitive type in the mesh there's nothing to do for us From 6abdd0cd3e5f199f0c3bd5f4b08eb3c296edce81 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Wed, 28 Apr 2021 16:38:22 +0200 Subject: [PATCH 3/3] Fix crash when reading 0 bytes - This is a valid option so crash shall not happen --- code/Common/DefaultIOStream.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/code/Common/DefaultIOStream.cpp b/code/Common/DefaultIOStream.cpp index 115fea63c..ba9b9d625 100644 --- a/code/Common/DefaultIOStream.cpp +++ b/code/Common/DefaultIOStream.cpp @@ -90,10 +90,12 @@ DefaultIOStream::~DefaultIOStream() { size_t DefaultIOStream::Read(void *pvBuffer, size_t pSize, size_t pCount) { + if (0 == pCount) { + return 0; + } ai_assert(nullptr != pvBuffer); ai_assert(0 != pSize); - ai_assert(0 != pCount); - + return (mFile ? ::fread(pvBuffer, pSize, pCount, mFile) : 0); }