Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash when using create_shape_owner and shape_owner_get_owner #47135

Closed
Tracked by #45334
qarmin opened this issue Mar 18, 2021 · 1 comment · Fixed by #57182
Closed
Tracked by #45334

Crash when using create_shape_owner and shape_owner_get_owner #47135

qarmin opened this issue Mar 18, 2021 · 1 comment · Fixed by #57182

Comments

@qarmin
Copy link
Contributor

qarmin commented Mar 18, 2021

Godot version:
3.3.rc.custom_build. 7f2107e

OS
Ubuntu 20.04 - Ubuntu 3.36 X11

Issue description:
Executing

var q_Area2D : Area2D = Area2D.new()

func _ready() -> void:
	add_child(q_Area2D)

func _process(_delta : float) -> void:
	if randi() % 10 == 0:
		q_Area2D.queue_free()
		q_Area2D = Area2D.new()
		add_child(q_Area2D)

	modify_object(q_Area2D)

static func modify_object(q_Area2D : Area2D) -> void:
	if randi() % 2 == 0:
		print("Executing Area2D.create_shape_owner")
		var variable1: CPUParticles = CPUParticles.new()
		print("Parameters[" + str(variable1) + "]")
		q_Area2D.create_shape_owner(variable1)
		variable1.queue_free()

	if randi() % 2 == 0:
		print("Executing Area2D.shape_owner_get_owner")
		var variable1: int = (randi() % int(10)) - int(10 / 2.0)
		print("Parameters[" + str(variable1) + "]")
		q_Area2D.shape_owner_get_owner(variable1)

crashes Godot and address sanitizer shows this info

==40374==ERROR: AddressSanitizer: heap-use-after-free on address 0x61a000134490 at pc 0x000011788ea6 bp 0x7fffcddace20 sp 0x7fffcddace10
READ of size 8 at 0x61a000134490 thread T0
    #0 0x11788ea5 in Variant::Variant(Object const*) core/variant.cpp:2350
    #1 0xd370fbd in MethodBind1RC<Object*, unsigned int>::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:1333
    #2 0x115568a7 in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:919
    #3 0x117dcebd in Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) core/variant_call.cpp:1149
    #4 0x1d9a4fb in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_function.cpp:1086
    #5 0x1bcabfb in GDScriptInstance::call(StringName const&, Variant const**, int, Variant::CallError&) modules/gdscript/gdscript.cpp:1208
    #6 0x11556417 in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:898
    #7 0x117dcebd in Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) core/variant_call.cpp:1149
    #8 0x1d9a4fb in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_function.cpp:1086
    #9 0x1bcb10e in GDScriptInstance::call_multilevel(StringName const&, Variant const**, int) modules/gdscript/gdscript.cpp:1224
    #10 0xc053bd5 in Node::_notification(int) scene/main/node.cpp:60
    #11 0x1a7971b in Node::_notificationv(int, bool) scene/main/node.h:46
    #12 0x1a7bb90 in CanvasItem::_notificationv(int, bool) scene/2d/canvas_item.h:166
    #13 0xdb40388 in Node2D::_notificationv(int, bool) scene/2d/node_2d.h:38
    #14 0x11556d41 in Object::notification(int, bool) core/object.cpp:929
    #15 0xc18260d in SceneTree::_notify_group_pause(StringName const&, int) scene/main/scene_tree.cpp:992
    #16 0xc172b9b in SceneTree::idle(float) scene/main/scene_tree.cpp:529
    #17 0x18dda96 in Main::iteration() main/main.cpp:2117
    #18 0x17c1bd2 in OS_X11::run() platform/x11/os_x11.cpp:3641
    #19 0x172defb in main platform/x11/godot_x11.cpp:56
    #20 0x7fd59c52a0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #21 0x172db1d in _start (/usr/bin/godots+0x172db1d)

