Fix use-after-free at shutdown

[mihe]

Fixes #889.

Depends on godotengine/godot#67285.

This PR adds a local static struct instance to the generated singleton getters, which has its destructor called when the extension library unloads. This allows the singleton bindings to be cleared and deallocated through gdn_interface->object_clear_instance_binding before the underlying singletons gets destroyed, thereby preventing the use-after-free described in the issue.

To help with the case where the extension library unload happens after a singleton is destroyed, and thereby making singleton_obj stale, I've also added a *_destroyed bool that gets set in the bindings destructor.

Here is an example of what the generated singleton bindings would look like:

+static bool OS_singleton_destroyed = false;
+
 OS *OS::get_singleton() {
     static GDNativeObjectPtr singleton_obj = internal::gdn_interface->global_get_singleton("OS");
 #ifdef DEBUG_ENABLED
     ERR_FAIL_COND_V(singleton_obj == nullptr, nullptr);
 #endif // DEBUG_ENABLED
     static OS *singleton = reinterpret_cast<OS *>(internal::gdn_interface->object_get_instance_binding(singleton_obj, internal::token, &OS::___binding_callbacks));
+    static struct OS_BindingCleanup {
+        ~OS_BindingCleanup() {
+            if (!OS_singleton_destroyed) {
+                internal::gdn_interface->object_clear_instance_binding(singleton_obj, internal::token);
+            }
+        }
+    } binding_cleanup;
     return singleton;
 }
+
+OS::~OS() {
+    OS_singleton_destroyed = true;
+}

[Zylann]

This might highlight an deeper issue, I wonder:

This problem occurs because an extension library needed to represent a Godot object through its wrappers, library side. And to do so, had to store a pointer to those wrappers inside Godot objects so they can be easily retrieved when passed again.
Here the issue occurred for singletons. But what about any other object? If I understand correctly, any object that "comes into contact" with the library, will have to store wrappers from that library. Like, if at any point I pass a Texture2D to a function of the library, without even storing it, there will be a "godot::Texture2D instance binding" of the library stored on that texture from now on.
If any of those objects get destroyed after the library gets destroyed, the same problem will happen again, just in a way much harder to manage. Is that right?

If so, I guess it confines extensions to the flow of loading on startup, no unload and no hot-reload possible, then unload only when Godot shuts down. And any objects that "came in contact" (and are not singletons, with this PR changes) really should better be freed while the library is still loaded.

What about SceneTree? It is an implementation of MainLoop, therefore has a pretty long lifespan comparable to public singletons. It isn't exposed as singleton, but can still get attached an instance binding when you call get_tree() at least once.

And what if another extension library B was holding a ref on an object that came into contact with library A? That object might get destroyed (refcount) later too from A's perspective.

