godotengine/godot

GDNative terminate too early if the same shared library been loaded multiple times

Opened this issue · 3 comments

Godot version:
3.0 ~ master

OS/device including version:
OS and device independent

Issue description:
When a shared library is loaded multiple times, and load_once = true, godot will crash at first terminate call.

Steps to reproduce:

  1. create a gdnative library
  2. create a .gdnlib
  3. create a .gdns, and attach to a node
  4. create a .gd and attach to a node. in _init, load, initialize and terminate gdnative library
  5. run the project

godot will be crashed by the terminate call in .gd.

Minimal reproduction project:
simple.zip

The fix
this issue is caused by these lines

// modules/gdnative/gdnative.cpp
bool GDNative::initialize() {
...
        if (library->should_load_once()) {
                if (GDNativeLibrary::loaded_libraries->has(lib_path)) {
                        // already loaded. Don't load again.
                        // copy some of the stuff instead
                        this->native_handle = (*GDNativeLibrary::loaded_libraries)[lib_path][0]->native_handle;
                        initialized = true;
                        return true;
                }
        }
...
}

Add one line to fix this problem

                        this->native_handle = (*GDNativeLibrary::loaded_libraries)[lib_path][0]->native_handle;
                        initialized = true;
                        GDNativeLibrary::loaded_libraries[lib_path].push_back(Ref<GDNative>(this));
                        return true;

@Calinou will the fix be merged into master? Without the one line fix, the code block bellow will run terminate no matter the same shared library been loaded how many times (gdnatives->size() > 1 never true)

// modules/gdnative/gdnative.cpp
bool GDNative::terminate() {
...
        if (library->should_load_once()) {
                Vector<Ref<GDNative> > *gdnatives = &GDNativeLibrary::loaded_libraries[library->get_current_library_path()];
                if (gdnatives->size() > 1) {
                        // there are other GDNative's still using this library, so we actually don't terminate
                        gdnatives->erase(Ref<GDNative>(this));
                        initialized = false;
                        return true;
                } else if (gdnatives->size() == 1) {
                        // we're the last one, terminate!
                        gdnatives->clear();
                        // whew this looks scary, but all it does is remove the entry completely
                        GDNativeLibrary::loaded_libraries.erase(GDNativeLibrary::loaded_libraries.find(library->get_current_library_path()));
                }
        }
...

The recommended workflow would be for you to make a Pull Request with the suggested fix. Then we can review it with GDNative contributors to confirm that it's good, or ask for changes.

I see