fix: use-after-free when deleting skinned articulated object rigs

src/esp/assets/RigManager.cpp

@@ -27,8 +27,10 @@ void RigManager::deleteRigInstance(int rigId) {
   ESP_CHECK(rigIt != _rigInstances.end(),
             "The specified rig instance id isn't known by rig manager or "
             "was already deleted.");
-  for (auto* bone : getRigInstance(rigId).bones) {
-    delete bone;
+  if (rigIt->second.ownsBones) {
+    for (auto* bone : rigIt->second.bones) {
+      delete bone;
+    }
   }
   _rigInstances.erase(rigIt);
 }

src/esp/assets/RigManager.h

@@ -26,16 +26,18 @@ class RigManager {
 
   /**
    * @brief Registers a rig instance. This gives ownership of the rig to the rig manager. Use @ref deleteRigInstance to dispose of the rig.
-   * This variant assumes that the rig id comes from gfx-replay, so id
-   * management can be skipped.
+   * This variant is used when the rig id is managed externally (e.g. rig ids
+   * that come from gfx-replay, or articulated object ids), so id management
+   * can be skipped.
    *
    * @param rigId Unique id for the rig.
    * @param rig Instantiated rig to register.
    */
   void registerRigInstance(int rigId, gfx::Rig&& rig);
 
   /**
-   * @brief Unregisters a rig instance and deletes its bone nodes.
+   * @brief Unregisters a rig instance. The rig's bone nodes are deleted if
+   * the rig owns them (see @ref gfx::Rig::ownsBones).
    *
    * @param rigId ID of the rig.
    */

src/esp/gfx/SkinData.h

@@ -52,6 +52,11 @@ struct Rig {
   std::vector<scene::SceneNode*> bones;
   /** @brief Bone name to 'bones' index map. */
   std::unordered_map<std::string, int> boneNames;
+  /** @brief Whether the rig owns its bone nodes. If true, the bone nodes are
+   * deleted along with the rig instance. If false, the bone nodes are owned by
+   * the scene graph (e.g. parented to articulated object link nodes) and are
+   * deleted along with their parent nodes. */
+  bool ownsBones = false;
 };
 }  // namespace gfx
 }  // namespace esp

src/esp/physics/bullet/BulletPhysicsManager.cpp

