Godot: Godot crashes when opening up scene: heap-use-after-free in CanvasItemEditor::_build_bones_list(Node*)

[Shadowblitz16]

Godot version:
3.2.0

OS/device including version:
Window 10 Pro Education 64 bit

Issue description:
Godot crashes when opening up my WidgetItem scene.

Steps to reproduce:

• download this project
• open it
• open widgets/widgetItem/widgetItem scene

Minimal reproduction project:
above click on this.

[KoBeWi]

Backtrace:

CrashHandlerException: Program crashed
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[0] HashMap<Variant,List<Pair<Variant const *,Variant>,DefaultAllocator>::Element *,VariantHasher,VariantComparator,3,8>::getptr (x:\inne\godot\source\godot\core\hash_map.h:328)
[1] HashMap<Variant,List<Pair<Variant const *,Variant>,DefaultAllocator>::Element *,VariantHasher,VariantComparator,3,8>::getptr (x:\inne\godot\source\godot\core\hash_map.h:328)
[2] HashMap<Variant,List<Pair<Variant const *,Variant>,DefaultAllocator>::Element *,VariantHasher,VariantComparator,3,8>::has (x:\inne\godot\source\godot\core\hash_map.h:285)
[3] OrderedHashMap<Variant,Variant,VariantHasher,VariantComparator,3,8>::has (x:\inne\godot\source\godot\core\ordered_hash_map.h:241)
[4] Dictionary::has (x:\inne\godot\source\godot\core\dictionary.cpp:136)
[5] Object::has_meta (x:\inne\godot\source\godot\core\object.cpp:1032)
[6] SceneTreeEditor::_add_nodes (x:\inne\godot\source\godot\editor\scene_tree_editor.cpp:196)
[7] SceneTreeEditor::_update_tree (x:\inne\godot\source\godot\editor\scene_tree_editor.cpp:557)
[8] MethodBind0<SceneTreeEditor>::call (x:\inne\godot\source\godot\core\method_bind.gen.inc:149)
[9] Object::call (x:\inne\godot\source\godot\core\object.cpp:921)
[10] MessageQueue::_call_function (x:\inne\godot\source\godot\core\message_queue.cpp:250)
[11] MessageQueue::flush (x:\inne\godot\source\godot\core\message_queue.cpp:299)
[12] Main::iteration (x:\inne\godot\source\godot\main\main.cpp:2012)
[13] OS_Windows::run (x:\inne\godot\source\godot\platform\windows\os_windows.cpp:3152)
[14] widechar_main (x:\inne\godot\source\godot\platform\windows\godot_windows.cpp:162)
[15] _main (x:\inne\godot\source\godot\platform\windows\godot_windows.cpp:184)
[16] main (x:\inne\godot\source\godot\platform\windows\godot_windows.cpp:196)
[17] __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283)
[18] BaseThreadInitThunk
-- END OF BACKTRACE --

[timothyqiu]

The node's metadata is a dictionary with a null _p.

[4] Dictionary::has (x:\inne\godot\source\godot\core\dictionary.cpp:136)
[5] Object::has_meta (x:\inne\godot\source\godot\core\object.cpp:1032)
[6] SceneTreeEditor::_add_nodes (x:\inne\godot\source\godot\editor\scene_tree_editor.cpp:196)

https://github.com/godotengine/godot/blob/5127afa812f80c02e6f9fada854d46482daafca4/core/dictionary.cpp#L133-L136

[Shadowblitz16]

can this be printed out as a error instead of crashing?

[qarmin]

Address Sanitizer Log

==17412==ERROR: AddressSanitizer: heap-use-after-free on address 0x619001b6c090 at pc 0x0000069a6693 bp 0x7ffe8f194970 sp 0x7ffe8f194960
READ of size 8 at 0x619001b6c090 thread T0
    #0 0x69a6692 in CanvasItemEditor::_build_bones_list(Node*) editor/plugins/canvas_item_editor_plugin.cpp:3538
    #1 0x69cb721 in CanvasItemEditor::_update_bone_list() editor/plugins/canvas_item_editor_plugin.cpp:3961
    #2 0x1740bd5 in MethodBind0::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:59
    #3 0xce51dc2 in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:921
    #4 0xce2827c in MessageQueue::_call_function(Object*, StringName const&, Variant const*, int, bool) core/message_queue.cpp:250
    #5 0xce28e5b in MessageQueue::flush() core/message_queue.cpp:297
    #6 0x15722d3 in Main::iteration() main/main.cpp:2010
    #7 0x14898ee in OS_X11::run() platform/x11/os_x11.cpp:3262
    #8 0x140f489 in main platform/x11/godot_x11.cpp:56
    #9 0x7fca6ddb61e2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x271e2)
    #10 0x140f0ad in _start (/usr/bin/godots+0x140f0ad)

0x619001b6c090 is located 16 bytes inside of 1104-byte region [0x619001b6c080,0x619001b6c4d0)
freed by thread T0 here:
    #0 0x7fca6f2da6ef in __interceptor_free (/lib/x86_64-linux-gnu/libasan.so.5+0x10d6ef)
    #1 0xd2d4a93 in Memory::free_static(void*, bool) core/os/memory.cpp:178
    #2 0x157ce40 in void memdelete<Object>(Object*) core/os/memory.h:119
    #3 0x864d1be in SceneTree::_flush_delete_queue() scene/main/scene_tree.cpp:1113
    #4 0x863b27f in SceneTree::iteration(float) scene/main/scene_tree.cpp:491
    #5 0x157229e in Main::iteration() main/main.cpp:2005
    #6 0x14898ee in OS_X11::run() platform/x11/os_x11.cpp:3262
    #7 0x140f489 in main platform/x11/godot_x11.cpp:56
    #8 0x7fca6ddb61e2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x271e2)

previously allocated by thread T0 here:
    #0 0x7fca6f2daae8 in malloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10dae8)
    #1 0xd2d3c25 in Memory::alloc_static(unsigned long, bool) core/os/memory.cpp:82
    #2 0xd2d3b24 in operator new(unsigned long, char const*) core/os/memory.cpp:42
    #3 0x84bbe69 in Object* ClassDB::creator<Control>() (/usr/bin/godots+0x84bbe69)
    #4 0xcc20454 in ClassDB::instance(StringName const&) core/class_db.cpp:548
    #5 0xacf0ac4 in SceneState::instance(SceneState::GenEditState) const scene/resources/packed_scene.cpp:154
    #6 0xad1e291 in PackedScene::instance(PackedScene::GenEditState) const scene/resources/packed_scene.cpp:1695
    #7 0x5668892 in EditorNode::load_scene(String const&, bool, bool, bool, bool) editor/editor_node.cpp:3412
    #8 0x566b52a in EditorNode::open_request(String const&) editor/editor_node.cpp:3474
    #9 0x56708ce in EditorNode::_quick_opened() editor/editor_node.cpp:3584
    #10 0x1740bd5 in MethodBind0::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:59
    #11 0xce51dc2 in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:921
    #12 0xce5a69b in Object::emit_signal(StringName const&, Variant const**, int) core/object.cpp:1217
    #13 0xce5c361 in Object::emit_signal(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) core/object.cpp:1274
    #14 0x5ff75c7 in EditorQuickOpen::_confirmed() editor/quick_open.cpp:252
    #15 0x1740bd5 in MethodBind0::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:59
    #16 0xce51dc2 in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:921
    #17 0xce5a69b in Object::emit_signal(StringName const&, Variant const**, int) core/object.cpp:1217
    #18 0xce5c361 in Object::emit_signal(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) core/object.cpp:1274
    #19 0x92af718 in Tree::_gui_input(Ref<InputEvent>) scene/gui/tree.cpp:2719
    #20 0x5c92df2 in MethodBind1<Ref<InputEvent> >::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:775
    #21 0xce4df68 in Object::call_multilevel(StringName const&, Variant const**, int) core/object.cpp:763
    #22 0xce50cbb in Object::call_multilevel(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) core/object.cpp:863
    #23 0x873173a in Viewport::_gui_call_input(Control*, Ref<InputEvent> const&) scene/main/viewport.cpp:1669
    #24 0x873bee7 in Viewport::_gui_input_event(Ref<InputEvent>) scene/main/viewport.cpp:1979
    #25 0x875e3ab in Viewport::input(Ref<InputEvent> const&) scene/main/viewport.cpp:2825
    #26 0x872676a in Viewport::_vp_input(Ref<InputEvent> const&) scene/main/viewport.cpp:1446
    #27 0x219fb56 in MethodBind1<Ref<InputEvent> const&>::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:775
    #28 0xce51dc2 in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:921
    #29 0xce50735 in Object::call(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) core/object.cpp:847

SUMMARY: AddressSanitizer: heap-use-after-free editor/plugins/canvas_item_editor_plugin.cpp:3538 in CanvasItemEditor::_build_bones_list(Node*)

[timothyqiu]

AstroWars/widgets/widgetItem/widgetItem.gd is a tool script, and has the code:

func _on_Control_tree_entered():
    var parent = get_parent();
    if(parent != null && parent.get_class() == "WidgetList"):
        if!(is_connected("item_pressed", parent, "item_pressed")):
            connect("item_pressed", parent, "item_pressed")
        if!(is_connected("item_changed", parent, "item_changed")):
            connect("item_changed", parent, "item_changed")
        var i = get_index();
        var x = 0;
        var y = (parent.rect_size.y / 2) - (i * rect_min_size.y);
        var w = (parent.rect_size.x    )
        var h =         rect_min_size.y;
        rect_position = Vector2(x,y)
        rect_size     = Vector2(w,h)
    else:
        queue_free();

So when widgetItem.tscn is opened, the scene root enters the tree, and queue_free on the last line is called, crashing other editor codes that uses the scene root.

Removing tool, or commenting out the queue_free call can prevent the editor from crashing.

p.s. This also crashes 3.1.2

[akien-mga]

Then I'm not sure we can fix this.

tool scripts are very powerful, but that power comes with responsibility and the risk of crashing the engine if doing wrong things. So this crash is not a bug, but a consequence of the feature letting users shoot themselves in the foot :)

I'll make this clearer in the docs.

[akien-mga]

See godotengine/godot-docs#3094.

[timothyqiu]

@Shadowblitz16 Maybe checking Engine.editor_hint might help before the queue_free() call.

[Shadowblitz16]

@akien-mga
please reopen this.
people should be able to use tool scripts without the editor abruptly crashing on them.

what should happen is that the scene editor goes blank and shows the error and requires you to fix it before you can edit the scene again.

here is a example

this is kinda what visual studio does with winforms or wpf when your have a invalid script

[vnen]

Well, in this case the script isn't invalid. It is a valid script performing its tasks. It just so happens that the tasks involve freeing nodes that are being accessed by the editor itself, which causes it to crash. I don't think there's anything that can be done to prevent that.

[akien-mga]

@Shadowblitz16 As I said, this cannot be fixed.

It's not a matter of how to present information, but it's technically not possible to do what you ask for, as least not without greatly crippling the engine's performance by adding checks to all methods to restrict using some in tool scripts, and that only in very specific situations. Here you'd want to prevent deleting the scene root, or any node which is being used by an editor plugin -- how do we know that from the Node API? -- but not prevent outright freeing nodes, since it's a normal operation even in tool scripts, when used with caution.

As @vnen mentions, the script is valid, so we can't predict the issue from the parser. Running it however breaks stuff in the editor because of the power give to you over the editor by the tool keyword. It's a powerful feature, but that has drawbacks. There are not many technologies that let you do what tool keywords allow (i.e. run code that can modify the editor itself), in part for the same reasons: it needs to be wielded with caution.

[Shadowblitz16]

wouldn't the only check need to be in the methods the free nodes?
btw if the root of the scene is freed shouldn't it just go back to the new scene window?