From f556d4d94fd04337d32e194ad4f1a73b8f09ee05 Mon Sep 17 00:00:00 2001 From: Nimrod Gileadi Date: Fri, 16 Sep 2022 11:48:49 -0700 Subject: [PATCH] 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 | 12 +++--- src/user/user_objects.cc | 6 ++- src/user/user_util.cc | 12 +++++- src/user/user_util.h | 4 ++ test/user/CMakeLists.txt | 2 +- test/user/testdata/hfield.png | Bin 0 -> 1117 bytes test/user/testdata/png_hfield.xml | 9 +++++ test/user/user_mesh_test.cc | 65 ++++++++++++++++++++++-------- test/user/user_objects_test.cc | 16 ++++++++ 9 files changed, 99 insertions(+), 27 deletions(-) create mode 100644 test/user/testdata/hfield.png create mode 100644 test/user/testdata/png_hfield.xml diff --git a/src/user/user_mesh.cc b/src/user/user_mesh.cc index a5991241..8c48a303 100644 --- a/src/user/user_mesh.cc +++ b/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); - if (!strcasecmp(ext.c_str(), "stl")) { + string ext = mjuu_getext(file); + 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); - if (!strcasecmp(ext.c_str(), "skn")) { + string ext = mjuu_getext(file); + if (!strcasecmp(ext.c_str(), ".skn")) { LoadSKN(vfs); } else { throw mjCError(this, "Unknown skin file type: %s", file.c_str()); diff --git a/src/user/user_objects.cc b/src/user/user_objects.cc index 69880e61..af7c2d4c 100644 --- a/src/user/user_objects.cc +++ b/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& 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); diff --git a/src/user/user_util.cc b/src/user/user_util.cc index 95f8a375..2067396f 100644 --- a/src/user/user_util.cc +++ b/src/user/user_util.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include #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)); } diff --git a/src/user/user_util.h b/src/user/user_util.h index b5cf8341..745923e1 100644 --- a/src/user/user_util.h +++ b/src/user/user_util.h @@ -16,6 +16,7 @@ #define MUJOCO_SRC_USER_USER_UTIL_H_ #include +#include 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); diff --git a/test/user/CMakeLists.txt b/test/user/CMakeLists.txt index d78b32d9..70a7b344 100644 --- a/test/user/CMakeLists.txt +++ b/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) diff --git a/test/user/testdata/hfield.png b/test/user/testdata/hfield.png new file mode 100644 index 0000000000000000000000000000000000000000..22fcde5eaed42e9281756aacfdede509ac4933d1 GIT binary patch literal 1117 zcmV-j1fu(iP)A|0D+qips-3?RqE0fIO-C63Mik{GT5Og|xp0|G0L7vN<38Ti>T zCo|^!>iHu${`%qqz(fEbLbc&#suc+Mv)9<-(%kh6nBa41ewxjGZo&;z;vkfPFk;LC z79@c}Zj3EoUd%;04QD@>g!s^a05anSjPj&XK!jsMh>0tgAe~I`&9mbWAG#c&7Y?d4 ze^YKAAMST#T?f&}9b*Opxv@ux$h#W%7V~bAifehvmundVJ@QX&h6oiCp0Jq!N^>_X zk%ss2p?;Bf5b2nVs3wg8B|sW6b?hN6WRh_UfRzSuVfu9tV60Uj`aITPh;VB5e*%7P zT-%UF$ymT5B%uIP7JyQ5MrTUXj#Y+Oh+zsifNK2!I^EygQ8sA-<@qnL=bN8?Ai#q? zE5|59U5GgUjf0=C`sOpce|Ue+a_(nSkWP)V=jF;aRO|bkf%#cGU7|E;Yj<=)#Y7lA zXP^z`jjBw8Vu4uYw_e$XM(dcfm7mloH%r*j2^9mGR6=grY@a~2-eNNyyxdeD@bf7d zeW!Z{)p`>NG3|`rNP&?CgMpoTo7qDaSi40j;5AS4zsr6%1p{1&S!eVSB2=w7R5cY} zvvtJm<&7N?)dZwdBf|N#Y`^Q3Ew)FaeT0NKplQZKU?LevrxGmJJpV1ZhKh^-2BG>u zMgMUZI;UrRU9AC~?zv)sfDyd}5E#^R1`14MfB-0l%9Sr74ZOxn#aRL1yXJ(k#)O!W zc25h~DZ-F}5ecAkdX9PnO@jbXoMELGcDvQr?@@#SV?hcrpXSsX?c1FKDnQiLxUd2g z*`ciSl_yIwda8ch#2W2GxyRjNUO(oElL3XkfDV-=6_gYw-=s{%Mw`-uI|4x6I8`~& za10BPIy7K2K*bk!wPyQR2upuSDZkwUJXY5>tyjCv4>eVa3wdjg$TGzA@6`6~`XvE0 z+lNAQJbFgILGNkzm#_n*C$3u2(|wF8;1@pASx3VM6^eq<9?}mug+lFwK=s?6=mmq| z=cXJQaGdKIK|f0YD + + + + + + + + \ No newline at end of file diff --git a/test/user/user_mesh_test.cc b/test/user/user_mesh_test.cc index ef98956b..a1b59915 100644 --- a/test/user/user_mesh_test.cc +++ b/test/user/user_mesh_test.cc @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -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"( + + + + + + + + + )"; + std::vector 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 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"( @@ -142,7 +175,7 @@ TEST_F(MujocoTest, TinyMeshLoads) { // ------------- test inertia ------------------------------------------------- -TEST_F(MujocoTest, SmallInertiaLoads) { +TEST_F(MjCMeshTest, SmallInertiaLoads) { static constexpr char xml[] = R"( @@ -162,7 +195,7 @@ TEST_F(MujocoTest, SmallInertiaLoads) { mj_deleteModel(model); } -TEST_F(MujocoTest, TinyInertiaFails) { +TEST_F(MjCMeshTest, TinyInertiaFails) { static constexpr char xml[] = R"( @@ -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 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"( @@ -223,7 +256,7 @@ void CheckTetrahedronWasRescaled(mjModel* model) { } } -TEST_F(MujocoTest, FlippedFaceAllowedWorld) { +TEST_F(MjCMeshTest, FlippedFaceAllowedWorld) { static constexpr char xml[] = R"( @@ -243,7 +276,7 @@ TEST_F(MujocoTest, FlippedFaceAllowedWorld) { mj_deleteModel(model); } -TEST_F(MujocoTest, FlippedFaceAllowedNoMass) { +TEST_F(MjCMeshTest, FlippedFaceAllowedNoMass) { static constexpr char xml[] = R"( @@ -265,7 +298,7 @@ TEST_F(MujocoTest, FlippedFaceAllowedNoMass) { mj_deleteModel(model); } -TEST_F(MujocoTest, FlippedFaceAllowedInertial) { +TEST_F(MjCMeshTest, FlippedFaceAllowedInertial) { static constexpr char xml[] = R"( @@ -288,7 +321,7 @@ TEST_F(MujocoTest, FlippedFaceAllowedInertial) { mj_deleteModel(model); } -TEST_F(MujocoTest, FlippedFaceAllowedNegligibleArea) { +TEST_F(MjCMeshTest, FlippedFaceAllowedNegligibleArea) { static constexpr char xml[] = R"( @@ -310,7 +343,7 @@ TEST_F(MujocoTest, FlippedFaceAllowedNegligibleArea) { mj_deleteModel(model); } -TEST_F(MujocoTest, AreaTooSmall) { +TEST_F(MjCMeshTest, AreaTooSmall) { static constexpr char xml[] = R"( @@ -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"( @@ -350,7 +383,7 @@ TEST_F(MujocoTest, AreaTooSmallAllowedWorld) { mj_deleteModel(model); } -TEST_F(MujocoTest, VolumeTooSmall) { +TEST_F(MjCMeshTest, VolumeTooSmall) { static constexpr char xml[] = R"( @@ -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"( @@ -394,7 +427,7 @@ TEST_F(MujocoTest, VolumeTooSmall) { const mjtNum max_abs_err = std::numeric_limits::epsilon(); -TEST_F(MujocoTest, ExactConcaveInertia) { +TEST_F(MjCMeshTest, ExactConcaveInertia) { const std::string xml_path = GetTestDataFilePath(kConcaveInertiaPath); std::array 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 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 error; mjModel* model = mj_loadXML(xml_path.c_str(), 0, error.data(), error.size()); diff --git a/test/user/user_objects_test.cc b/test/user/user_objects_test.cc index ec3e0d2a..5acbdfc5 100644 --- a/test/user/user_objects_test.cc +++ b/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 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;