@@ -41,6 +41,15 @@ BulletPhysicsManager::BulletPhysicsManager(
 
 BulletPhysicsManager::~BulletPhysicsManager() {
   ESP_DEBUG() << "Deconstructing BulletPhysicsManager";
+  // Unregister the rigs of any remaining skinned articulated objects so that
+  // no stale rig instances survive in the rig manager, which outlives this
+  // physics manager (e.g. across Simulator::reconfigure()).
+  auto& rigManager = resourceManager_.getRigManager();
+  for (const auto& aoPair : existingArticulatedObjects_) {
+    if (rigManager.rigInstanceExists(aoPair.first)) {
+      rigManager.deleteRigInstance(aoPair.first);
+    }
+  }
   existingObjects_.clear();
   existingArticulatedObjects_.clear();
   staticStageObject_.reset();
@@ -963,16 +972,20 @@ void BulletPhysicsManager::instantiateSkinnedModel(
 
   // Instantiate rig articulation nodes.
   // The nodes are parented to the articulated object links to couple the pose
-  // to the articulated object.
+  // to the articulated object. The link nodes own the bones; the rig instance
+  // only references them.
   esp::gfx::Rig rig{};
   for (int linkId : ao->getLinkIdsWithBase()) {
     auto& link = ao->getLink(linkId);
     rig.boneNames[link.linkName] = rig.bones.size();
     auto* linkNode = &link.node().createChild();
     rig.bones.push_back(linkNode);
   }
-  creationInfo.rigId =
-      resourceManager_.getRigManager().registerRigInstance(std::move(rig));
+  // Key the rig by the articulated object's id so that
+  // removeArticulatedObject() can look it up to unregister it.
+  creationInfo.rigId = ao->getObjectID();
+  resourceManager_.getRigManager().registerRigInstance(creationInfo.rigId,
+                                                       std::move(rig));
 
   auto* gfxNode = resourceManager_.loadAndCreateRenderAssetInstance(
       assetInfo, creationInfo, parentNode, drawables);

src/esp/sim/ClassicReplayRenderer.cpp

@@ -59,6 +59,9 @@ ClassicReplayRenderer::ClassicReplayRenderer(
                 "A rig instance with the specified ID already exists.");
 
       gfx::Rig rig{};
+      // The bone nodes created below are parented to the scene root, so the
+      // rig owns them and they are deleted along with the rig instance.
+      rig.ownsBones = true;
       for (uint32_t i = 0; i < boneNames.size(); ++i) {
         const std::string& boneName = boneNames[i];
         rig.boneNames[boneName] = i;

tests/test_physics.py

@@ -2217,3 +2217,67 @@ def test_bullet_collision_helper():
             sim.get_physics_step_collision_summary()
             == "(no active collision manifolds)\n"
         )
+
+
+@pytest.mark.skipif(
+    not habitat_sim.bindings.built_with_bullet,
+    reason="Articulated objects require Bullet physics.",
+)
+def test_skinned_articulated_object_removal():
+    # Regression test for a heap-use-after-free in RigManager::deleteRigInstance.
+    # Rigs were registered under an auto-incremented rig id but unregistered by
+    # articulated object id. The two id spaces desync as objects are added and
+    # removed, so a removal could delete the bone nodes of a stale rig (already
+    # destroyed with its object's scene graph subtree) or of another live
+    # skinned object.
+    cfg_settings = habitat_sim.utils.settings.default_sim_settings.copy()
+    cfg_settings["scene"] = "NONE"
+    cfg_settings["enable_physics"] = True
+    hab_cfg = habitat_sim.utils.settings.make_cfg(cfg_settings)
+
+    urdf_file = "data/test_assets/urdf/skinned_prism.urdf"
+
+    with habitat_sim.Simulator(hab_cfg) as sim:
+        ao_mgr = sim.get_articulated_object_manager()
+
+        # Two skinned objects, removed in reverse creation order. Before the
+        # fix, removing ao2 missed its rig (the lookup used object ids while
+        # rigs were registered under auto-incremented rig ids), leaving a
+        # stale rig whose bone nodes died with ao2. Removing ao1 then matched
+        # that stale rig by object id and deleted its dangling bone nodes - a
+        # use-after-free.
+        ao1 = ao_mgr.add_articulated_object_from_urdf(urdf_file)
+        ao2 = ao_mgr.add_articulated_object_from_urdf(urdf_file)
+        ao_mgr.remove_object_by_id(ao2.object_id)
+        ao_mgr.remove_object_by_id(ao1.object_id)
+        assert ao_mgr.get_num_objects() == 0
+
+        # A fresh skinned object must still render correctly.
+        ao = ao_mgr.add_articulated_object_from_urdf(urdf_file)
+        ao.translation = [1.0, -3.0, -6.0]
+        sim.get_sensor_observations()
+        ao_mgr.remove_object_by_id(ao.object_id)
+        assert ao_mgr.get_num_objects() == 0
+
+        # Interleave additions and removals so that object ids and rig ids
+        # cannot stay accidentally aligned.
+        live = [ao_mgr.add_articulated_object_from_urdf(urdf_file) for _ in range(3)]
+        for _ in range(6):
+            ao_mgr.remove_object_by_id(live.pop(0).object_id)
+            live.append(ao_mgr.add_articulated_object_from_urdf(urdf_file))
+            sim.get_sensor_observations()
+        for ao in live:
+            ao_mgr.remove_object_by_id(ao.object_id)
+        assert ao_mgr.get_num_objects() == 0
+
+        # Reconfigure replaces the physics manager without removing objects
+        # one by one. The rigs of any remaining skinned objects must be
+        # unregistered so that no stale rig instances survive in the
+        # ResourceManager, which outlives the physics manager.
+        ao_mgr.add_articulated_object_from_urdf(urdf_file)
+        cfg_settings["frustum_culling"] = not cfg_settings.get("frustum_culling", False)
+        sim.reconfigure(habitat_sim.utils.settings.make_cfg(cfg_settings))
+        ao_mgr = sim.get_articulated_object_manager()
+        ao = ao_mgr.add_articulated_object_from_urdf(urdf_file)
+        ao_mgr.remove_object_by_id(ao.object_id)
+        assert ao_mgr.get_num_objects() == 0