Each time, the crash cause being that Godot will attempt to call into [unloaded] library A's function to free instance bindings that said library isn't keeping track of.
Or maybe libraries would have to somehow build a big database of all the bindings it created + object IDs, perhaps, so they can all be detached at once when the library unloads (if Object IDs are still valid), so Godot doesn't have to do anything and no use-after-free can possibly happen? It's not pretty and adds to the overhead, but I'm not sure how else this can be dealt with.
(It's late here)

[mihe]

(I'm fairly new to the Godot codebase, so take all this with a grain of salt.)

If any of those objects get destroyed after the library gets destroyed, the same problem will happen again, just in a way much harder to manage. Is that right?

I would imagine so, yes.

it confines extensions to the flow of loading on startup, no unload and no hot-reload possible, then unload only when Godot shuts down

Yes, the thing that does throw a spanner into this whole presumption is the exposed bindings of NativeExtensionManager::unload_extension and I guess eventually NativeExtensionManager::reload_extension. They present themselves as a bit of a footgun with the way instance bindings are set up currently. Without those I guess it would be safe to assume that extensions outlast most/all objects that aren't engine singletons.

What about SceneTree?

As far as I can tell, it gets deleted pretty early in the cleanup, before extensions are unloaded.

And what if another extension library B was holding a ref on an object that came into contact with library A?

So long as the object itself doesn't outlast either of the two libraries you'd be fine I suppose. I imagine most types that could be shared between the two (assuming the libraries are always communicating through the engine) would end up living in some sort of manager within the engine that gets deleted before the extensions do. Even stuff like custom singletons registered through Engine::register_singleton would presumably be cleaned up and unregistered within the extension's terminator, which should be fine, since unloading happens later down the line.

Again, I'm fairly new to all this, and I haven't looked too deeply into it, so I'd be happy to have someone confirm/deny the statements here.

[Bromeon]

Yes, the thing that does throw a spanner into this whole presumption is the exposed bindings of NativeExtensionManager::unload_extension and I guess eventually NativeExtensionManager::reload_extension. They present themselves as a bit of a footgun with the way instance bindings are set up currently. Without those I guess it would be safe to assume that extensions outlast most/all objects that aren't engine singletons.

(emphasis mine)

Note however that without extension unloading/reloading, the main workflow of game development through extensions is obstructed, see godotengine/godot#66231. Seems like getting hot-reloading working reliable might be a huge endeavor, but to me that's even more of a reason to start sooner rather than later 😉

So, regarding the problem here:

If I understand correctly, any object that "comes into contact" with the library, will have to store wrappers from that library. Like, if at any point I pass a Texture2D to a function of the library, without even storing it, there will be a "godot::Texture2D instance binding" of the library stored on that texture from now on.

I'm not very familiar with the inner workings of godot-cpp, could you explain why the library permanently stores this instance binding inside the texture? As far as I know, this is not required to access the texture via GDExtension?

Or maybe libraries would have to somehow build a big database of all the bindings it created + object IDs, perhaps, so they can all be detached at once when the library unloads (if Object IDs are still valid), so Godot doesn't have to do anything and no use-after-free can possibly happen? It's not pretty and adds to the overhead, but I'm not sure how else this can be dealt with.

Could this be implemented on Godot side at all, or is it just the extension library that knows about these bindings?
Godot would probably itself need to maintain a map of library to all bindings, but at least this might be reusable across all extensions?

[MGilleronFJ]

I'm not very familiar with the inner workings of godot-cpp, could you explain why the library permanently stores this instance binding inside the texture? As far as I know, this is not required to access the texture via GDExtension?

GodotCpp wraps object pointers into classes that have the same API as the real class, such that they act the same as if Godot had a C++ API. But instances of these wrappers are not the instance, the real object is instead stored in a private member, via a C-style opaque pointer, because GDExtension's API is C. The wrapper is not absolutely required, but then all you have is a C-style pointer.
Those wrappers are "instance bindings". If they were not cached into the objects, the "C++ class instance" that GodotCpp gives you would have to be re-created every time it gets passed to any of your functions. Performance would get worse due to more frequent allocations to create the wrapper anytime you pass or obtain an instance of said class, and it also invalidates its pointer, which leads to confusing situations (like for example you wouldn't be able to cache a node and compare it with another pointer obtained from elsewhere since the wrapper would be different everytime). There must also be a place to store the data added by custom classes inheriting Godot ones. Something similar was done for C# support, to cache the C#-side instance.

And what if another extension library B was holding a ref on an object that came into contact with library A?

So long as the object itself doesn't outlast either of the two libraries you'd be fine I suppose. I imagine most types that could be shared between the two (assuming the libraries are always communicating through the engine) would end up living in some sort of manager within the engine that gets deleted before the extensions do.

That's not always the case though, because of RefCounted classes. There is no "manager". The two libraries don't even need to communicate directly or know each other. From what I described above with how instance bindings are cached, a core Godot class inheriting RefCounted could have been passed at some point to both library A and B. It could be that library A holds the last reference to it when it unloads (from a singleton for example, or built-in default assets like themes, icons...). If that unload happens after B, then the same issue happens.
The assumption all objects are cleared before any library unloads is very fragile, or doesn't hold.
Which is why I ended up thinking of creating such "manager" on library side for instance bindings of non-custom classes. Because without that you'd have to run after every single object this library "received" that could possibly remain.

Could this be implemented on Godot side at all, or is it just the extension library that knows about these bindings?
Godot would probably itself need to maintain a map of library to all bindings, but at least this might be reusable across all extensions?

Godot might be able to do it. But doing it on library side might be more direct. It depends how implementing that turns out to be. Not every library needs it also, someone could use C directly, or a language that happens to not need "instance bindings". I was thinking of a simple pool where each instance binding pointer is stored in a growing array, and classes would keep track of the index if they need to unregister it. And when the library unloads, that array is iterated and all valid slots would call the function to free those instance bindings. That would also have to be protected by mutex the first time instance bindings are created, and when they are removed.

An alternative would be to exploit de-initialization levels: after the very last level, Godot could enforce that all libraries have been de-initialized and every Object has been destroyed, so we can unload each library. Although such approach doesn't allow unloading or reloading just one library at runtime.