From b1aae0c9d5054098332a8ff51a6fcd40a4cac0ea Mon Sep 17 00:00:00 2001 From: ralyassi Date: Thu, 11 Jun 2026 21:39:22 +0200 Subject: [PATCH] fix: use-after-free when deleting skinned articulated object rigs RigManager::deleteRigInstance() unconditionally deleted the rig's bone nodes. For skinned articulated objects the bones are parented to (and owned by) the object's link nodes, so they are destroyed with the object's scene graph subtree; deleting them again is a double delete. Worse, the rig was registered under an auto-incremented rig id, but BulletPhysicsManager::removeArticulatedObject() looks the rig up by object id. The two id spaces desync as objects are added and removed, so a removal could silently skip unregistration (leaking the entry), delete the bones of another live skinned object, or delete the dangling bones of a stale rig - a use-after-free that intermittently segfaults (e.g. when deleting humanoids in habitat-lab). - Add Rig::ownsBones: the rig manager only deletes bone nodes it owns (gfx-replay rigs parented to the scene root). Articulated object rigs reference link-owned nodes and are no longer deleted by the manager. - Key articulated object rigs by object id at registration so that removeArticulatedObject() unregisters the right rig. - Unregister remaining rigs in ~BulletPhysicsManager() so no stale rig instances survive Simulator::reconfigure() (the ResourceManager and its RigManager outlive the physics manager). - Add a python regression test covering interleaved add/remove and reconfigure. --- src/esp/assets/RigManager.cpp | 6 +- src/esp/assets/RigManager.h | 8 ++- src/esp/gfx/SkinData.h | 5 ++ .../physics/bullet/BulletPhysicsManager.cpp | 19 +++++- src/esp/sim/ClassicReplayRenderer.cpp | 3 + tests/test_physics.py | 64 +++++++++++++++++++ 6 files changed, 97 insertions(+), 8 deletions(-) diff --git a/src/esp/assets/RigManager.cpp b/src/esp/assets/RigManager.cpp index f18ee794f5..159e7c23fc 100644 --- a/src/esp/assets/RigManager.cpp +++ b/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); } diff --git a/src/esp/assets/RigManager.h b/src/esp/assets/RigManager.h index 02d99694ac..0fe5d2c9ac 100644 --- a/src/esp/assets/RigManager.h +++ b/src/esp/assets/RigManager.h @@ -26,8 +26,9 @@ 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. @@ -35,7 +36,8 @@ class RigManager { 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. */ diff --git a/src/esp/gfx/SkinData.h b/src/esp/gfx/SkinData.h index 4e5a562a52..2416584126 100644 --- a/src/esp/gfx/SkinData.h +++ b/src/esp/gfx/SkinData.h @@ -52,6 +52,11 @@ struct Rig { std::vector bones; /** @brief Bone name to 'bones' index map. */ std::unordered_map 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 diff --git a/src/esp/physics/bullet/BulletPhysicsManager.cpp b/src/esp/physics/bullet/BulletPhysicsManager.cpp index 6cc0db4d1e..f40967da33 100644 --- a/src/esp/physics/bullet/BulletPhysicsManager.cpp +++ b/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,7 +972,8 @@ 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); @@ -971,8 +981,11 @@ void BulletPhysicsManager::instantiateSkinnedModel( 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); diff --git a/src/esp/sim/ClassicReplayRenderer.cpp b/src/esp/sim/ClassicReplayRenderer.cpp index 2ab43beffa..0d4f4bc79a 100644 --- a/src/esp/sim/ClassicReplayRenderer.cpp +++ b/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; diff --git a/tests/test_physics.py b/tests/test_physics.py index f494ef5a77..2695435684 100644 --- a/tests/test_physics.py +++ b/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