godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.99k stars 21.16k forks source link

Crash when opening a project containing a script that preloads a GDNative class #15793

Closed Zylann closed 6 years ago

Zylann commented 6 years ago

Godot 3.0 master

Just tried to open the project, and it crashed with the following call stack. The script doesn't even references that class, as there is no .gdns referring to it, but it still gets registered and crashes in the middle of registration.

Edit: it seems to happen as soon as I try to register a signal.

image

akien-mga commented 6 years ago

CC @karroffel

Zylann commented 6 years ago

It may not be the cause of the bug, but by reading the code, I found that the number of arguments of my signal is indeed zero. But... It means that this following code in C++ bindings runs down to an equivalent of malloc(0):

template<class T>
void register_signal(String name, Dictionary args = Dictionary())
{
    godot_signal signal = {};
    signal.name = *(godot_string *)&name;
    signal.num_args = args.size(); // <--- THIS IS ZERO
    signal.num_default_args = 0;

    // HERE MALLOC(0)
    signal.args = (godot_signal_argument*) godot::api->godot_alloc(sizeof(godot_signal_argument) * signal.num_args);

Which, according to http://www.cplusplus.com/reference/cstdlib/malloc/, returns a pointer that should not be referenced because the return of malloc will be implementation-dependent. Unless DEBUG_ENABLED is defined in Godot, which "luckily" adds 16 bytes to the array. Is this macro defined in debug builds?

Which also leads me to a GDNative question:

typedef struct {
    godot_string name;
    int num_args;
    godot_signal_argument *args;
    int num_default_args;
    godot_variant *default_args;
} godot_signal;

What should be the value of args if the signal has no arguments? NULL? Also, which parts of that struct should be freed after calling godot_nativescript_register_signal? And... why does it take a godot_string for the name, while method registrations only need a const char*?

Zylann commented 6 years ago

I nailed it further. I wrote my own register_signal function which does nothing, and I found this:

Crashes:

template<class T>
void my_register_signal(const wchar_t *name, Dictionary args = Dictionary())
{

}

Doesn't crash:

template<class T>
void my_register_signal(const wchar_t *name)
{
    Dictionary args = Dictionary();
}

image

Considering that Dictionary has neither copy constructor nor assignment operator... bad things may happen if any copy is done. That case above is an implicit copy from the calling scope, I get a crash too if I attempt an explicit copy. So for that part, it's a C++ bindings issue, I'll see if fixing that solves the whole thing...

Edit: ok, that was it. Will do a PR on C++ bindings.