2898: Don't crash when ASE file is missing UV data (TrenchBroom#2967)

* 2898: Add test case (this does not always fail, it will just access garbage)

* 2898: Check vertex and UV indices and report out of bounds errors

* 2898: Add test case for missing UVs

* 2898: Set UV to zero if ASE model does not contain UV data at all

common/src/IO/AseParser.cpp

@@ -260,6 +260,8 @@ namespace TrenchBroom {
             expectDirective("MESH_FACE");
             expectSizeArgument(faces.size());
 
+            const std::size_t line = m_tokenizer.line();
+
             // the colon after the face index is sometimes missing
             m_tokenizer.skipToken(AseToken::Colon);
 
@@ -293,7 +295,7 @@ namespace TrenchBroom {
                 MeshFaceVertex{ vertexIndexA, 0 },
                 MeshFaceVertex{ vertexIndexB, 0 },
                 MeshFaceVertex{ vertexIndexC, 0 }
-            }});
+            }, line });
         }
 
         void AseParser::parseGeomObjectMeshNumTVertex(Logger& /* logger */, std::vector<vm::vec2f>& uv) {
@@ -334,7 +336,7 @@ namespace TrenchBroom {
             }
 
             for (size_t i = 0; i < 3; ++i) {
-                faces[index][i].uvIndex = parseSizeArgument();
+                faces[index].vertices[i].uvIndex = parseSizeArgument();
             }
         }
 
@@ -446,7 +448,7 @@ namespace TrenchBroom {
             result[AseToken::Eof]          = "end of file";
             return result;
         }
-
+    
         std::unique_ptr<Assets::EntityModel> AseParser::buildModel(Logger& logger, const Scene& scene) const {
             using Vertex = Assets::EntityModelVertex;
 
@@ -500,11 +502,27 @@ namespace TrenchBroom {
                 auto* texture = textureIndex < textures.size() ? textures[textureIndex] : nullptr;
 
                 for (const auto& face : mesh.faces) {
+                    if (!checkIndices(logger, face, mesh)) {
+                        continue;
+                    }
+
+                    const auto& fv0 = face.vertices[0];
+                    const auto& fv1 = face.vertices[1];
+                    const auto& fv2 = face.vertices[2];
+
+                    const auto v0 = mesh.vertices[fv0.vertexIndex];
+                    const auto v1 = mesh.vertices[fv1.vertexIndex];
+                    const auto v2 = mesh.vertices[fv2.vertexIndex];
+
+                    const auto uv0 = fv0.uvIndex == 0u && mesh.uv.empty() ? vm::vec2f::zero() : mesh.uv[fv0.uvIndex];
+                    const auto uv1 = fv1.uvIndex == 0u && mesh.uv.empty() ? vm::vec2f::zero() : mesh.uv[fv1.uvIndex];
+                    const auto uv2 = fv2.uvIndex == 0u && mesh.uv.empty() ? vm::vec2f::zero() : mesh.uv[fv2.uvIndex];
+
                     builder.addTriangle(
                         texture,
-                        Vertex(mesh.vertices[face[2].vertexIndex], mesh.uv[face[2].uvIndex]),
-                        Vertex(mesh.vertices[face[1].vertexIndex], mesh.uv[face[1].uvIndex]),
-                        Vertex(mesh.vertices[face[0].vertexIndex], mesh.uv[face[0].uvIndex]));
+                        Vertex(v2, uv2),
+                        Vertex(v1, uv1),
+                        Vertex(v0, uv0));
                 }
 
             }
@@ -513,6 +531,20 @@ namespace TrenchBroom {
             return model;
         }
 
