Avoid out-of-bounds access when checking file extensions.

Include the '.' in the file extension check.

PiperOrigin-RevId: 474866478
Change-Id: I809ae975e4c8b00f17278735975a7d07e0748643

src/user/user_mesh.cc

@@ -160,12 +160,12 @@ void mjCMesh::Compile(const mjVFS* vfs) {
     }
 
     // load STL, OBJ or MSH
-    string ext = file.substr(file.size()-3, 3);
+    string ext = mjuu_getext(file);
-    if (!strcasecmp(ext.c_str(), "stl")) {
+    if (!strcasecmp(ext.c_str(), ".stl")) {
       LoadSTL(vfs);
-    } else if (!strcasecmp(ext.c_str(), "obj")) {
+    } else if (!strcasecmp(ext.c_str(), ".obj")) {
       LoadOBJ(vfs);
-    } else if (!strcasecmp(ext.c_str(), "msh")) {
+    } else if (!strcasecmp(ext.c_str(), ".msh")) {
       LoadMSH(vfs);
     } else {
       throw mjCError(this, "Unknown mesh file type: %s", file.c_str());
@@ -1621,8 +1621,8 @@ void mjCSkin::Compile(const mjVFS* vfs) {
     }
 
     // load SKN
-    string ext = file.substr(file.size()-3, 3);
+    string ext = mjuu_getext(file);
-    if (!strcasecmp(ext.c_str(), "skn")) {
+    if (!strcasecmp(ext.c_str(), ".skn")) {
       LoadSKN(vfs);
     } else {
       throw mjCError(this, "Unknown skin file type: %s", file.c_str());

src/user/user_objects.cc

@@ -1879,7 +1879,8 @@ void mjCHField::Compile(const mjVFS* vfs) {
     string filename = mjuu_makefullname(model->modelfiledir, model->meshdir, file);
 
     // load depending on format
-    if (!strcasecmp(filename.substr(filename.length()-4, 5).c_str(), ".png")) {
+    string ext = mjuu_getext(filename);
+    if (!strcasecmp(ext.c_str(), ".png")) {
       LoadPNG(filename, vfs);
     } else {
       LoadCustom(filename, vfs);
@@ -2301,7 +2302,8 @@ void mjCTexture::LoadFlip(string filename, const mjVFS* vfs,
                           std::vector<unsigned char>& image,
                           unsigned int& w, unsigned int& h) {
   // dispatch to PNG or Custom loaded
-  if (!strcasecmp(filename.substr(filename.length()-4, 5).c_str(), ".png")) {
+  string ext = mjuu_getext(filename);
+  if (!strcasecmp(ext.c_str(), ".png")) {
     LoadPNG(filename, vfs, image, w, h);
   } else {
     LoadCustom(filename, vfs, image, w, h);

src/user/user_util.cc

@@ -19,6 +19,7 @@
 #include <cstdio>
 #include <limits>
 #include <string>
+#include <string_view>
 
 #include <mujoco/mjtnum.h>
 #include "engine/engine_macro.h"
@@ -488,9 +489,16 @@ string mjuu_stripext(string filename) {
   }
 
   // return name without extension
-  else {
+  return filename.substr(0, end);
-    return filename.substr(0, end);
+}
+
+string mjuu_getext(std::string_view filename) {
+  size_t dot = filename.find_last_of('.');
+
+  if (dot==string::npos) {
+    return "";
   }
+  return string(filename.substr(dot, filename.size() - dot));
 }
 
 

src/user/user_util.h

@@ -16,6 +16,7 @@
 #define MUJOCO_SRC_USER_USER_UTIL_H_
 
 #include <string>
+#include <string_view>
 
 
 extern const double mjNAN;                      // used to mark undefined fields
@@ -129,6 +130,9 @@ std::string mjuu_strippath(std::string filename);
 // strip extension from filename
 std::string mjuu_stripext(std::string filename);
 
+// get the extension of a filename
+std::string mjuu_getext(std::string_view filename);
+
 // check if path is absolute
 bool mjuu_isabspath(std::string path);
 

test/user/CMakeLists.txt

@@ -19,4 +19,4 @@ mujoco_test(user_objects_test)
 target_link_libraries(user_objects_test fixture gmock)
 
 mujoco_test(user_mesh_test)
-target_link_libraries(user_mesh_test fixture gmock)
+target_link_libraries(user_mesh_test fixture gmock absl::str_format)

test/user/testdata/png_hfield.xml

@@ -0,0 +1,9 @@
+<mujoco>
+  <asset>
+    <hfield name="hfield" file="hfield.png" size="0.5 0.5 1 0.1"/>
+  </asset>
+
+  <worldbody>
+    <geom type="hfield" hfield="hfield" pos="-.4 .6 .05"/>
+  </worldbody>
+</mujoco>

test/user/user_mesh_test.cc

@@ -21,6 +21,7 @@
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
+#include <absl/strings/str_format.h>
 #include <mujoco/mjmodel.h>
 #include <mujoco/mjtnum.h>
 #include <mujoco/mujoco.h>
@@ -54,6 +55,38 @@ static const char* const kMalformedFaceOBJPath =
 
 using ::testing::HasSubstr;
 
+// ------------- test invalid filenames ----------------------------------------
+
+TEST_F(MjCMeshTest, UnknownMeshFormat) {
+  static constexpr char xml_format[] = R"(
+    <mujoco>
+      <asset>
+        <mesh name="m" file="%s"/>
+      </asset>
+      <worldbody>
+        <geom type="mesh" mesh="m"/>
+      </worldbody>
+    </mujoco>
+  )";
+  std::vector<std::string> invalid_names = {
+    "noextension",
+    "anobj",
+    "f",
+    "mesh.exe",
+    "file%s"
+  };
+  for (const auto& name : invalid_names) {
+    std::string xml = absl::StrFormat(xml_format, name);
+    std::array<char, 1024> error;
+    mjModel* model =
+        LoadModelFromString(xml.c_str(), error.data(), error.size());
+    ASSERT_THAT(model, testing::IsNull())
+        << "Should fail to load a mesh named: " << name;
+    EXPECT_THAT(error.data(), HasSubstr("Unknown mesh file type"));
+    EXPECT_THAT(error.data(), HasSubstr(name));
+  }
+}
+
 // ------------- test vertex de-duplication (STL) ------------------------------
 
 TEST_F(MjCMeshTest, DeDuplicateSTLVertices) {
@@ -124,7 +157,7 @@ TEST_F(MjCMeshTest, SaveMeshOnce) {
   mj_deleteModel(model);
 }
 
-TEST_F(MujocoTest, TinyMeshLoads) {
+TEST_F(MjCMeshTest, TinyMeshLoads) {
   static constexpr char xml[] = R"(
   <mujoco>
     <asset>
@@ -142,7 +175,7 @@ TEST_F(MujocoTest, TinyMeshLoads) {
 
 // ------------- test inertia -------------------------------------------------
 
-TEST_F(MujocoTest, SmallInertiaLoads) {
+TEST_F(MjCMeshTest, SmallInertiaLoads) {
   static constexpr char xml[] = R"(
   <mujoco>
     <asset>
@@ -162,7 +195,7 @@ TEST_F(MujocoTest, SmallInertiaLoads) {
   mj_deleteModel(model);
 }
 
-TEST_F(MujocoTest, TinyInertiaFails) {
+TEST_F(MjCMeshTest, TinyInertiaFails) {
   static constexpr char xml[] = R"(
   <mujoco>
     <asset>
@@ -184,7 +217,7 @@ TEST_F(MujocoTest, TinyInertiaFails) {
           "mass and inertia of moving bodies must be larger than mjMINVAL"));
 }
 
-TEST_F(MujocoTest, MalformedFaceFails) {
+TEST_F(MjCMeshTest, MalformedFaceFails) {
   const std::string xml_path = GetTestDataFilePath(kMalformedFaceOBJPath);
   std::array<char, 1024> error;
   mjModel* model = mj_loadXML(xml_path.c_str(), 0, error.data(), error.size());
@@ -192,7 +225,7 @@ TEST_F(MujocoTest, MalformedFaceFails) {
   EXPECT_THAT(error.data(), HasSubstr("faces have inconsistent orientation"));
 }
 
-TEST_F(MujocoTest, FlippedFaceFails) {
+TEST_F(MjCMeshTest, FlippedFaceFails) {
   static constexpr char xml[] = R"(
   <mujoco>
     <asset>
@@ -223,7 +256,7 @@ void CheckTetrahedronWasRescaled(mjModel* model) {
   }
 }
 
-TEST_F(MujocoTest, FlippedFaceAllowedWorld) {
+TEST_F(MjCMeshTest, FlippedFaceAllowedWorld) {
   static constexpr char xml[] = R"(
   <mujoco>
     <asset>
@@ -243,7 +276,7 @@ TEST_F(MujocoTest, FlippedFaceAllowedWorld) {
   mj_deleteModel(model);
 }
 
-TEST_F(MujocoTest, FlippedFaceAllowedNoMass) {
+TEST_F(MjCMeshTest, FlippedFaceAllowedNoMass) {
   static constexpr char xml[] = R"(
   <mujoco>
     <asset>
@@ -265,7 +298,7 @@ TEST_F(MujocoTest, FlippedFaceAllowedNoMass) {
   mj_deleteModel(model);
 }
 
-TEST_F(MujocoTest, FlippedFaceAllowedInertial) {
+TEST_F(MjCMeshTest, FlippedFaceAllowedInertial) {
   static constexpr char xml[] = R"(
   <mujoco>
     <asset>
@@ -288,7 +321,7 @@ TEST_F(MujocoTest, FlippedFaceAllowedInertial) {
   mj_deleteModel(model);
 }
 
-TEST_F(MujocoTest, FlippedFaceAllowedNegligibleArea) {
+TEST_F(MjCMeshTest, FlippedFaceAllowedNegligibleArea) {
   static constexpr char xml[] = R"(
   <mujoco>
     <asset>
@@ -310,7 +343,7 @@ TEST_F(MujocoTest, FlippedFaceAllowedNegligibleArea) {
   mj_deleteModel(model);
 }
 
-TEST_F(MujocoTest, AreaTooSmall) {
+TEST_F(MjCMeshTest, AreaTooSmall) {
   static constexpr char xml[] = R"(
   <mujoco>
     <asset>
@@ -331,7 +364,7 @@ TEST_F(MujocoTest, AreaTooSmall) {
   EXPECT_THAT(error.data(), HasSubstr("mesh surface area is too small"));
 }
 
-TEST_F(MujocoTest, AreaTooSmallAllowedWorld) {
+TEST_F(MjCMeshTest, AreaTooSmallAllowedWorld) {
   static constexpr char xml[] = R"(
   <mujoco>
     <asset>
@@ -350,7 +383,7 @@ TEST_F(MujocoTest, AreaTooSmallAllowedWorld) {
   mj_deleteModel(model);
 }
 
-TEST_F(MujocoTest, VolumeTooSmall) {
+TEST_F(MjCMeshTest, VolumeTooSmall) {
   static constexpr char xml[] = R"(
   <mujoco>
     <asset>
@@ -371,7 +404,7 @@ TEST_F(MujocoTest, VolumeTooSmall) {
   EXPECT_THAT(error.data(), HasSubstr("mesh volume is too small"));
 }
 
-  TEST_F(MujocoTest, VolumeTooSmallAllowedWorld) {
+  TEST_F(MjCMeshTest, VolumeTooSmallAllowedWorld) {
   static constexpr char xml[] = R"(
   <mujoco>
     <asset>
@@ -394,7 +427,7 @@ TEST_F(MujocoTest, VolumeTooSmall) {
 
 const mjtNum max_abs_err = std::numeric_limits<float>::epsilon();
 
-TEST_F(MujocoTest, ExactConcaveInertia) {
+TEST_F(MjCMeshTest, ExactConcaveInertia) {
   const std::string xml_path = GetTestDataFilePath(kConcaveInertiaPath);
   std::array<char, 1024> error;
   mjModel* model = mj_loadXML(xml_path.c_str(), 0, error.data(), error.size());
@@ -422,7 +455,7 @@ TEST_F(MujocoTest, ExactConcaveInertia) {
   mj_deleteModel(model);
 }
 
-TEST_F(MujocoTest, ExactConvexInertia) {
+TEST_F(MjCMeshTest, ExactConvexInertia) {
   const std::string xml_path = GetTestDataFilePath(kConvexInertiaPath);
   std::array<char, 1024> error;
   mjModel* model = mj_loadXML(xml_path.c_str(), 0, error.data(), error.size());
@@ -437,7 +470,7 @@ TEST_F(MujocoTest, ExactConvexInertia) {
   mj_deleteModel(model);
 }
 
-TEST_F(MujocoTest, ExactShellInertia) {
+TEST_F(MjCMeshTest, ExactShellInertia) {
   const std::string xml_path = GetTestDataFilePath(kShellInertiaPath);
   std::array<char, 1024> error;
   mjModel* model = mj_loadXML(xml_path.c_str(), 0, error.data(), error.size());

test/user/user_objects_test.cc

@@ -265,6 +265,22 @@ TEST_F(MjCGeomTest, CapsuleInertiaX) {
   mj_deleteModel(model);
 }
 
+// ------------- test height fields --------------------------------------------
+
+using MjCHFieldTest = MujocoTest;
+
+TEST_F(MjCHFieldTest, PngMap) {
+  const std::string xml_path =
+      GetTestDataFilePath("user/testdata/png_hfield.xml");
+  std::array<char, 1024> error;
+  mjModel* model =
+      mj_loadXML(xml_path.c_str(), nullptr, error.data(), error.size());
+  ASSERT_THAT(model, ::testing::NotNull()) << error.data();
+  EXPECT_EQ(model->nhfield, 1);
+  EXPECT_EQ(model->geom_type[0], mjGEOM_HFIELD);
+  mj_deleteModel(model);
+}
+
 // ------------- test quaternion normalization----------------------------------
 
 using QuatNorm = MujocoTest;