From d8691310096a0dfcc5a6597fa85e75907dde3a66 Mon Sep 17 00:00:00 2001 From: dloe Date: Tue, 3 Sep 2024 16:15:31 -0700 Subject: [PATCH 1/4] somewhat messy Unoptimized Solution for UAF exception fix --- .../Elements/CharacterComponent.cs | 55 +++++++++++++++++++ .../Stride.Physics/Engine/PhysicsComponent.cs | 5 +- 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs index 7e0202cb83..64c0cb756c 100644 --- a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs +++ b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs @@ -300,6 +300,61 @@ protected override void OnAttach() Simulation.AddCharacter(this, (CollisionFilterGroupFlags)CollisionGroup, CanCollideWith); } + /// + /// Reconstruct of character controller to avoid UAF crash related to disposed ref to native object + /// + /// - NativeCollisionObject shows old refs + /// - Therefore KinematicCharacter also holds old refs + /// + /// - Tests show nativeCollisionObject being remade does not cause the read while use exception, its just kinematic character + /// + public override void ComposeShape() + { + //Disposing of ColliderShape should happen before we remove NativeCollisionObject and KinematicCharacter from Simulation + base.ComposeShape(); + + //make PairCachingGhostObject valid, therefore make new instance of it and/or kinematic character controller + //keep references valid + if (NativeCollisionObject != null && KinematicCharacter != null) + { + //very mediocre workaround to avoid the nullref when we remove character + Simulation simRef = Simulation; + + //remove references in discreteDynamicsWorld + Simulation.RemoveCharacter(this); + Simulation = simRef; + + /*NativeCollisionObject.Dispose(); + NativeCollisionObject = null; + NativeCollisionObject = new BulletSharp.PairCachingGhostObject + { + CollisionShape = ColliderShape.InternalShape, + UserObject = this, + }; + + NativeCollisionObject.CollisionFlags |= BulletSharp.CollisionFlags.CharacterObject; + + if (ColliderShape.NeedsCustomCollisionCallback) + { + NativeCollisionObject.CollisionFlags |= BulletSharp.CollisionFlags.CustomMaterialCallback; + } + + NativeCollisionObject.ContactProcessingThreshold = !Simulation.CanCcd ? 1e18f : 1e30f;*/ + + //destroy and deref KinematicCharacter then reconstruct it + KinematicCharacter.Dispose(); + KinematicCharacter = null; + BulletSharp.Math.Vector3 unitY = new BulletSharp.Math.Vector3(0f, 1f, 0f); + //Make new KinematicCharacter + KinematicCharacter = new BulletSharp.KinematicCharacterController((BulletSharp.PairCachingGhostObject)NativeCollisionObject, (BulletSharp.ConvexShape)ColliderShape.InternalShape, StepHeight, ref unitY); + + //now we can add these references BACK in discreteDynamicsWorld. + Simulation.AddCharacter(this, (CollisionFilterGroupFlags)CollisionGroup, CanCollideWith); + } + + + } + protected override void OnDetach() { if (KinematicCharacter == null) return; diff --git a/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs b/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs index 74b5acbc39..1e1e04c6c9 100644 --- a/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs +++ b/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs @@ -583,7 +583,10 @@ public void UpdatePhysicsTransformation(bool forceUpdateTransform = true) } } - public void ComposeShape() + /// + /// Made virtual for added behavior in CharacterComponent with UAF exception + /// + public virtual void ComposeShape() { ColliderShapeChanged = false; From e4871c28e582eea106acc4472fd2eeea697163d3 Mon Sep 17 00:00:00 2001 From: dloe Date: Wed, 4 Sep 2024 15:13:15 -0700 Subject: [PATCH 2/4] Multiple physics properties carry over on KinematicCharacter reConstruction --- .../Elements/CharacterComponent.cs | 45 ++++++++----------- 1 file changed, 19 insertions(+), 26 deletions(-) diff --git a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs index 64c0cb756c..cd9b17ea76 100644 --- a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs +++ b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs @@ -301,19 +301,16 @@ protected override void OnAttach() } /// - /// Reconstruct of character controller to avoid UAF crash related to disposed ref to native object - /// - /// - NativeCollisionObject shows old refs - /// - Therefore KinematicCharacter also holds old refs - /// - /// - Tests show nativeCollisionObject being remade does not cause the read while use exception, its just kinematic character + /// Reconstruct of character controller to avoid possible UAF issues + /// Context: On ColliderShape changes, when ComposeShape is ran to rebuild ColliderShape properties Disposing the old ColliderShape can cause UAF + /// issues inside KinematicCharacter and inside Simulation discreteDynamicWorld due to old disposed references to the native object. /// public override void ComposeShape() { //Disposing of ColliderShape should happen before we remove NativeCollisionObject and KinematicCharacter from Simulation base.ComposeShape(); - //make PairCachingGhostObject valid, therefore make new instance of it and/or kinematic character controller + //make PairCachingGhostObject references in KinematicCharacter valid, therefore make new instance of kinematic character controller with updated NativeCollisionObject //keep references valid if (NativeCollisionObject != null && KinematicCharacter != null) { @@ -324,35 +321,31 @@ public override void ComposeShape() Simulation.RemoveCharacter(this); Simulation = simRef; - /*NativeCollisionObject.Dispose(); - NativeCollisionObject = null; - NativeCollisionObject = new BulletSharp.PairCachingGhostObject - { - CollisionShape = ColliderShape.InternalShape, - UserObject = this, - }; - - NativeCollisionObject.CollisionFlags |= BulletSharp.CollisionFlags.CharacterObject; - - if (ColliderShape.NeedsCustomCollisionCallback) - { - NativeCollisionObject.CollisionFlags |= BulletSharp.CollisionFlags.CustomMaterialCallback; - } - - NativeCollisionObject.ContactProcessingThreshold = !Simulation.CanCcd ? 1e18f : 1e30f;*/ - //destroy and deref KinematicCharacter then reconstruct it + BulletSharp.KinematicCharacterController kinematicCharacterProperties = KinematicCharacter; KinematicCharacter.Dispose(); KinematicCharacter = null; BulletSharp.Math.Vector3 unitY = new BulletSharp.Math.Vector3(0f, 1f, 0f); //Make new KinematicCharacter KinematicCharacter = new BulletSharp.KinematicCharacterController((BulletSharp.PairCachingGhostObject)NativeCollisionObject, (BulletSharp.ConvexShape)ColliderShape.InternalShape, StepHeight, ref unitY); + OverrideKinematicCharacterValues(kinematicCharacterProperties); - //now we can add these references BACK in discreteDynamicsWorld. + //now we can add these references BACK in Simulation's discreteDynamicsWorld Simulation.AddCharacter(this, (CollisionFilterGroupFlags)CollisionGroup, CanCollideWith); } + } - + /// + /// When we reconstruct our KinematicCharacter, we would want to preserve some physics properties. Try to have the + /// physics of the object try to match as close as possible. + /// + /// + private void OverrideKinematicCharacterValues(BulletSharp.KinematicCharacterController oldController) + { + KinematicCharacter.FallSpeed = oldController.FallSpeed; + KinematicCharacter.Gravity = oldController.Gravity; + KinematicCharacter.JumpSpeed = oldController.JumpSpeed; + KinematicCharacter.LinearVelocity = oldController.LinearVelocity; } protected override void OnDetach() From 8b1de26bad7168a4a8a587953df999d52b205f22 Mon Sep 17 00:00:00 2001 From: dloe Date: Wed, 4 Sep 2024 15:30:33 -0700 Subject: [PATCH 3/4] Forgot to remove unneeded reference to NativeCollisionObject in reconstruction --- sources/engine/Stride.Physics/Elements/CharacterComponent.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs index cd9b17ea76..f6477ff794 100644 --- a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs +++ b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs @@ -312,7 +312,7 @@ public override void ComposeShape() //make PairCachingGhostObject references in KinematicCharacter valid, therefore make new instance of kinematic character controller with updated NativeCollisionObject //keep references valid - if (NativeCollisionObject != null && KinematicCharacter != null) + if (KinematicCharacter != null) { //very mediocre workaround to avoid the nullref when we remove character Simulation simRef = Simulation; From 63c51a448160cd92b6b183ecd0ed8d7675c1d1ac Mon Sep 17 00:00:00 2001 From: dloe Date: Wed, 4 Sep 2024 15:39:25 -0700 Subject: [PATCH 4/4] grammar issues with comments --- .../engine/Stride.Physics/Elements/CharacterComponent.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs index f6477ff794..b318e0e88f 100644 --- a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs +++ b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs @@ -302,7 +302,7 @@ protected override void OnAttach() /// /// Reconstruct of character controller to avoid possible UAF issues - /// Context: On ColliderShape changes, when ComposeShape is ran to rebuild ColliderShape properties Disposing the old ColliderShape can cause UAF + /// Context: On ColliderShape changes, when ComposeShape is ran to rebuild ColliderShape properties, disposing the old ColliderShape can cause UAF /// issues inside KinematicCharacter and inside Simulation discreteDynamicWorld due to old disposed references to the native object. /// public override void ComposeShape() @@ -310,8 +310,8 @@ public override void ComposeShape() //Disposing of ColliderShape should happen before we remove NativeCollisionObject and KinematicCharacter from Simulation base.ComposeShape(); - //make PairCachingGhostObject references in KinematicCharacter valid, therefore make new instance of kinematic character controller with updated NativeCollisionObject - //keep references valid + //make PairCachingGhostObject references in KinematicCharacter valid, therefore make new instance of kinematic character controller with + //updated NativeCollisionObject keep references valid if (KinematicCharacter != null) { //very mediocre workaround to avoid the nullref when we remove character