+        bool AseParser::checkIndices(Logger& logger, const MeshFace& face, const Mesh& mesh) const {
+            for (std::size_t i = 0u; i < 3u; ++i) {
+                const auto& faceVertex = face.vertices[i];
+                if (faceVertex.vertexIndex >= mesh.vertices.size()) {
+                    logger.warn() << "Line " << face.line << ": Vertex index " << faceVertex.vertexIndex << " is out of bounds, skipping face";
+                    return false;
+                } else if (!mesh.uv.empty() && faceVertex.uvIndex >= mesh.uv.size()) {
+                    logger.warn() << "Line " << face.line << ": UV index " << faceVertex.uvIndex << " is out of bounds, skipping face";
+                    return false;
+                }
+            }
+            return true;
+        }
+
         std::unique_ptr<Assets::Texture> AseParser::loadTexture(Logger& logger, const Path& path) const {
             const auto actualPath = fixTexturePath(logger, path);
             if (!actualPath.isEmpty()) {

common/src/IO/AseParser.h

@@ -76,8 +76,11 @@ namespace TrenchBroom {
                 size_t uvIndex;
             };
 
-            using MeshFace = std::array<MeshFaceVertex, 3>;
-
+            struct MeshFace {
+                std::array<MeshFaceVertex, 3> vertices;
+                size_t line;
+            };
+
             struct Mesh {
                 std::vector<vm::vec3f> vertices;
                 std::vector<vm::vec2f> uv;
@@ -88,6 +91,7 @@ namespace TrenchBroom {
                 std::string name;
                 Mesh mesh;
                 size_t materialIndex;
+                size_t line;
             };
 
             struct Scene {
@@ -154,6 +158,7 @@ namespace TrenchBroom {
             TokenNameMap tokenNames() const override;
         private: // model construction
             std::unique_ptr<Assets::EntityModel> buildModel(Logger& logger, const Scene& scene) const;
+            bool checkIndices(Logger& logger, const MeshFace& face, const Mesh& mesh) const;
             std::unique_ptr<Assets::Texture> loadTexture(Logger& logger, const Path& path) const;
             Path fixTexturePath(Logger& logger, Path path) const;
         };

common/test/fixture/IO/Ase/index_out_of_bounds/wedge_45.ase

@@ -0,0 +1,88 @@
+*3DSMAX_ASCIIEXPORT	200
+*MATERIAL_LIST	{
+	*MATERIAL_COUNT	2
+	*MATERIAL	0	{
+		*MATERIAL_NAME	"textures/radiant_regression_tests/tile_model"
+		*MATERIAL_CLASS	"Standard"
+		*MATERIAL_DIFFUSE	1.000000	1.000000	0.833333
+		*MATERIAL_SHADING Phong
+		*MAP_DIFFUSE	{
+			*MAP_NAME	"textures/radiant_regression_tests/tile_model"
+			*MAP_CLASS	"Bitmap"
+			*MAP_SUBNO	1
+			*MAP_AMOUNT	1.0
+			*MAP_TYPE	Screen
+			*BITMAP	"..\textures\radiant_regression_tests\tile_model.tga"
+			*BITMAP_FILTER	Pyramidal
+		}
+	}
+	*MATERIAL	1	{
+		*MATERIAL_NAME	"noshader"
+		*MATERIAL_CLASS	"Standard"
+		*MATERIAL_DIFFUSE	1.000000	1.000000	1.000000
+		*MATERIAL_SHADING Phong
+		*MAP_DIFFUSE	{
+			*MAP_NAME	"noshader"
+			*MAP_CLASS	"Bitmap"
+			*MAP_SUBNO	1
+			*MAP_AMOUNT	1.0
+			*MAP_TYPE	Screen
+			*BITMAP	"..\noshader.tga"
+			*BITMAP_FILTER	Pyramidal
+		}
+	}
+}
+*GEOMOBJECT	{
+	*NODE_NAME	"mat0model0surf0"
+	*NODE_TM	{
+		*NODE_NAME	"mat0model0surf0"
+		*INHERIT_POS	0	0	0
+		*INHERIT_ROT	0	0	0
+		*INHERIT_SCL	0	0	0
+		*TM_ROW0	1.0	0	0
+		*TM_ROW1	0	1.0	0
+		*TM_ROW2	0	0	1.0
+		*TM_ROW3	0	0	0
+		*TM_POS	0.000000	0.000000	0.000000
+	}
+	*MESH	{
+		*TIMEVALUE	0
+		*MESH_NUMVERTEX	4
+		*MESH_NUMFACES	2
+		*COMMENT	"SURFACETYPE	MST_PLANAR"
+		*MESH_VERTEX_LIST	{
+			*MESH_VERTEX	0	128.000000	0.000000	0.000000
+			*MESH_VERTEX	1	64.000000	0.000000	64.000000
+			*MESH_VERTEX	2	128.000000	128.000000	0.000000
+			*MESH_VERTEX	3	64.000000	128.000000	64.000000
+		}
+		*MESH_NORMALS	{
+			*MESH_FACENORMAL	0	0.707107	0.000000	0.707107
+			*MESH_FACENORMAL	1	0.707107	0.000000	0.707107
+			*MESH_VERTEXNORMAL	0	0.707107	0.000000	0.707107
+			*MESH_VERTEXNORMAL	1	0.707107	0.000000	0.707107
+			*MESH_VERTEXNORMAL	2	0.707107	0.000000	0.707107
+			*MESH_VERTEXNORMAL	3	0.707107	0.000000	0.707107
+		}
+		*MESH_FACE_LIST	{
+			*MESH_FACE	0	A:	0	B:	2	C:	1	AB:	1	BC:	1	CA:	1	*MESH_SMOOTHING	0	*MESH_MTLID	0
+			*MESH_FACE	1	A:	5	B:	3	C:	1	AB:	1	BC:	1	CA:	1	*MESH_SMOOTHING	0	*MESH_MTLID	0
+		}
+		*MESH_NUMTVERTEX	4
+		*MESH_TVERTLIST	{
+			*MESH_TVERT	0	1.000000	-1.000000	1.000000
+			*MESH_TVERT	1	-1.000000	-1.000000	1.000000
+			*MESH_TVERT	2	1.000000	3.000000	1.000000
+			*MESH_TVERT	3	-1.000000	3.000000	1.000000
+		}
+		*MESH_NUMTVFACES	2
+		*MESH_TFACELIST	{
+			*MESH_TFACE	0	0	2	1
+			*MESH_TFACE	1	2	3	1
+		}
+	}
+	*PROP_MOTIONBLUR	0
+	*PROP_CASTSHADOW	1
+	*PROP_RECVSHADOW	1
+	*MATERIAL_REF	0
+}

common/test/fixture/IO/Ase/index_out_of_bounds/wedge_45_no_uv.ase

@@ -0,0 +1,76 @@
+*3DSMAX_ASCIIEXPORT	200
+*MATERIAL_LIST	{
+	*MATERIAL_COUNT	2
+	*MATERIAL	0	{
+		*MATERIAL_NAME	"textures/radiant_regression_tests/tile_model"
+		*MATERIAL_CLASS	"Standard"
+		*MATERIAL_DIFFUSE	1.000000	1.000000	0.833333
+		*MATERIAL_SHADING Phong
+		*MAP_DIFFUSE	{
+			*MAP_NAME	"textures/radiant_regression_tests/tile_model"
+			*MAP_CLASS	"Bitmap"
+			*MAP_SUBNO	1
+			*MAP_AMOUNT	1.0
+			*MAP_TYPE	Screen
+			*BITMAP	"..\textures\radiant_regression_tests\tile_model.tga"
+			*BITMAP_FILTER	Pyramidal
+		}
+	}
+	*MATERIAL	1	{
+		*MATERIAL_NAME	"noshader"
+		*MATERIAL_CLASS	"Standard"
+		*MATERIAL_DIFFUSE	1.000000	1.000000	1.000000
+		*MATERIAL_SHADING Phong
+		*MAP_DIFFUSE	{
+			*MAP_NAME	"noshader"
+			*MAP_CLASS	"Bitmap"
+			*MAP_SUBNO	1
+			*MAP_AMOUNT	1.0
+			*MAP_TYPE	Screen
+			*BITMAP	"..\noshader.tga"
+			*BITMAP_FILTER	Pyramidal
+		}
+	}
+}
+*GEOMOBJECT	{
+	*NODE_NAME	"mat0model0surf0"
+	*NODE_TM	{
+		*NODE_NAME	"mat0model0surf0"
+		*INHERIT_POS	0	0	0
+		*INHERIT_ROT	0	0	0
+		*INHERIT_SCL	0	0	0
+		*TM_ROW0	1.0	0	0
+		*TM_ROW1	0	1.0	0
+		*TM_ROW2	0	0	1.0
+		*TM_ROW3	0	0	0
+		*TM_POS	0.000000	0.000000	0.000000
+	}
+	*MESH	{
+		*TIMEVALUE	0
+		*MESH_NUMVERTEX	4
+		*MESH_NUMFACES	2
+		*COMMENT	"SURFACETYPE	MST_PLANAR"
+		*MESH_VERTEX_LIST	{
+			*MESH_VERTEX	0	128.000000	0.000000	0.000000
+			*MESH_VERTEX	1	64.000000	0.000000	64.000000
+			*MESH_VERTEX	2	128.000000	128.000000	0.000000
+			*MESH_VERTEX	3	64.000000	128.000000	64.000000
+		}
+		*MESH_NORMALS	{
+			*MESH_FACENORMAL	0	0.707107	0.000000	0.707107
+			*MESH_FACENORMAL	1	0.707107	0.000000	0.707107
+			*MESH_VERTEXNORMAL	0	0.707107	0.000000	0.707107
+			*MESH_VERTEXNORMAL	1	0.707107	0.000000	0.707107
+			*MESH_VERTEXNORMAL	2	0.707107	0.000000	0.707107
+			*MESH_VERTEXNORMAL	3	0.707107	0.000000	0.707107
+		}
+		*MESH_FACE_LIST	{
+			*MESH_FACE	0	A:	0	B:	2	C:	1	AB:	1	BC:	1	CA:	1	*MESH_SMOOTHING	0	*MESH_MTLID	0
+			*MESH_FACE	1	A:	5	B:	3	C:	1	AB:	1	BC:	1	CA:	1	*MESH_SMOOTHING	0	*MESH_MTLID	0
+		}
+	}
+	*PROP_MOTIONBLUR	0
+	*PROP_CASTSHADOW	1
+	*PROP_RECVSHADOW	1
+	*MATERIAL_REF	0
+}

common/test/src/IO/AseParserTest.cpp

@@ -87,6 +87,44 @@ namespace TrenchBroom {
             ASSERT_NO_THROW(parser.loadFrame(0, *model, logger));
             ASSERT_TRUE(model->frame(0)->loaded());
         }
+
+        TEST(AseParserTest, parseFailure_2898_vertex_index) {
+            NullLogger logger;
+            const auto shaderSearchPath = Path("scripts");
+            const auto textureSearchPaths = std::vector<Path> { Path("models") };
+            std::shared_ptr<FileSystem> fs = std::make_shared<DiskFileSystem>(IO::Disk::getCurrentWorkingDir() + Path("fixture/test/IO/Ase/index_out_of_bounds"));
+            fs = std::make_shared<Quake3ShaderFileSystem>(fs, shaderSearchPath, textureSearchPaths, logger);
+
+            const auto aseFile = fs->openFile(Path("wedge_45.ase"));
+            const auto basePath = Path();
+            auto reader = aseFile->reader().buffer();
+            AseParser parser("wedge", std::begin(reader), std::end(reader), *fs);
+
+            auto model = parser.initializeModel(logger);
+            ASSERT_NE(nullptr, model);
+
+            ASSERT_NO_THROW(parser.loadFrame(0, *model, logger));
+            ASSERT_TRUE(model->frame(0)->loaded());
+        }
+
+        TEST(AseParserTest, parseFailure_2898_no_uv) {
+            NullLogger logger;
+            const auto shaderSearchPath = Path("scripts");
+            const auto textureSearchPaths = std::vector<Path> { Path("models") };
+            std::shared_ptr<FileSystem> fs = std::make_shared<DiskFileSystem>(IO::Disk::getCurrentWorkingDir() + Path("fixture/test/IO/Ase/index_out_of_bounds"));
+            fs = std::make_shared<Quake3ShaderFileSystem>(fs, shaderSearchPath, textureSearchPaths, logger);
+
+            const auto aseFile = fs->openFile(Path("wedge_45_no_uv.ase"));
+            const auto basePath = Path();
+            auto reader = aseFile->reader().buffer();
+            AseParser parser("wedge", std::begin(reader), std::end(reader), *fs);
+
+            auto model = parser.initializeModel(logger);
+            ASSERT_NE(nullptr, model);
+
+            ASSERT_NO_THROW(parser.loadFrame(0, *model, logger));
+            ASSERT_TRUE(model->frame(0)->loaded());
+        }
     }
 }