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