0x61a000134490 is located 16 bytes inside of 1360-byte region [0x61a000134480,0x61a0001349d0)
freed by thread T0 here:
    #0 0x7fd59d6a31b7 in __interceptor_free (/lib/x86_64-linux-gnu/libasan.so.6+0xb01b7)
    #1 0x11a9b2da in Memory::free_static(void*, bool) core/os/memory.cpp:178
    #2 0x18e756b in void memdelete<Object>(Object*) core/os/memory.h:119
    #3 0xc18538d in SceneTree::_flush_delete_queue() scene/main/scene_tree.cpp:1117
    #4 0xc174148 in SceneTree::idle(float) scene/main/scene_tree.cpp:547
    #5 0x18dda96 in Main::iteration() main/main.cpp:2117
    #6 0x17c1bd2 in OS_X11::run() platform/x11/os_x11.cpp:3641
    #7 0x172defb in main platform/x11/godot_x11.cpp:56
    #8 0x7fd59c52a0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

previously allocated by thread T0 here:
    #0 0x7fd59d6a3517 in malloc (/lib/x86_64-linux-gnu/libasan.so.6+0xb0517)
    #1 0x11a9a29b in Memory::alloc_static(unsigned long, bool) core/os/memory.cpp:82
    #2 0x11a9a1ac in operator new(unsigned long, char const*) core/os/memory.cpp:42
    #3 0xbfc0cd5 in Object* ClassDB::creator<CPUParticles>() core/class_db.h:143
    #4 0x112d906a in ClassDB::instance(StringName const&) core/class_db.cpp:559
    #5 0x1ba6102 in GDScriptNativeClass::instance() modules/gdscript/gdscript.cpp:82
    #6 0x1ba5aea in GDScriptNativeClass::_new() modules/gdscript/gdscript.cpp:69
    #7 0x1c81387 in MethodBind0R<Variant>::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:237
    #8 0x115568a7 in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:919
    #9 0x117dcebd in Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) core/variant_call.cpp:1149
    #10 0x1d9a47a in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_function.cpp:1083
    #11 0x1bcabfb in GDScriptInstance::call(StringName const&, Variant const**, int, Variant::CallError&) modules/gdscript/gdscript.cpp:1208
    #12 0x11556417 in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:898
    #13 0x117dcebd in Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) core/variant_call.cpp:1149
    #14 0x1d9a4fb in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_function.cpp:1086
    #15 0x1bcb10e in GDScriptInstance::call_multilevel(StringName const&, Variant const**, int) modules/gdscript/gdscript.cpp:1224
    #16 0xc053bd5 in Node::_notification(int) scene/main/node.cpp:60
    #17 0x1a7971b in Node::_notificationv(int, bool) scene/main/node.h:46
    #18 0x1a7bb90 in CanvasItem::_notificationv(int, bool) scene/2d/canvas_item.h:166
    #19 0xdb40388 in Node2D::_notificationv(int, bool) scene/2d/node_2d.h:38
    #20 0x11556d41 in Object::notification(int, bool) core/object.cpp:929
    #21 0xc18260d in SceneTree::_notify_group_pause(StringName const&, int) scene/main/scene_tree.cpp:992
    #22 0xc172b9b in SceneTree::idle(float) scene/main/scene_tree.cpp:529
    #23 0x18dda96 in Main::iteration() main/main.cpp:2117
    #24 0x17c1bd2 in OS_X11::run() platform/x11/os_x11.cpp:3641
    #25 0x172defb in main platform/x11/godot_x11.cpp:56
    #26 0x7fd59c52a0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

SUMMARY: AddressSanitizer: heap-use-after-free core/variant.cpp:2350 in Variant::Variant(Object const*)

Project for easier testing - GDScript.zip

@timothyqiu
Copy link
Member

This smaller script can also reproduce the crash:

extends SceneTree

var area = Area2D.new()

func _init():
	var v = CPUParticles.new()
	var id = area.create_shape_owner(v)
	v.queue_free()
	yield(self, "idle_frame")  # wait for the object to be freed
	area.shape_owner_get_owner(id)

CollisionObject2D::create_shape_owner() stores a raw Object * so it has no idea that the object is already freed.

After the freed pointer is returned by CollisionObject2D::shape_owner_get_owner(), it's then wrapped into a new Variant object, and there's a call to Object::_use_rc() in the constructor, resulting in a "heap-use-after-free" issue:

_get_obj().rc = likely(obj) ? obj->_use_rc() : NULL;

Adding a ERR_FAIL_COND_V(!ObjectDB::instance_validate(shapes[p_owner].owner), NULL); check in shape_owner_get_owner resolves the crash, but I don't know whether it's an ideal solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants