(EditorInspectorPlugin) `add_property_editor()` sometimes fails to catch stale reference before casting, crashes editor with segfault error.

CardboardCarl commented 4 months ago

Tested versions

Reproducible in v4.3.beta[6b281c0c0] and 4.2.2.stable

System information

Godot v4.3.beta (6b281c0c0) - Arch Linux #1 SMP PREEMPT_DYNAMIC Fri, 31 May 2024 15:14:45 +0000 - X11 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3050 Laptop GPU - AMD Ryzen 5 5600H with Radeon Graphics (12 Threads)

Issue description

EditorInspectorPlugin's add_property_editor() function fails to catch a stale reference from a freed object, resulting in an editor crash when it tries to cast the object before adding it to the inspector. An easy way to trigger this is by deselecting a node with a custom property editor, causing it to be freed when the inspector is hidden, then opening the node's inspector again.

instantiating a property like this, although already written unsafe, should at least throw an error:

extends EditorInspectorPlugin

var new_editor = preload("res://addons/condition_checker/editor/new_editor.gd").new() # <--- new() should not be used here 

func _can_handle(object):
    return true

func _parse_property(object, type, name, hint_type, hint_string, usage_flags, wide):
    if type == TYPE_ARRAY:
        # if the inspector is closed, this object is deleted and the reference to it becomes stale.
        add_property_editor(name, new_editor)
        return true
    else:
        return false

I'm positive the bug is coming from this function specifically, as when I encountered this crash accidentally I slowly removed my code until the crash stopped happening, and this is where it happened. As to why it happens, I'm not sure. I'm going to investigate it further.

Note: It seems the error is sometimes caught, but not always, as this error is thrown sometimes:

 SCRIPT ERROR: Invalid type in function 'add_property_editor' in base 'EditorInspectorPlugin (new_inspector.gd)'. The Object-derived class of argument 2 (previously freed) is not a subclass of the expected argument class.

My best guess as to why is that the memory block where the old object was might not have been reallocated and overwritten when the node was selected again, so the inspector cast goes through with little to no corruption despite the inspector node itself being freed.

