Store ObjectID instead of pointer for KinematicCollision owner

[timothyqiu]

The intention is to fix heap-use-after-free crash on sanitizer build.

When the result of move_and_collide() or get_slide_collision() is stored in a separate variable, accessing get_local_shape() will crash if the physics body is already freed.

---

To reproduce the crash, save the following script and run godot --headless -s /path/to/script.gd (headless is not necessary) with a sanitizer build.

move_and_collide.gd

extends SceneTree

var player
var saved_collision

func _initialize() -> void:
    _build_scene()


func _physics_process(delta: float) -> bool:
    match Engine.get_physics_frames():
        0:
            # Move and save collision info.
            var collision = player.move_and_collide(Vector3.DOWN)
            saved_collision = collision
            print("[Frame 0] Saved collision: ", saved_collision)

        1:
            # Generate another collision info.
            var collision = player.move_and_collide(Vector3.DOWN)
            print("[Frame 1] Collision: ", collision)

        2:
            print("[Frame 2] Deleting player...")
            player.queue_free()
            player = null

        3:
            print("[Frame 3] Trying to access freed shape...")
            print(saved_collision.get_local_shape())
            return true

    return false


func _build_scene() -> void:
    # A sphere on the top of a box.
    _build_player()
    _build_floor()


func _build_player() -> void:
    var shape = SphereShape3D.new()

    var body = CharacterBody3D.new()
    body.position = Vector3(0, shape.radius, 0)

    var collision = CollisionShape3D.new()
    collision.shape = shape
    body.add_child(collision)

    root.add_child(body)
    player = body


func _build_floor() -> void:
    var shape = BoxShape3D.new()

    var body = StaticBody3D.new()
    body.position = Vector3(0, -shape.size.y / 2, 0)

    var collision = CollisionShape3D.new()
    collision.shape = shape
    body.add_child(collision)

    root.add_child(body)

get_slide_collision.gd

extends SceneTree

var player
var saved_collision

func _initialize() -> void:
    _build_scene()


func _physics_process(delta: float) -> bool:
    match Engine.get_physics_frames():
        0:
            # Move and save collision info.
            player.velocity = Vector3.RIGHT
            player.move_and_slide()
            var collision = player.get_slide_collision(0)
            saved_collision = collision
            print("[Frame 0] Saved collision: ", saved_collision)

        1:
            # Generate another collision info.
            player.velocity = Vector3.RIGHT
            player.move_and_slide()
            var collision = player.get_slide_collision(0)
            print("[Frame 1] Collision: ", collision)

        2:
            print("[Frame 2] Deleting player...")
            player.queue_free()
            player = null

        3:
            print("[Frame 3] Trying to access freed shape...")
            print(saved_collision.get_local_shape())
            return true

    return false


func _build_scene() -> void:
    # A sphere on the top of a box.
    _build_player()
    _build_floor()


func _build_player() -> void:
    var shape = SphereShape3D.new()

    var body = CharacterBody3D.new()
    body.position = Vector3(0, shape.radius, 0)

    var collision = CollisionShape3D.new()
    collision.shape = shape
    body.add_child(collision)

    root.add_child(body)
    player = body


func _build_floor() -> void:
    var shape = BoxShape3D.new()

    var body = StaticBody3D.new()
    body.position = Vector3(0, -shape.size.y / 2, 0)

    var collision = CollisionShape3D.new()
    collision.shape = shape
    body.add_child(collision)

    root.add_child(body)

Output (almost identical for both scripts)

Godot Engine v4.3.dev.custom_build.d00734053 (2024-04-14 12:39:17 UTC) - https://godotengine.org

