fix: use-after-free when deleting skinned articulated object rigs#2637
fix: use-after-free when deleting skinned articulated object rigs#2637ralyassi wants to merge 1 commit into
Conversation
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.
|
Hi @ralyassi! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Motivation and Context
Deleting a skinned articulated object (e.g. a habitat-lab humanoid) intermittently segfaults inside RigManager::deleteRigInstance(). Two combined defects:
Mismatched id spaces. BulletPhysicsManager::instantiateSkinnedModel() registers the rig under an auto-incremented rig id, but removeArticulatedObject() looks the rig up by object id. These desync as objects are added and removed, so a removal can miss its own rig (leaking the rig instance), delete the bone nodes of another live skinned object, or delete the dangling bone nodes of a stale rig — a use-after-free.
Double ownership of bone nodes. Articulated object rig bones are parented to the object's link nodes, so they are destroyed with the object's scene graph subtree (delete objectNode in PhysicsManager::removeArticulatedObject()). Deleting them again in the rig manager is a double delete. gfx-replay rigs created by ClassicReplayRenderer are different: their bones are parented to the scene root and the rig manager is their rightful owner.
Simulator::reconfigure() compounds this: it replaces the physics manager without per-object removal while the ResourceManager (owner of the RigManager) survives, so stale rig instances with dangling bone pointers accumulate across reconfigures.
Randomized add/remove of skinned AOs reproduces the crash on main within a few dozen operations when freed memory is poisoned (MALLOC_PERTURB_=42):
Note the object being removed is skinned_prism_:0002 (its own rig id is 2); the lookup by object id 11 matched a stale rig of a previously deleted object and deleted its already-freed bone nodes.
Changes:
How Has This Been Tested