Here's a (sanitized) stack dump (debug symbols enabled):

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.3.beta.custom_build (6b281c0c07b07f2142b1fc8a6b3158091a9b124c)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /usr/lib/libc.so.6(+0x3cae0) [0x7ffff7cf0ae0] (??:0)
[2] /home/user/godot/bin/godot.linuxbsd.editor.dev.x86_64.llvm(__dynamic_cast+0xf8) [0x55555d7c1ef8] (??:?)
[3] Control* Object::cast_to<Control>(Object*) (/home/user/godot/./core/object/object.h:805)
[4] VariantObjectClassChecker<Control*>::check(Variant const&) (/home/user/godot/./core/variant/binder_common.h:231)
[5] VariantCasterAndValidate<Control*>::cast(Variant const**, unsigned int, Callable::CallError&) (/home/user/godot/./core/variant/binder_common.h:256)
[6] void call_with_variant_args_helper<__UnexistingClass, String const&, Control*, bool, String const&, 0ul, 1ul, 2ul, 3ul>(__UnexistingClass*, void (__UnexistingClass::*)(String const&, Control*, bool, String const&), Variant const**, Callable::CallError&, IndexSequence<0ul, 1ul, 2ul, 3ul>) (/home/user/godot/./core/variant/binder_common.h:304)
[7] void call_with_variant_args_dv<__UnexistingClass, String const&, Control*, bool, String const&>(__UnexistingClass*, void (__UnexistingClass::*)(String const&, Control*, bool, String const&), Variant const**, int, Callable::CallError&, Vector<Variant> const&) (/home/user/godot/./core/variant/binder_common.h:452)
[8] MethodBindT<String const&, Control*, bool, String const&>::call(Object*, Variant const**, int, Callable::CallError&) const (/home/user/godot/./core/object/method_bind.h:345)
[9] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (/home/user/godot/modules/gdscript/gdscript_vm.cpp:1863)
[10] GDScriptInstance::callp(StringName const&, Variant const**, int, Callable::CallError&) (/home/user/godot/modules/gdscript/gdscript.cpp:2058)
[11] bool EditorInspectorPlugin::_gdvirtual__parse_property_call<false>(Object*, Variant::Type, String, PropertyHint, String, BitField<PropertyUsageFlags>, bool, bool&) (/home/user/godot/editor/editor_inspector.h:253)
[12] EditorInspectorPlugin::parse_property(Object*, Variant::Type, String const&, PropertyHint, String const&, BitField<PropertyUsageFlags>, bool) (/home/user/godot/editor/editor_inspector.cpp:1193)
[13] EditorInspector::update_tree() (/home/user/godot/editor/editor_inspector.cpp:3380)
[14] EditorInspector::edit(Object*) (/home/user/godot/editor/editor_inspector.cpp:?)
[15] EditorNode::_edit_current(bool, bool) (/home/user/godot/editor/editor_node.cpp:2484)
[16] EditorNode::push_item(Object*, String const&, bool) (/home/user/godot/editor/editor_node.cpp:2309)
[17] EditorNode::push_node_item(Node*) (/home/user/godot/editor/editor_node.cpp:2294)
[18] SceneTreeDock::_push_item(Object*) (/home/user/godot/editor/scene_tree_dock.cpp:1739)
[19] SceneTreeDock::_handle_select(Node*) (/home/user/godot/editor/scene_tree_dock.cpp:1754)
[20] SceneTreeDock::_node_selected() (/home/user/godot/editor/scene_tree_dock.cpp:1763)
[21] void call_with_variant_args_helper<SceneTreeDock>(SceneTreeDock*, void (SceneTreeDock::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (/home/user/godot/./core/variant/binder_common.h:309)
[22] void call_with_variant_args<SceneTreeDock>(SceneTreeDock*, void (SceneTreeDock::*)(), Variant const**, int, Callable::CallError&) (/home/user/godot/./core/variant/binder_common.h:419)
[23] CallableCustomMethodPointer<SceneTreeDock>::call(Variant const**, int, Variant&, Callable::CallError&) const (/home/user/godot/./core/object/callable_method_pointer.h:104)
[24] Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const (/home/user/godot/core/variant/callable.cpp:58)
[25] CallQueue::_call_function(Callable const&, Variant const*, int, bool) (/home/user/godot/core/object/message_queue.cpp:221)
[26] CallQueue::flush() (/home/user/godot/core/object/message_queue.cpp:270)
[27] SceneTree::physics_process(double) (/home/user/godot/scene/main/scene_tree.cpp:492)
[28] Main::iteration() (/home/user/godot/main/main.cpp:4057)
[29] OS_LinuxBSD::run() (/home/user/godot/platform/linuxbsd/os_linuxbsd.cpp:962)
[30] /home/user/godot/bin/godot.linuxbsd.editor.dev.x86_64.llvm(main+0x1bf) [0x555557ebacaf] (/home/user/godot/platform/linuxbsd/godot_linuxbsd.cpp:86)
[31] /usr/lib/libc.so.6(+0x25c88) [0x7ffff7cd9c88] (??:0)
[32] /usr/lib/libc.so.6(__libc_start_main+0x8c) [0x7ffff7cd9d4c] (??:0)
[33] /home/user/godot/bin/godot.linuxbsd.editor.dev.x86_64.llvm(_start+0x25) [0x555557ebaa15] (??:?)
-- END OF BACKTRACE --
================================================================

Steps to reproduce

Open the example project and open control.tscn, or create and preload any EditorProperty script into an EditorInspectorPlugin with add_property_editor without creating a new instance for the EditorProperty
Open a node affected by the plugin in the inspector (the Control node in control.tscn in the example project)
Deselect the node to make the inspector hide, and then reselect the node
Repeat until editor crashes

Minimal reproduction project (MRP)

EditorBugExample.zip

Edit: Failed to notice the segfault error code at the top of the stack dump. Whoops!

CardboardCarl commented 4 months ago

I'm already working on a potential fix, I'll link the pull request here once I'm finished.

CardboardCarl commented 4 months ago

It's worth mentioning that this might be a problem with variant casting validation itself and inspector plugins are just one of the ways the crash is triggered.

If anyone has any thoughts or theories, it'd be appreciated! I'm still trying to figure out how the object is slipping past validation...

huwpascoe commented 4 months ago

There's a double no-no here. If the node has multiple properties of TYPE_ARRAY...

@export var a: Array
@export var b: Array

The same Control node will be bound twice, potentially clobbering both properties' data.

As for the crash, the inspector explicitly deletes all property nodes when it's cleared, bypassing any reference counting. dynamic_cast is performed on a freed pointer, resulting in std::bad_cast. Since Godot doesn't make exceptions, the only thing to do is abort.

A proper fix would be to change add_property_editor() to accept and store RefCounted, which if count is != 1 then ERR_FAIL_EDMSG("usage: add_property_editor(name, MyPropertyEditor.new())"), or better worded.

CardboardCarl commented 4 months ago

There's a double no-no here. If the node has multiple properties of TYPE_ARRAY...
@export var a: Array
@export var b: Array
The same Control node will be bound twice, potentially clobbering both properties' data.

Good point! I'll have to keep that in mind.

As for the crash, the inspector explicitly deletes all property nodes when it's cleared, bypassing any reference counting. dynamic_cast is performed on a freed pointer, resulting in std::bad_cast. Since Godot doesn't make exceptions, the only thing to do is abort.

I wasn't aware the engine couldn't handle bad casts without crashing. I'm still trying to grasp how the engine's error handling works 😅

A proper fix would be to change add_property_editor() to accept and store RefCounted, which if count is != 1 then ERR_FAIL_EDMSG("usage: add_property_editor(name, MyPropertyEditor.new())"), or better worded.

Good idea, I'm just worried about breaking compatibility. I guess I could add versions of the function that take PackedScenes or Scripts instead and fix the current function to check if the referenced object has already been freed, but the exception occurs just before the native add_property_editor function is called, so that's gonna make things difficult...

huwpascoe commented 4 months ago

I wasn't aware the engine couldn't handle bad casts without crashing. I'm still trying to grasp how the engine's error handling works

Normally when cast fails it will return nullptr, no problem. But since it's invalid memory, anything could happen. I was wrong to call it bad_cast come to think of it, it's undefined behavior.

I guess I could add versions of the function that take PackedScenes or Scripts instead and fix the current function to check if the referenced object has already been freed, but the exception occurs just before the native add_property_editor function is called, so that's gonna make things difficult...

Yeah it's a lot of work. It needn't be a complete fix, a documentation change to make it clear "EditorProperty instances are deleted when the inspector is done with them. Don't reuse them!" would certainly be a magnitude easier to both write and get merged.

godotengine / godot