[Frame 0] Saved collision: <KinematicCollision3D#-9223371813617130681>
[Frame 1] Collision: <KinematicCollision3D#-9223371812425948335>
[Frame 2] Deleting player...
[Frame 3] Trying to access freed shape...
=================================================================
==34049==ERROR: AddressSanitizer: heap-use-after-free on address 0x51c0000c4db4 at pc 0x5c891f996e43 bp 0x7fff45f29740 sp 0x7fff45f29730
READ of size 4 at 0x51c0000c4db4 thread T0
    #0 0x5c891f996e42 in CollisionObject3D::shape_find_owner(int) const scene/3d/physics/collision_object_3d.cpp:696
    #1 0x5c891f9d6152 in KinematicCollision3D::get_local_shape(int) const scene/3d/physics/kinematic_collision_3d.cpp:73
    #2 0x5c891f9e16bd in void call_with_variant_args_retc_helper<__UnexistingClass, Object*, int, 0ul>(__UnexistingClass*, Object* (__UnexistingClass::*)(int) const, Variant const**, Variant&, Callable::CallError&, IndexSequence<0ul>) core/variant/binder_common.h:807
    #3 0x5c891f9df974 in void call_with_variant_args_retc_dv<__UnexistingClass, Object*, int>(__UnexistingClass*, Object* (__UnexistingClass::*)(int) const, Variant const**, int, Variant&, Callable::CallError&, Vector<Variant> const&) core/variant/binder_common.h:568
    #4 0x5c891f9dd2bb in MethodBindTRC<Object*, int>::call(Object*, Variant const**, int, Callable::CallError&) const core/object/method_bind.h:619
    #5 0x5c89193722cb in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_vm.cpp:1828
    #6 0x5c8918fdf4f8 in GDScriptInstance::callp(StringName const&, Variant const**, int, Callable::CallError&) modules/gdscript/gdscript.cpp:1970
    #7 0x5c8923826647 in bool MainLoop::_gdvirtual__physics_process_call<false>(double, bool&) core/os/main_loop.h:45
    #8 0x5c8923824866 in MainLoop::physics_process(double) core/os/main_loop.cpp:61
    #9 0x5c891e6c1f1d in SceneTree::physics_process(double) scene/main/scene_tree.cpp:481
    #10 0x5c89186972d4 in Main::iteration() main/main.cpp:3976
    #11 0x5c89184ab32d in OS_LinuxBSD::run() platform/linuxbsd/os_linuxbsd.cpp:962
    #12 0x5c8918499536 in main platform/linuxbsd/godot_linuxbsd.cpp:85
    #13 0x7da1fb957ccf  (/usr/lib/libc.so.6+0x25ccf) (BuildId: c0caa0b7709d3369ee575fcd7d7d0b0fc48733af)
    #14 0x7da1fb957d89 in __libc_start_main (/usr/lib/libc.so.6+0x25d89) (BuildId: c0caa0b7709d3369ee575fcd7d7d0b0fc48733af)
    #15 0x5c8918499124 in _start (/home/timothy/repos/godot-master/bin/godot.linuxbsd.editor.dev.x86_64.san+0x84e0124) (BuildId: f7e8bd0644eaad36d27c78cb4f9e9f57ca427b6e)

0x51c0000c4db4 is located 1332 bytes inside of 1776-byte region [0x51c0000c4880,0x51c0000c4f70)
freed by thread T0 here:
    #0 0x7da1fbcdfdb2 in __interceptor_free /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:52
    #1 0x5c8923829dac in Memory::free_static(void*, bool) core/os/memory.cpp:168
    #2 0x5c89186ada8e in void memdelete<Object>(Object*) core/os/memory.h:119
    #3 0x5c891e6cad46 in SceneTree::_flush_delete_queue() scene/main/scene_tree.cpp:1374
    #4 0x5c891e6c20e4 in SceneTree::physics_process(double) scene/main/scene_tree.cpp:501
    #5 0x5c89186972d4 in Main::iteration() main/main.cpp:3976
    #6 0x5c89184ab32d in OS_LinuxBSD::run() platform/linuxbsd/os_linuxbsd.cpp:962
    #7 0x5c8918499536 in main platform/linuxbsd/godot_linuxbsd.cpp:85
    #8 0x7da1fb957ccf  (/usr/lib/libc.so.6+0x25ccf) (BuildId: c0caa0b7709d3369ee575fcd7d7d0b0fc48733af)

