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

Use-after-free when closing editor while using GDExtension that has registered classes #95306

Closed
mihe opened this issue Aug 9, 2024 · 1 comment · Fixed by #95307
Closed

Comments

@mihe
Copy link
Contributor

mihe commented Aug 9, 2024

Tested versions

Reproducible in: 4.3.rc [739019e]

System information

Windows 11 (10.0.22631)

Issue description

As far as I can tell this looks to be a bug/regression introduced in #83747. I only noticed it when running the editor under Application Verifier on Windows, which similar to Valgrind lets you know if a use-after-free has occurred, among other things.

I haven't noticed any crash when running normally, but I suppose that could maybe happen, given the somewhat random nature of a use-after-free.

Basically, when closing the editor while a GDExtension (using godot-cpp) is loaded, that has itself registered a class with ClassDB, it will seemingly trigger a use-after-free here:

godot/editor/editor_help.cpp

Lines 2879 to 2881 in 739019e

if (doc && doc->has_doc(p_class)) {
doc->remove_doc(p_class);
}

... as doc will already have been freed at that point, but not zeroed out.

The flow goes something like this:

  1. Shutdown of the editor is initiated
  2. SceneTree::finalize is called
  3. EditorNode is deleted
  4. EditorNode::~EditorNode calls EditorHelp::cleanup_doc
  5. ⚠️ EditorHelp::cleanup_doc deletes EditorHelp::doc (but doesn't zero it out)
  6. Main::cleanup is called (later down the line)
  7. Main::cleanup calls GDExtensionManager::deinitialize_extensions (with INITIALIZATION_LEVEL_EDITOR in my case)
  8. godot-cpp calls its ClassDB::deinitialize
  9. ClassDB::deinitialize calls into Godot's GDExtension::_unregister_extension_class
  10. GDExtension::_unregister_extension_class calls GDExtensionEditorHelp::remove_class
  11. GDExtensionEditorHelp::remove_class calls EditorHelp::remove_class
  12. Use-after-free occurs

Simply adding a doc = nullptr in EditorHelp::cleanup_doc seems to resolve the issue.

Steps to reproduce

N/A

Minimal reproduction project (MRP)

I'd be happy to provide an MRP, but I'm fairly sure this should be reproducible in pretty much any godot-cpp project.

@mihe
Copy link
Contributor Author

mihe commented Aug 9, 2024

I put up a tentative fix in #95307.

CC @dsnopek @Riteo

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

Successfully merging a pull request may close this issue.

2 participants