Heap-use-after-free in move_and_slide if the floor body is freed

[tcoxon]

Godot version

3.5.1

System information

Arch Linux (rolling release), Bullet Physics

Issue description

While testing my project in address sanitizer I ran into this fatal error that seems to occur when my player character smashes into an object from above and destroys it:

Screencast.from.2023-03-10.16-24-25.mp4

==20566==ERROR: AddressSanitizer: heap-use-after-free on address 0x61300087f448 at pc 0x000000f34f89 bp 0x7fffc27c0c00 sp 0x7fffc27c0bf0
READ of size 8 at 0x61300087f448 thread T0
    #0 0xf34f88 in RID_OwnerBase::_is_owner(RID const&) const core/rid.h:105
    #1 0xf34f88 in RID_Owner<RigidBodyBullet>::owns(RID const&) const core/rid.h:168
    #2 0xf34f88 in BulletPhysicsServer::body_get_direct_state(RID) modules/bullet/bullet_physics_server.cpp:872
    #3 0x37d187a in KinematicBody::_move_and_slide_internal(Vector3 const&, Vector3 const&, Vector3 const&, bool, int, float, bool) scene/3d/physics_body.cpp:1082
    #4 0x37db506 in KinematicBody::move_and_slide_with_snap(Vector3 const&, Vector3 const&, Vector3 const&, bool, int, float, bool) scene/3d/physics_body.cpp:1229
    #5 0x37e385f in MethodBind7R<Vector3, Vector3 const&, Vector3 const&, Vector3 const&, bool, int, float, bool>::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind_ext.gen.inc:1205
    #6 0x666d907 in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:918
    #7 0x68a8cba in Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) core/variant_call.cpp:1229
    #8 0x1242c20 in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_function.cpp:1050
    #9 0x118b81d in GDScriptInstance::call_multilevel(StringName const&, Variant const**, int) modules/gdscript/gdscript.cpp:1214
    #10 0x2c24123 in Node::_notification(int) scene/main/node.cpp:65
    #11 0x3805dd2 in Node::_notificationv(int, bool) scene/main/node.h:47
    #12 0x3805dd2 in Spatial::_notificationv(int, bool) scene/3d/spatial.h:52
    #13 0x3805dd2 in CollisionObject::_notificationv(int, bool) scene/3d/collision_object.h:38
    #14 0x3805dd2 in PhysicsBody::_notificationv(int, bool) scene/3d/physics_body.h:41
    #15 0x3805dd2 in KinematicBody::_notificationv(int, bool) scene/3d/physics_body.h:262
    #16 0x6668292 in Object::notification(int, bool) core/object.cpp:927
    #17 0x2ca37e6 in SceneTree::_notify_group_pause(StringName const&, int) scene/main/scene_tree.cpp:1105
    #18 0x2cb2913 in SceneTree::iteration(float) scene/main/scene_tree.cpp:561
    #19 0xa4fb69 in Main::iteration() main/main.cpp:2267
    #20 0x9e93af in OS_X11::run() platform/x11/os_x11.cpp:3987
    #21 0x98bacc in main platform/x11/godot_x11.cpp:59
    #22 0x7f2e0a63c78f  (/usr/lib/libc.so.6+0x2378f)
    #23 0x7f2e0a63c849 in __libc_start_main (/usr/lib/libc.so.6+0x23849)
    #24 0x9a1744 in _start (/media/scratch/coding/godot/godot-builder/godot/bin/godot.x11.opt.64s+0x9a1744)

0x61300087f448 is located 8 bytes inside of 360-byte region [0x61300087f440,0x61300087f5a8)
freed by thread T0 here:
    #0 0x7f2e0b2c178a in operator delete(void*, unsigned long) /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_new_delete.cpp:164
    #1 0xf39065 in BulletPhysicsServer::free(RID) modules/bullet/bullet_physics_server.cpp:1547

previously allocated by thread T0 here:
    #0 0x7f2e0b2c0672 in operator new(unsigned long) /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_new_delete.cpp:95
    #1 0xf36153 in BulletPhysicsServer::body_create(PhysicsServer::BodyMode, bool) modules/bullet/bullet_physics_server.cpp:450
    #2 0x61900112b57f  (<unknown module>)

SUMMARY: AddressSanitizer: heap-use-after-free core/rid.h:105 in RID_OwnerBase::_is_owner(RID const&) const
Shadow bytes around the buggy address:
  0x0c2680107e30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2680107e40: 00 00 00 00 00 fa fa fa fa fa fa fa fa fa fa fa
  0x0c2680107e50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2680107e60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2680107e70: 00 00 00 00 00 00 00 00 00 00 00 00 00 fa fa fa
=>0x0c2680107e80: fa fa fa fa fa fa fa fa fd[fd]fd fd fd fd fd fd
  0x0c2680107e90: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2680107ea0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2680107eb0: fd fd fd fd fd fa fa fa fa fa fa fa fa fa fa fa
  0x0c2680107ec0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2680107ed0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==20566==ABORTING

I believe that this could cause a crash (but haven't seen it do that in practice yet).

The player character is a KinematicBody. The rock is a RigidBody with mode static. Upon contact with the rock, if the player has enough speed, the rock is destroyed (queue_free is called). Asan only reports the error if the rock is smashed from above like in the video, not if you slam into it from the side.

I think that the KinematicBody's on_floor_body is left dangling when the rock's RID is freed. Then when move_and_slide is called next time, it tries to get the direct state of the body with the freed RID.

Calling queue_free on the rock a frame after removing it from the tree does not produce the error.

Steps to reproduce

1. Download the attached reproducer project.
2. Build the engine with use_asan=yes
3. Run the project with the asan engine build.

Minimal reproduction project

move_and_slide.zip

[lawnjelly]

Looks like the problem is the KinematicBody stores a RID for on_floor_body when a collision occurs, and it checks this during _move_and_slide_internal() (via body_get_direct_state).

There doesn't appear to be any accounting for the possibility the body could be deleted in between move_and_slide() calls.

Some thoughts on options to solve:

• Notify any physics bodies holding a reference that a particular RID is being deleted. This could be done simply but inefficiently by informing all the physics bodies each time something is deleted.
• Notify by maintaining a back reference to the physics bodies that have a reference. This is tricky because of the client code / physics server boundary.
• Have a way of checking the RID is pointing to a valid object - this is possible with the RID handles build but not with regular RIDs.

I'll have a think about best way to solve.

[akien-mga]

@tcoxon FYI, there's #77616 which should solve this issue.
I expect it might take a while to review/merge so feel free to backport it to your build - it's a correct fix, what might be challenged is whether it fits the way reduz would like it done architecturally.

[lawnjelly]

Had a look in master today, it seems like the same vulnerability is present there too in CharacterBody3D.

Master does additionally store an ObjectID which could be used to prevent the problem (by looking up the ObjectID to see if it is valid before using the RID), but the RID is still used in CharacterBody3D::move_and_slide().

So whichever method we go with will need to be applied there too, but will wait for discussion with @reduz before proceeding.

[lawnjelly]

Fixed by #88946.