previously allocated by thread T0 here:
    #0 0x7da1fbce1359 in __interceptor_malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x5c89238291d6 in Memory::alloc_static(unsigned long, bool) core/os/memory.cpp:75
    #2 0x5c89238290f4 in operator new(unsigned long, char const*) core/os/memory.cpp:40
    #3 0x5c891e48b304 in Object* ClassDB::creator<CharacterBody3D>() core/object/class_db.h:145
    #4 0x5c892466ad5b in ClassDB::_instantiate_internal(StringName const&, bool) core/object/class_db.cpp:529
    #5 0x5c892466ae64 in ClassDB::instantiate_no_placeholders(StringName const&) core/object/class_db.cpp:538
    #6 0x5c8918fc29bd in GDScriptNativeClass::instantiate() modules/gdscript/gdscript.cpp:98
    #7 0x5c8918fc2617 in GDScriptNativeClass::_new() modules/gdscript/gdscript.cpp:86
    #8 0x5c891904f105 in void call_with_validated_variant_args_ret_helper<__UnexistingClass, Variant>(__UnexistingClass*, Variant (__UnexistingClass::*)(), Variant const**, Variant*, IndexSequence<>) core/variant/binder_common.h:375
    #9 0x5c891904e9d8 in void call_with_validated_object_instance_args_ret<__UnexistingClass, Variant>(__UnexistingClass*, Variant (__UnexistingClass::*)(), Variant const**, Variant*) core/variant/binder_common.h:662
    #10 0x5c891904dfb7 in MethodBindTR<Variant>::validated_call(Object*, Variant const**, Variant*) const core/object/method_bind.h:535
    #11 0x5c89193768df in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_vm.cpp:1986
    #12 0x5c8918fdf4f8 in GDScriptInstance::callp(StringName const&, Variant const**, int, Callable::CallError&) modules/gdscript/gdscript.cpp:1970
    #13 0x5c89246bfcd5 in Object::callp(StringName const&, Variant const**, int, Callable::CallError&) core/object/object.cpp:815
    #14 0x5c8923ed9ee7 in Variant::callp(StringName const&, Variant const**, int, Variant&, Callable::CallError&) core/variant/variant_call.cpp:1212
    #15 0x5c8919370058 in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_vm.cpp:1734
    #16 0x5c8918fdf4f8 in GDScriptInstance::callp(StringName const&, Variant const**, int, Callable::CallError&) modules/gdscript/gdscript.cpp:1970
    #17 0x5c892382588b in bool MainLoop::_gdvirtual__initialize_call<false>() core/os/main_loop.h:44
    #18 0x5c8923824779 in MainLoop::initialize() core/os/main_loop.cpp:56
    #19 0x5c891e6c1989 in SceneTree::initialize() scene/main/scene_tree.cpp:448
    #20 0x5c89184ab296 in OS_LinuxBSD::run() platform/linuxbsd/os_linuxbsd.cpp:950
    #21 0x5c8918499536 in main platform/linuxbsd/godot_linuxbsd.cpp:85
    #22 0x7da1fb957ccf  (/usr/lib/libc.so.6+0x25ccf) (BuildId: c0caa0b7709d3369ee575fcd7d7d0b0fc48733af)

SUMMARY: AddressSanitizer: heap-use-after-free scene/3d/physics/collision_object_3d.cpp:696 in CollisionObject3D::shape_find_owner(int) const
Shadow bytes around the buggy address:
  0x51c0000c4b00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x51c0000c4b80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x51c0000c4c00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x51c0000c4c80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x51c0000c4d00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x51c0000c4d80: fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd
  0x51c0000c4e00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x51c0000c4e80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x51c0000c4f00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
  0x51c0000c4f80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x51c0000c5000: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
==34049==ABORTING

[akien-mga]

Makes sense, storing Node pointers is always asking for trouble.

[akien-mga]

Thanks!