Merge pull request #705 from hartikainen:bug/mj_deleteFileVFS-infinite-loop

PiperOrigin-RevId: 507544780
Change-Id: Ie5819d4cc567dce44ceb59624a889a06ecdd7c1b

doc/changelog.rst

@@ -29,16 +29,17 @@ Bug fixes
    :align: right
    :width: 300px
 
-- The ``launch_repl`` functionality in the Python viewer is fixed.
+- The ``launch_repl`` functionality in the Python viewer has been fixed.
 - Set ``time`` correctly in ``mjd_transitionFD``, to support time-dependent user code.
-- Fix sensor data dimension validation when ``user`` type sensors are present.
-- Fix incorrect plugin error message when a null ``nsensordata`` callback is encountered during model compilation.
+- Fixed sensor data dimension validation when ``user`` type sensors are present.
+- Fixed incorrect plugin error message when a null ``nsensordata`` callback is encountered during model compilation.
 - Correctly end the timer (``TM_END``) ``mj_fwdConstraint`` returns early.
+- Fixed an infinite loop in ``mj_deleteFileVFS``.
 
 Simulate
 ^^^^^^^^
 
-- Increase precision of simulate sensor plot y-axis by 1 digit
+- Increased precision of simulate sensor plot y-axis by 1 digit
   (`#719 <https://github.com/deepmind/mujoco/issues/719>`_).
 - Body labels are now drawn at the body frame rather than inertial frame, unless inertia is being visualised.
 

src/engine/engine_vfs.c

@@ -179,10 +179,10 @@ int mj_deleteFileVFS(mjVFS* vfs, const char* filename) {
       mju_free(vfs->filedata[i]);
 
       // scroll remaining files forward
-      while (i<vfs->nfile-1) {
-        mjSTRNCPY(vfs->filename[i], vfs->filename[i+1]);
-        vfs->filesize[i] = vfs->filesize[i+1];
-        vfs->filedata[i] = vfs->filedata[i+1];
+      for (int j=i; j<vfs->nfile-1; j++) {
+        mjSTRNCPY(vfs->filename[j], vfs->filename[j+1]);
+        vfs->filesize[j] = vfs->filesize[j+1];
+        vfs->filedata[j] = vfs->filedata[j+1];
       }
 
       // set last to 0, for style

test/engine/engine_vfs_test.cc

@@ -29,17 +29,53 @@ using EngineVfsTest = MujocoTest;
 TEST_F(EngineVfsTest, AddFileVFS) {
   constexpr char path[] = "engine/testdata/";
   const std::string dir = GetTestDataFilePath(path);
-  std::string file = "refsite.xml";
+  std::string file1 = "activation.xml";
+  std::string file2 = "damper.xml";
+  std::string file3 = "refsite.xml";
 
-  std::FILE* fp = std::fopen((dir + file).c_str(), "r");
-  ASSERT_THAT(fp, NotNull()) << "Input file missing.";
-  std::fclose(fp);
+  std::FILE* fp1 = std::fopen((dir + file1).c_str(), "r");
+  ASSERT_THAT(fp1, NotNull()) << "Input file1 missing.";
+  std::fclose(fp1);
+
+  std::FILE* fp2 = std::fopen((dir + file2).c_str(), "r");
+  ASSERT_THAT(fp2, NotNull()) << "Input file2 missing.";
+  std::fclose(fp2);
+
+  std::FILE* fp3 = std::fopen((dir + file3).c_str(), "r");
+  ASSERT_THAT(fp3, NotNull()) << "Input file3 missing.";
+  std::fclose(fp3);
 
   auto mj_vfs = std::make_unique<mjVFS>();
   mj_defaultVFS(mj_vfs.get());
-  EXPECT_THAT(mj_addFileVFS(mj_vfs.get(), dir.c_str(), file.c_str()), 0);
+
+  EXPECT_THAT(mj_vfs->nfile, 0);
+  EXPECT_THAT(mj_addFileVFS(mj_vfs.get(), dir.c_str(), file1.c_str()), 0);
   EXPECT_THAT(mj_vfs->nfile, 1);
-  mj_deleteFileVFS(mj_vfs.get(), file.c_str());
+  EXPECT_THAT(mj_vfs->filename[0], file1);
+
+  EXPECT_THAT(mj_addFileVFS(mj_vfs.get(), dir.c_str(), file2.c_str()), 0);
+  EXPECT_THAT(mj_vfs->nfile, 2);
+  EXPECT_THAT(mj_vfs->filename[0], file1);
+  EXPECT_THAT(mj_vfs->filename[1], file2);
+
+  EXPECT_THAT(mj_addFileVFS(mj_vfs.get(), dir.c_str(), file3.c_str()), 0);
+  EXPECT_THAT(mj_vfs->nfile, 3);
+  EXPECT_THAT(mj_vfs->filename[0], file1);
+  EXPECT_THAT(mj_vfs->filename[1], file2);
+  EXPECT_THAT(mj_vfs->filename[2], file3);
+
+  mj_deleteFileVFS(mj_vfs.get(), file1.c_str());
+  EXPECT_THAT(mj_vfs->nfile, 2);
+  EXPECT_THAT(mj_vfs->filename[0], file2);
+  EXPECT_THAT(mj_vfs->filename[1], file3);
+
+  mj_deleteFileVFS(mj_vfs.get(), file3.c_str());
+  EXPECT_THAT(mj_vfs->nfile, 1);
+  EXPECT_THAT(mj_vfs->filename[0], file2);
+
+  mj_deleteFileVFS(mj_vfs.get(), file2.c_str());
+  EXPECT_THAT(mj_vfs->nfile, 0);
+
   mj_deleteVFS(mj_vfs.get());
 }