Removing an EditorPlugin results in a use-after-free

[mihe]

Godot version

4.1-stable

godot-cpp version

a9209ce

System information

Windows 11 (10.0.22621)

Issue description

When adding an EditorPlugin through the EditorPlugins::add_by_type method, and then removing it in the terminator using EditorPlugins::remove_by_type, you will run into a use-after-free, because the EditorNode instance on the engine side of things (which is responsible for hosting the editor plugins) has already been deallocated (at the end of OS_*::run) by the time the extension library terminators run in Main::cleanup.

Reverting #1138 and omitting any EditorPlugins::remove_by_type seems to resolve the use-after-free.

Steps to reproduce

1. Run the editor with a memory debugger like Application Verifier (or presumably Valgrind)
2. Load the reproduction project
3. Close the editor
4. Observe access violation crash

Minimal reproduction project

MRP_EditorPlugin.zip

[mihe]

I just realized ripping the test project out like that probably doesn't make for a very good MRP, seeing as how it relies on the parent SConstruct file, meaning the SConstruct file in the root of this repo.

Anyway, just place the MRP project alongside the test project, in its own folder, or just copy-paste register_types.cpp from the MRP into the test project, whichever you prefer.

[dsnopek]

Thanks!

I ran Godot with Valgrind using one of my GDExtension projects that adds an editor plugin, and got this during shutdown:

==501195== Invalid read of size 1
==501195==    at 0x4236EF3: EditorNode::remove_extension_editor_plugin(StringName const&) (editor_node.cpp:3266)
==501195==    by 0x7A97A89: GDExtensionEditorPlugins::remove_extension_class(StringName const&) (gdextension.cpp:693)
==501195==    by 0x7AA380A: gdextension_editor_remove_plugin(void const*) (gdextension_interface.cpp:1084)
==501195==    by 0x1F485FB6: godot::EditorPlugins::remove_plugin_class(godot::StringName const&) (editor_plugin.cpp:49)
==501195==    by 0x1F46B393: void godot::EditorPlugins::remove_by_type<SGCollisionShape2DEditorPlugin>() (editor_plugin.hpp:270)
==501195==    by 0x1F458111: unregister_sg_physics_2d_extension_types(godot::ModuleInitializationLevel) (init_gdextension.cpp:117)
==501195==    by 0x1F485A30: godot::GDExtensionBinding::deinitialize_level(void*, GDExtensionInitializationLevel) (godot.cpp:402)
==501195==    by 0x7A957D6: GDExtension::deinitialize_library(GDExtension::InitializationLevel) (gdextension.cpp:512)
==501195==    by 0x7AAA486: GDExtensionManager::deinitialize_extensions(GDExtension::InitializationLevel) (gdextension_manager.cpp:132)
==501195==    by 0x2C254C9: Main::cleanup(bool) (main.cpp:3593)
==501195==    by 0x2B884F1: main (godot_linuxbsd.cpp:76)
==501195==  Address 0x40b4e8ec is 1,948 bytes inside an unallocated block of size 9,440 in arena "client"

I just posted Godot PR godotengine/godot#79492 that should prevent GDExtension from trying to access EditorNode after it's been freed.

I personally think Godot (rather than godot-cpp) is the right place to fix it, because it is theoretically possible (although, basically never done at present because in most cases it would break things) to unload a GDExtension at runtime, and in that case we'd still want editor plugins to be automatically removed. Rather than increasing the number of things that break if a GDExtension is unloaded :-) I'd like to keep this accounted for.