Merge pull request #3329 from MalcolmTyrrell/gltfIndexOutOfRangeFix

Handle Gltf2 files where a value in a mesh index buffer is out of range.

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<aiMesh *> meshes;
+	std::vector<std::unique_ptr<aiMesh>> 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<aiMesh>(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<int>(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<unsigned int>(nFaces);
-				ai_assert(CheckValidFacesIndices(faces, static_cast<unsigned>(nFaces), aim->mNumVertices));
+                const unsigned int actualNumFaces = static_cast<unsigned int>(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) {

include/assimp/BaseImporter.h

@@ -57,6 +57,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #include <map>
 #include <set>
 #include <vector>
+#include <memory>
 
 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 <typename T>
+    AI_FORCE_INLINE static void CopyVector(
+            std::vector<std::unique_ptr<T>> &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<T>& uPtr){*outPtr = uPtr.release(); ++outPtr; });
+        }
+    }
+
 protected:
     /// Error description in case there was one.
     std::string m_ErrorText;

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"
+        }
+    ]
+}

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"
+        }
+    ]
+}

test/unit/utglTF2ImportExport.cpp

@@ -46,11 +46,13 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #include <assimp/scene.h>
 #include <assimp/Exporter.hpp>
 #include <assimp/Importer.hpp>
+#include <assimp/LogStream.hpp>
+#include <assimp/DefaultLogger.hpp>
+
 
 #include <array>
 
 #include <assimp/pbrmaterial.h>
-
 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);
+}