diff --git a/code/AssetLib/glTF2/glTF2Importer.cpp b/code/AssetLib/glTF2/glTF2Importer.cpp index 323f29f8e..00c647ed2 100644 --- a/code/AssetLib/glTF2/glTF2Importer.cpp +++ b/code/AssetLib/glTF2/glTF2Importer.cpp @@ -298,25 +298,37 @@ void glTF2Importer::ImportMaterials(glTF2::Asset &r) { } } -static inline void SetFace(aiFace &face, int a) { - face.mNumIndices = 1; - face.mIndices = new unsigned int[1]; - face.mIndices[0] = a; +static inline void SetFaceAndAdvance1(aiFace*& face, unsigned int numVertices, unsigned int a) { + if (a >= numVertices) { + return; + } + face->mNumIndices = 1; + face->mIndices = new unsigned int[1]; + face->mIndices[0] = a; + ++face; } -static inline void SetFace(aiFace &face, int a, int b) { - face.mNumIndices = 2; - face.mIndices = new unsigned int[2]; - face.mIndices[0] = a; - face.mIndices[1] = b; +static inline void SetFaceAndAdvance2(aiFace*& face, unsigned int numVertices, unsigned int a, unsigned int b) { + if ((a >= numVertices) || (b >= numVertices)) { + return; + } + face->mNumIndices = 2; + face->mIndices = new unsigned int[2]; + face->mIndices[0] = a; + face->mIndices[1] = b; + ++face; } -static inline void SetFace(aiFace &face, int a, int b, int c) { - face.mNumIndices = 3; - face.mIndices = new unsigned int[3]; - face.mIndices[0] = a; - face.mIndices[1] = b; - face.mIndices[2] = c; +static inline void SetFaceAndAdvance3(aiFace*& face, unsigned int numVertices, unsigned int a, unsigned int b, unsigned int c) { + if ((a >= numVertices) || (b >= numVertices) || (c >= numVertices)) { + return; + } + face->mNumIndices = 3; + face->mIndices = new unsigned int[3]; + face->mIndices[0] = a; + face->mIndices[1] = b; + face->mIndices[2] = c; + ++face; } #ifdef ASSIMP_BUILD_DEBUG @@ -335,7 +347,7 @@ static inline bool CheckValidFacesIndices(aiFace *faces, unsigned nFaces, unsign void glTF2Importer::ImportMeshes(glTF2::Asset &r) { ASSIMP_LOG_DEBUG_F("Importing ", r.meshes.Size(), " meshes"); - std::vector meshes; + std::vector> meshes; unsigned int k = 0; meshOffsets.clear(); @@ -350,7 +362,7 @@ void glTF2Importer::ImportMeshes(glTF2::Asset &r) { Mesh::Primitive &prim = mesh.primitives[p]; aiMesh *aim = new aiMesh(); - meshes.push_back(aim); + meshes.push_back(std::unique_ptr(aim)); aim->mName = mesh.name.empty() ? mesh.id : mesh.name; @@ -486,6 +498,7 @@ void glTF2Importer::ImportMeshes(glTF2::Asset &r) { } aiFace *faces = nullptr; + aiFace *facePtr = nullptr; size_t nFaces = 0; if (prim.indices) { @@ -497,9 +510,9 @@ void glTF2Importer::ImportMeshes(glTF2::Asset &r) { switch (prim.mode) { case PrimitiveMode_POINTS: { nFaces = count; - faces = new aiFace[nFaces]; + facePtr = faces = new aiFace[nFaces]; for (unsigned int i = 0; i < count; ++i) { - SetFace(faces[i], data.GetUInt(i)); + SetFaceAndAdvance1(facePtr, aim->mNumVertices, data.GetUInt(i)); } break; } @@ -510,9 +523,9 @@ void glTF2Importer::ImportMeshes(glTF2::Asset &r) { ASSIMP_LOG_WARN("The number of vertices was not compatible with the LINES mode. Some vertices were dropped."); count = nFaces * 2; } - faces = new aiFace[nFaces]; + facePtr = faces = new aiFace[nFaces]; for (unsigned int i = 0; i < count; i += 2) { - SetFace(faces[i / 2], data.GetUInt(i), data.GetUInt(i + 1)); + SetFaceAndAdvance2(facePtr, aim->mNumVertices, data.GetUInt(i), data.GetUInt(i + 1)); } break; } @@ -520,13 +533,13 @@ void glTF2Importer::ImportMeshes(glTF2::Asset &r) { case PrimitiveMode_LINE_LOOP: case PrimitiveMode_LINE_STRIP: { nFaces = count - ((prim.mode == PrimitiveMode_LINE_STRIP) ? 1 : 0); - faces = new aiFace[nFaces]; - SetFace(faces[0], data.GetUInt(0), data.GetUInt(1)); + facePtr = faces = new aiFace[nFaces]; + SetFaceAndAdvance2(facePtr, aim->mNumVertices, data.GetUInt(0), data.GetUInt(1)); for (unsigned int i = 2; i < count; ++i) { - SetFace(faces[i - 1], faces[i - 2].mIndices[1], data.GetUInt(i)); + SetFaceAndAdvance2(facePtr, aim->mNumVertices, data.GetUInt(i - 1), data.GetUInt(i)); } if (prim.mode == PrimitiveMode_LINE_LOOP) { // close the loop - SetFace(faces[count - 1], faces[count - 2].mIndices[1], faces[0].mIndices[0]); + SetFaceAndAdvance2(facePtr, aim->mNumVertices, data.GetUInt(static_cast(count) - 1), faces[0].mIndices[0]); } break; } @@ -537,33 +550,33 @@ void glTF2Importer::ImportMeshes(glTF2::Asset &r) { ASSIMP_LOG_WARN("The number of vertices was not compatible with the TRIANGLES mode. Some vertices were dropped."); count = nFaces * 3; } - faces = new aiFace[nFaces]; + facePtr = faces = new aiFace[nFaces]; for (unsigned int i = 0; i < count; i += 3) { - SetFace(faces[i / 3], data.GetUInt(i), data.GetUInt(i + 1), data.GetUInt(i + 2)); + SetFaceAndAdvance3(facePtr, aim->mNumVertices, data.GetUInt(i), data.GetUInt(i + 1), data.GetUInt(i + 2)); } break; } case PrimitiveMode_TRIANGLE_STRIP: { nFaces = count - 2; - faces = new aiFace[nFaces]; + facePtr = faces = new aiFace[nFaces]; for (unsigned int i = 0; i < nFaces; ++i) { //The ordering is to ensure that the triangles are all drawn with the same orientation if ((i + 1) % 2 == 0) { //For even n, vertices n + 1, n, and n + 2 define triangle n - SetFace(faces[i], data.GetUInt(i + 1), data.GetUInt(i), data.GetUInt(i + 2)); + SetFaceAndAdvance3(facePtr, aim->mNumVertices, data.GetUInt(i + 1), data.GetUInt(i), data.GetUInt(i + 2)); } else { //For odd n, vertices n, n+1, and n+2 define triangle n - SetFace(faces[i], data.GetUInt(i), data.GetUInt(i + 1), data.GetUInt(i + 2)); + SetFaceAndAdvance3(facePtr, aim->mNumVertices, data.GetUInt(i), data.GetUInt(i + 1), data.GetUInt(i + 2)); } } break; } case PrimitiveMode_TRIANGLE_FAN: nFaces = count - 2; - faces = new aiFace[nFaces]; - SetFace(faces[0], data.GetUInt(0), data.GetUInt(1), data.GetUInt(2)); + facePtr = faces = new aiFace[nFaces]; + SetFaceAndAdvance3(facePtr, aim->mNumVertices, data.GetUInt(0), data.GetUInt(1), data.GetUInt(2)); for (unsigned int i = 1; i < nFaces; ++i) { - SetFace(faces[i], faces[0].mIndices[0], faces[i - 1].mIndices[2], data.GetUInt(i + 2)); + SetFaceAndAdvance3(facePtr, aim->mNumVertices, data.GetUInt(0), data.GetUInt(i + 1), data.GetUInt(i + 2)); } break; } @@ -575,9 +588,9 @@ void glTF2Importer::ImportMeshes(glTF2::Asset &r) { switch (prim.mode) { case PrimitiveMode_POINTS: { nFaces = count; - faces = new aiFace[nFaces]; + facePtr = faces = new aiFace[nFaces]; for (unsigned int i = 0; i < count; ++i) { - SetFace(faces[i], i); + SetFaceAndAdvance1(facePtr, aim->mNumVertices, i); } break; } @@ -588,9 +601,9 @@ void glTF2Importer::ImportMeshes(glTF2::Asset &r) { ASSIMP_LOG_WARN("The number of vertices was not compatible with the LINES mode. Some vertices were dropped."); count = (unsigned int)nFaces * 2; } - faces = new aiFace[nFaces]; + facePtr = faces = new aiFace[nFaces]; for (unsigned int i = 0; i < count; i += 2) { - SetFace(faces[i / 2], i, i + 1); + SetFaceAndAdvance2(facePtr, aim->mNumVertices, i, i + 1); } break; } @@ -598,13 +611,13 @@ void glTF2Importer::ImportMeshes(glTF2::Asset &r) { case PrimitiveMode_LINE_LOOP: case PrimitiveMode_LINE_STRIP: { nFaces = count - ((prim.mode == PrimitiveMode_LINE_STRIP) ? 1 : 0); - faces = new aiFace[nFaces]; - SetFace(faces[0], 0, 1); + facePtr = faces = new aiFace[nFaces]; + SetFaceAndAdvance2(facePtr, aim->mNumVertices, 0, 1); for (unsigned int i = 2; i < count; ++i) { - SetFace(faces[i - 1], faces[i - 2].mIndices[1], i); + SetFaceAndAdvance2(facePtr, aim->mNumVertices, i - 1, i); } if (prim.mode == PrimitiveMode_LINE_LOOP) { // close the loop - SetFace(faces[count - 1], faces[count - 2].mIndices[1], faces[0].mIndices[0]); + SetFaceAndAdvance2(facePtr, aim->mNumVertices, count - 1, 0); } break; } @@ -615,42 +628,50 @@ void glTF2Importer::ImportMeshes(glTF2::Asset &r) { ASSIMP_LOG_WARN("The number of vertices was not compatible with the TRIANGLES mode. Some vertices were dropped."); count = (unsigned int)nFaces * 3; } - faces = new aiFace[nFaces]; + facePtr = faces = new aiFace[nFaces]; for (unsigned int i = 0; i < count; i += 3) { - SetFace(faces[i / 3], i, i + 1, i + 2); + SetFaceAndAdvance3(facePtr, aim->mNumVertices, i, i + 1, i + 2); } break; } case PrimitiveMode_TRIANGLE_STRIP: { nFaces = count - 2; - faces = new aiFace[nFaces]; + facePtr = faces = new aiFace[nFaces]; for (unsigned int i = 0; i < nFaces; ++i) { //The ordering is to ensure that the triangles are all drawn with the same orientation if ((i + 1) % 2 == 0) { //For even n, vertices n + 1, n, and n + 2 define triangle n - SetFace(faces[i], i + 1, i, i + 2); + SetFaceAndAdvance3(facePtr, aim->mNumVertices, i + 1, i, i + 2); } else { //For odd n, vertices n, n+1, and n+2 define triangle n - SetFace(faces[i], i, i + 1, i + 2); + SetFaceAndAdvance3(facePtr, aim->mNumVertices, i, i + 1, i + 2); } } break; } case PrimitiveMode_TRIANGLE_FAN: nFaces = count - 2; - faces = new aiFace[nFaces]; - SetFace(faces[0], 0, 1, 2); + facePtr = faces = new aiFace[nFaces]; + SetFaceAndAdvance3(facePtr, aim->mNumVertices, 0, 1, 2); for (unsigned int i = 1; i < nFaces; ++i) { - SetFace(faces[i], faces[0].mIndices[0], faces[i - 1].mIndices[2], i + 2); + SetFaceAndAdvance3(facePtr, aim->mNumVertices, 0, i + 1, i + 2); } break; } } - if (nullptr != faces) { + if (faces) { aim->mFaces = faces; - aim->mNumFaces = static_cast(nFaces); - ai_assert(CheckValidFacesIndices(faces, static_cast(nFaces), aim->mNumVertices)); + const unsigned int actualNumFaces = static_cast(facePtr - faces); + if (actualNumFaces < nFaces) { + ASSIMP_LOG_WARN("Some faces had out-of-range indices. Those faces were dropped."); + } + if (actualNumFaces == 0) + { + throw DeadlyImportError(std::string("Mesh \"") + aim->mName.C_Str() + "\" has no faces"); + } + aim->mNumFaces = actualNumFaces; + ai_assert(CheckValidFacesIndices(faces, actualNumFaces, aim->mNumVertices)); } if (prim.material) { diff --git a/include/assimp/BaseImporter.h b/include/assimp/BaseImporter.h index ed146168d..e6ff2e68d 100644 --- a/include/assimp/BaseImporter.h +++ b/include/assimp/BaseImporter.h @@ -57,6 +57,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #include #include +#include struct aiScene; struct aiImporterDesc; @@ -391,6 +392,24 @@ public: // static utilities } } + // ------------------------------------------------------------------- + /** Utility function to move a std::vector of unique_ptrs into a aiScene array + * @param vec The vector of unique_ptrs to be moved + * @param out The output pointer to the allocated array. + * @param numOut The output count of elements copied. */ + template + AI_FORCE_INLINE static void CopyVector( + std::vector> &vec, + T **&out, + unsigned int &outLength) { + outLength = unsigned(vec.size()); + if (outLength) { + out = new T*[outLength]; + T** outPtr = out; + std::for_each(vec.begin(), vec.end(), [&outPtr](std::unique_ptr& uPtr){*outPtr = uPtr.release(); ++outPtr; }); + } + } + protected: /// Error description in case there was one. std::string m_ErrorText; diff --git a/test/models/glTF2/IndexOutOfRange/AllIndicesOutOfRange.bin b/test/models/glTF2/IndexOutOfRange/AllIndicesOutOfRange.bin new file mode 100644 index 000000000..8fef5c7c0 Binary files /dev/null and b/test/models/glTF2/IndexOutOfRange/AllIndicesOutOfRange.bin differ diff --git a/test/models/glTF2/IndexOutOfRange/AllIndicesOutOfRange.gltf b/test/models/glTF2/IndexOutOfRange/AllIndicesOutOfRange.gltf new file mode 100644 index 000000000..3dbb6efe4 --- /dev/null +++ b/test/models/glTF2/IndexOutOfRange/AllIndicesOutOfRange.gltf @@ -0,0 +1,142 @@ +{ + "asset": { + "generator": "COLLADA2GLTF", + "version": "2.0" + }, + "scene": 0, + "scenes": [ + { + "nodes": [ + 0 + ] + } + ], + "nodes": [ + { + "children": [ + 1 + ], + "matrix": [ + 1.0, + 0.0, + 0.0, + 0.0, + 0.0, + 0.0, + -1.0, + 0.0, + 0.0, + 1.0, + 0.0, + 0.0, + 0.0, + 0.0, + 0.0, + 1.0 + ] + }, + { + "mesh": 0 + } + ], + "meshes": [ + { + "primitives": [ + { + "attributes": { + "NORMAL": 1, + "POSITION": 2 + }, + "indices": 0, + "mode": 4, + "material": 0 + } + ], + "name": "Mesh" + } + ], + "accessors": [ + { + "bufferView": 0, + "byteOffset": 0, + "componentType": 5123, + "count": 36, + "max": [ + 255 + ], + "min": [ + 0 + ], + "type": "SCALAR" + }, + { + "bufferView": 1, + "byteOffset": 0, + "componentType": 5126, + "count": 24, + "max": [ + 1.0, + 1.0, + 1.0 + ], + "min": [ + -1.0, + -1.0, + -1.0 + ], + "type": "VEC3" + }, + { + "bufferView": 1, + "byteOffset": 288, + "componentType": 5126, + "count": 24, + "max": [ + 0.5, + 0.5, + 0.5 + ], + "min": [ + -0.5, + -0.5, + -0.5 + ], + "type": "VEC3" + } + ], + "materials": [ + { + "pbrMetallicRoughness": { + "baseColorFactor": [ + 0.800000011920929, + 0.0, + 0.0, + 1.0 + ], + "metallicFactor": 0.0 + }, + "name": "Red" + } + ], + "bufferViews": [ + { + "buffer": 0, + "byteOffset": 576, + "byteLength": 72, + "target": 34963 + }, + { + "buffer": 0, + "byteOffset": 0, + "byteLength": 576, + "byteStride": 12, + "target": 34962 + } + ], + "buffers": [ + { + "byteLength": 648, + "uri": "AllIndicesOutOfRange.bin" + } + ] +} diff --git a/test/models/glTF2/IndexOutOfRange/IndexOutOfRange.bin b/test/models/glTF2/IndexOutOfRange/IndexOutOfRange.bin new file mode 100644 index 000000000..b7cfe377b Binary files /dev/null and b/test/models/glTF2/IndexOutOfRange/IndexOutOfRange.bin differ diff --git a/test/models/glTF2/IndexOutOfRange/IndexOutOfRange.gltf b/test/models/glTF2/IndexOutOfRange/IndexOutOfRange.gltf new file mode 100644 index 000000000..67561c591 --- /dev/null +++ b/test/models/glTF2/IndexOutOfRange/IndexOutOfRange.gltf @@ -0,0 +1,142 @@ +{ + "asset": { + "generator": "COLLADA2GLTF", + "version": "2.0" + }, + "scene": 0, + "scenes": [ + { + "nodes": [ + 0 + ] + } + ], + "nodes": [ + { + "children": [ + 1 + ], + "matrix": [ + 1.0, + 0.0, + 0.0, + 0.0, + 0.0, + 0.0, + -1.0, + 0.0, + 0.0, + 1.0, + 0.0, + 0.0, + 0.0, + 0.0, + 0.0, + 1.0 + ] + }, + { + "mesh": 0 + } + ], + "meshes": [ + { + "primitives": [ + { + "attributes": { + "NORMAL": 1, + "POSITION": 2 + }, + "indices": 0, + "mode": 4, + "material": 0 + } + ], + "name": "Mesh" + } + ], + "accessors": [ + { + "bufferView": 0, + "byteOffset": 0, + "componentType": 5123, + "count": 36, + "max": [ + 255 + ], + "min": [ + 0 + ], + "type": "SCALAR" + }, + { + "bufferView": 1, + "byteOffset": 0, + "componentType": 5126, + "count": 24, + "max": [ + 1.0, + 1.0, + 1.0 + ], + "min": [ + -1.0, + -1.0, + -1.0 + ], + "type": "VEC3" + }, + { + "bufferView": 1, + "byteOffset": 288, + "componentType": 5126, + "count": 24, + "max": [ + 0.5, + 0.5, + 0.5 + ], + "min": [ + -0.5, + -0.5, + -0.5 + ], + "type": "VEC3" + } + ], + "materials": [ + { + "pbrMetallicRoughness": { + "baseColorFactor": [ + 0.800000011920929, + 0.0, + 0.0, + 1.0 + ], + "metallicFactor": 0.0 + }, + "name": "Red" + } + ], + "bufferViews": [ + { + "buffer": 0, + "byteOffset": 576, + "byteLength": 72, + "target": 34963 + }, + { + "buffer": 0, + "byteOffset": 0, + "byteLength": 576, + "byteStride": 12, + "target": 34962 + } + ], + "buffers": [ + { + "byteLength": 648, + "uri": "IndexOutOfRange.bin" + } + ] +} diff --git a/test/unit/utglTF2ImportExport.cpp b/test/unit/utglTF2ImportExport.cpp index 6791d5f89..c3ad2d994 100644 --- a/test/unit/utglTF2ImportExport.cpp +++ b/test/unit/utglTF2ImportExport.cpp @@ -46,11 +46,13 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #include #include +#include +#include + #include #include - using namespace Assimp; class utglTF2ImportExport : public AbstractImportExportBase { @@ -541,3 +543,36 @@ TEST_F(utglTF2ImportExport, norootnode_issue_3269) { const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/glTF2/issue_3269/texcoord_crash.gltf", aiProcess_ValidateDataStructure); ASSERT_EQ(scene, nullptr); } + +TEST_F(utglTF2ImportExport, indexOutOfRange) { + // The contents of an asset should not lead to an assert. + Assimp::Importer importer; + + struct LogObserver : Assimp::LogStream + { + bool m_observedWarning = false; + void write(const char *message) override + { + m_observedWarning = m_observedWarning || std::strstr(message, "faces were dropped"); + } + }; + LogObserver logObserver; + + DefaultLogger::get()->attachStream(&logObserver); + const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/glTF2/IndexOutOfRange/IndexOutOfRange.gltf", aiProcess_ValidateDataStructure); + ASSERT_NE(scene, nullptr); + ASSERT_NE(scene->mRootNode, nullptr); + ASSERT_EQ(scene->mNumMeshes, 1); + EXPECT_EQ(scene->mMeshes[0]->mNumFaces, 11); + DefaultLogger::get()->detachStream(&logObserver); + EXPECT_TRUE(logObserver.m_observedWarning); +} + +TEST_F(utglTF2ImportExport, allIndicesOutOfRange) { + // The contents of an asset should not lead to an assert. + Assimp::Importer importer; + const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/glTF2/IndexOutOfRange/AllIndicesOutOfRange.gltf", aiProcess_ValidateDataStructure); + ASSERT_EQ(scene, nullptr); + std::string error = importer.GetErrorString(); + ASSERT_NE(error.find("Mesh \"Mesh\" has no faces"), std::string::npos); +}