From 51f294c5876b2e41f72cd49561394d003c7c060a Mon Sep 17 00:00:00 2001 From: "Max Vollmer (Microsoft Havok)" <60260460+ms-maxvollmer@users.noreply.github.com> Date: Mon, 30 Aug 2021 14:59:17 +0100 Subject: [PATCH 1/6] Fixes issues our internal compliance and code quality tool found: * Adds nullptr checks and asserts to protect certain code paths * Fixes wrong integer type in a printf call * Adds const to const values * Prevents integer overflow with explicit casts --- code/AssetLib/FBX/FBXConverter.cpp | 4 +++- code/Common/DefaultIOSystem.cpp | 2 +- code/Common/ScenePreprocessor.cpp | 3 +++ code/Material/MaterialSystem.cpp | 12 +++++++++--- code/PostProcessing/OptimizeGraph.cpp | 2 +- code/PostProcessing/PretransformVertices.cpp | 2 +- code/PostProcessing/SortByPTypeProcess.cpp | 2 +- 7 files changed, 19 insertions(+), 8 deletions(-) diff --git a/code/AssetLib/FBX/FBXConverter.cpp b/code/AssetLib/FBX/FBXConverter.cpp index a92745fb6..fa7ee3986 100644 --- a/code/AssetLib/FBX/FBXConverter.cpp +++ b/code/AssetLib/FBX/FBXConverter.cpp @@ -917,8 +917,10 @@ void FBXConverter::ConvertModel(const Model &model, aiNode *parent, aiNode *root } else if (line) { const std::vector &indices = ConvertLine(*line, root_node); std::copy(indices.begin(), indices.end(), std::back_inserter(meshes)); - } else { + } else if (geo) { FBXImporter::LogWarn("ignoring unrecognized geometry: ", geo->Name()); + } else { + FBXImporter::LogWarn("skipping null geometry"); } } diff --git a/code/Common/DefaultIOSystem.cpp b/code/Common/DefaultIOSystem.cpp index 1ed615b84..de93909de 100644 --- a/code/Common/DefaultIOSystem.cpp +++ b/code/Common/DefaultIOSystem.cpp @@ -173,7 +173,7 @@ inline static std::string MakeAbsolutePath(const char *in) { free(ret); } #endif - if (!ret) { + else { // preserve the input path, maybe someone else is able to fix // the path before it is accessed (e.g. our file system filter) ASSIMP_LOG_WARN("Invalid path: ", std::string(in)); diff --git a/code/Common/ScenePreprocessor.cpp b/code/Common/ScenePreprocessor.cpp index 132b32df7..2ea17c643 100644 --- a/code/Common/ScenePreprocessor.cpp +++ b/code/Common/ScenePreprocessor.cpp @@ -89,6 +89,9 @@ void ScenePreprocessor::ProcessScene() { ASSIMP_LOG_DEBUG("ScenePreprocessor: Adding default material \'" AI_DEFAULT_MATERIAL_NAME "\'"); for (unsigned int i = 0; i < scene->mNumMeshes; ++i) { + if (nullptr == scene->mMeshes[i]) { + continue; + } scene->mMeshes[i]->mMaterialIndex = scene->mNumMaterials; } diff --git a/code/Material/MaterialSystem.cpp b/code/Material/MaterialSystem.cpp index c35a1aa93..23d198953 100644 --- a/code/Material/MaterialSystem.cpp +++ b/code/Material/MaterialSystem.cpp @@ -555,17 +555,23 @@ uint32_t Assimp::ComputeMaterialHash(const aiMaterial *mat, bool includeMatName } // ------------------------------------------------------------------------------------------------ -void aiMaterial::CopyPropertyList(aiMaterial *pcDest, +void aiMaterial::CopyPropertyList(aiMaterial *const pcDest, const aiMaterial *pcSrc) { ai_assert(nullptr != pcDest); ai_assert(nullptr != pcSrc); + ai_assert(pcDest->mNumProperties <= pcDest->mNumAllocated); + ai_assert(pcSrc->mNumProperties <= pcSrc->mNumAllocated); - unsigned int iOldNum = pcDest->mNumProperties; + const unsigned int iOldNum = pcDest->mNumProperties; pcDest->mNumAllocated += pcSrc->mNumAllocated; pcDest->mNumProperties += pcSrc->mNumProperties; + const unsigned int numAllocated = pcDest->mNumAllocated; aiMaterialProperty **pcOld = pcDest->mProperties; - pcDest->mProperties = new aiMaterialProperty *[pcDest->mNumAllocated]; + pcDest->mProperties = new aiMaterialProperty *[numAllocated]; + + ai_assert(!iOldNum || pcOld); + ai_assert(iOldNum < numAllocated); if (iOldNum && pcOld) { for (unsigned int i = 0; i < iOldNum; ++i) { diff --git a/code/PostProcessing/OptimizeGraph.cpp b/code/PostProcessing/OptimizeGraph.cpp index e33c2ab18..d7bcf3fec 100644 --- a/code/PostProcessing/OptimizeGraph.cpp +++ b/code/PostProcessing/OptimizeGraph.cpp @@ -170,7 +170,7 @@ void OptimizeGraphProcess::CollectNewChildren(aiNode *nd, std::list &n ++it; } if (join_master && !join.empty()) { - join_master->mName.length = ::ai_snprintf(join_master->mName.data, MAXLEN, "$MergedNode_%i", count_merged++); + join_master->mName.length = ::ai_snprintf(join_master->mName.data, MAXLEN, "$MergedNode_%u", count_merged++); unsigned int out_meshes = 0; for (std::list::const_iterator it = join.cbegin(); it != join.cend(); ++it) { diff --git a/code/PostProcessing/PretransformVertices.cpp b/code/PostProcessing/PretransformVertices.cpp index e9a3af0d2..fa95319ff 100644 --- a/code/PostProcessing/PretransformVertices.cpp +++ b/code/PostProcessing/PretransformVertices.cpp @@ -481,7 +481,7 @@ void PretransformVertices::Execute(aiScene *pScene) { pScene->mMeshes[i]->mNumBones = 0; } } else { - apcOutMeshes.reserve(pScene->mNumMaterials << 1u); + apcOutMeshes.reserve(static_cast(pScene->mNumMaterials) << 1u); std::list aiVFormats; std::vector s(pScene->mNumMeshes, 0); diff --git a/code/PostProcessing/SortByPTypeProcess.cpp b/code/PostProcessing/SortByPTypeProcess.cpp index 20ab63249..3787be51e 100644 --- a/code/PostProcessing/SortByPTypeProcess.cpp +++ b/code/PostProcessing/SortByPTypeProcess.cpp @@ -127,7 +127,7 @@ void SortByPTypeProcess::Execute(aiScene *pScene) { unsigned int aiNumMeshesPerPType[4] = { 0, 0, 0, 0 }; std::vector outMeshes; - outMeshes.reserve(pScene->mNumMeshes << 1u); + outMeshes.reserve(static_cast(pScene->mNumMeshes) << 1u); bool bAnyChanges = false; From 96f0787f51e91b759132ef5ecf631dd530e749f0 Mon Sep 17 00:00:00 2001 From: Doug Roeper Date: Mon, 30 Aug 2021 18:15:37 -0400 Subject: [PATCH 2/6] Fix the -Werror=unused-but-set-parameter warning by removing the skipFirst variable. --- code/AssetLib/XGL/XGLLoader.cpp | 7 ++----- code/AssetLib/XGL/XGLLoader.h | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/code/AssetLib/XGL/XGLLoader.cpp b/code/AssetLib/XGL/XGLLoader.cpp index 20c2c7079..bbfa31829 100644 --- a/code/AssetLib/XGL/XGLLoader.cpp +++ b/code/AssetLib/XGL/XGLLoader.cpp @@ -250,7 +250,7 @@ void XGLImporter::ReadWorld(XmlNode &node, TempScope &scope) { } } - aiNode *const nd = ReadObject(node, scope, true); + aiNode *const nd = ReadObject(node, scope); if (!nd) { ThrowException("failure reading "); } @@ -296,16 +296,13 @@ aiLight *XGLImporter::ReadDirectionalLight(XmlNode &node) { } // ------------------------------------------------------------------------------------------------ -aiNode *XGLImporter::ReadObject(XmlNode &node, TempScope &scope, bool skipFirst/*, const char *closetag */) { +aiNode *XGLImporter::ReadObject(XmlNode &node, TempScope &scope) { aiNode *nd = new aiNode; std::vector children; std::vector meshes; try { for (XmlNode &child : node.children()) { - - skipFirst = false; - const std::string &s = ai_stdStrToLower(child.name()); if (s == "mesh") { const size_t prev = scope.meshes_linear.size(); diff --git a/code/AssetLib/XGL/XGLLoader.h b/code/AssetLib/XGL/XGLLoader.h index f7da4e0a7..a2b224ac9 100644 --- a/code/AssetLib/XGL/XGLLoader.h +++ b/code/AssetLib/XGL/XGLLoader.h @@ -185,7 +185,7 @@ private: void ReadWorld(XmlNode &node, TempScope &scope); void ReadLighting(XmlNode &node, TempScope &scope); aiLight *ReadDirectionalLight(XmlNode &node); - aiNode *ReadObject(XmlNode &node, TempScope &scope, bool skipFirst = false/*, const char *closetag = "object"*/); + aiNode *ReadObject(XmlNode &node, TempScope &scope); bool ReadMesh(XmlNode &node, TempScope &scope); void ReadMaterial(XmlNode &node, TempScope &scope); aiVector2D ReadVec2(XmlNode &node); From 4c86772091dc5a9b0373adf3faabc4a321b92437 Mon Sep 17 00:00:00 2001 From: "Max Vollmer (Microsoft Havok)" <60260460+ms-maxvollmer@users.noreply.github.com> Date: Thu, 2 Sep 2021 08:27:03 +0100 Subject: [PATCH 3/6] Added another nullptr safety check --- code/AssetLib/glTF2/glTF2Importer.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/code/AssetLib/glTF2/glTF2Importer.cpp b/code/AssetLib/glTF2/glTF2Importer.cpp index 08573bce7..ada7aa046 100644 --- a/code/AssetLib/glTF2/glTF2Importer.cpp +++ b/code/AssetLib/glTF2/glTF2Importer.cpp @@ -1337,6 +1337,17 @@ std::unordered_map GatherSamplers(Animation &an } auto& animsampler = anim.samplers[channel.sampler]; + + if (!animsampler.input) { + ASSIMP_LOG_WARN("Animation ", anim.name, ": Missing sampler input. Skipping."); + continue; + } + + if (!animsampler.output) { + ASSIMP_LOG_WARN("Animation ", anim.name, ": Missing sampler output. Skipping."); + continue; + } + if (animsampler.input->count > animsampler.output->count) { ASSIMP_LOG_WARN("Animation ", anim.name, ": Number of keyframes in sampler input ", animsampler.input->count, " exceeds number of keyframes in sampler output ", animsampler.output->count); continue; From d710d0700fbfaf7c20153e4d89aa1eccc2aa232e Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Thu, 2 Sep 2021 10:10:42 +0200 Subject: [PATCH 4/6] Make nullptr test more explicit. --- code/AssetLib/glTF2/glTF2Importer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/code/AssetLib/glTF2/glTF2Importer.cpp b/code/AssetLib/glTF2/glTF2Importer.cpp index ada7aa046..2786614f3 100644 --- a/code/AssetLib/glTF2/glTF2Importer.cpp +++ b/code/AssetLib/glTF2/glTF2Importer.cpp @@ -1338,12 +1338,12 @@ std::unordered_map GatherSamplers(Animation &an auto& animsampler = anim.samplers[channel.sampler]; - if (!animsampler.input) { + if (nullptr == animsampler.input) { ASSIMP_LOG_WARN("Animation ", anim.name, ": Missing sampler input. Skipping."); continue; } - if (!animsampler.output) { + if (nullptr == animsampler.output) { ASSIMP_LOG_WARN("Animation ", anim.name, ": Missing sampler output. Skipping."); continue; } From 72ea80b41f7364ffacd5878ce4c357aaef0dd92d Mon Sep 17 00:00:00 2001 From: "Max Vollmer (Microsoft Havok)" <60260460+ms-maxvollmer@users.noreply.github.com> Date: Thu, 2 Sep 2021 10:00:56 +0100 Subject: [PATCH 5/6] Revert last change (gltf2::Ref type is not a pointer and has a bool() operator) --- code/AssetLib/glTF2/glTF2Importer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/code/AssetLib/glTF2/glTF2Importer.cpp b/code/AssetLib/glTF2/glTF2Importer.cpp index 2786614f3..aa48b7330 100644 --- a/code/AssetLib/glTF2/glTF2Importer.cpp +++ b/code/AssetLib/glTF2/glTF2Importer.cpp @@ -1338,12 +1338,12 @@ std::unordered_map GatherSamplers(Animation &an auto& animsampler = anim.samplers[channel.sampler]; - if (nullptr == animsampler.input) { + if (animsampler.input) { ASSIMP_LOG_WARN("Animation ", anim.name, ": Missing sampler input. Skipping."); continue; } - if (nullptr == animsampler.output) { + if (animsampler.output) { ASSIMP_LOG_WARN("Animation ", anim.name, ": Missing sampler output. Skipping."); continue; } From bf8e36ae28247bbb36a243215945748366be4f4d Mon Sep 17 00:00:00 2001 From: "Max Vollmer (Microsoft Havok)" <60260460+ms-maxvollmer@users.noreply.github.com> Date: Thu, 2 Sep 2021 10:07:28 +0100 Subject: [PATCH 6/6] Fixed typo --- code/AssetLib/glTF2/glTF2Importer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/code/AssetLib/glTF2/glTF2Importer.cpp b/code/AssetLib/glTF2/glTF2Importer.cpp index aa48b7330..ada7aa046 100644 --- a/code/AssetLib/glTF2/glTF2Importer.cpp +++ b/code/AssetLib/glTF2/glTF2Importer.cpp @@ -1338,12 +1338,12 @@ std::unordered_map GatherSamplers(Animation &an auto& animsampler = anim.samplers[channel.sampler]; - if (animsampler.input) { + if (!animsampler.input) { ASSIMP_LOG_WARN("Animation ", anim.name, ": Missing sampler input. Skipping."); continue; } - if (animsampler.output) { + if (!animsampler.output) { ASSIMP_LOG_WARN("Animation ", anim.name, ": Missing sampler output. Skipping."); continue; }