[GDNative] Crash when method returns reference objects

[touilleMan]

Unlike other Godot objects, Reference objects (like all the Resource for instance) are refcounted.
Hence they are not passed as a simple Object * but as a RefPtr.

This is a trouble for GDNative given it doesn't provide any binding to this RefPtr structure (nor any information about how to handle this).

For instance the RefPtr Object::get_script() const is defined in api.json as:

			{
				"name": "get_script",
				"return_type": "Reference",
				"is_editor": false,
				"is_noscript": false,
				"is_const": true,
				"is_reverse": false,
				"is_virtual": false,
				"has_varargs": false,
				"is_from_script": false,
				"arguments": [
				]
			},

From those information you would expect to write like this:

godot_object *get_script(godot_object p_obj) {
    static godot_method_bind *__methbind__Object__get_script = gdapi10->godot_method_bind_get_method("Object", "get_script");
    godot_object *ret;
    gdapi10->godot_method_bind_ptrcall(
        __methbind__Object__get_script,
        p_myobj,
        NULL,
        &ret
    );
    return ret;
}

However this cause segfault when godot_method_bind_ptrcall tries to store the returned RefPtr into the godot_object *ret :'(

BTW, there is a is_reference flag in the class definition, but Reference is not marked as is_reference (only it children are), though I'm not sure what's the reason, I would guess it's an implementation error from:

godot/modules/gdnative/nativescript/api_generator.cpp

Lines 221 to 224 in 9daaa12

	ClassDB::get_inheriters_from_class("Reference", &inheriters);
	bool is_reference = !!inheriters.find(class_name);
	// @Unclear
	class_api.is_reference = !class_api.is_singleton && is_reference;

To solve this issue, Godot-CPP provide it own custom Ref class:

https://github.com/GodotNativeTools/godot-cpp/blob/81783c6045ac5e3638846f791c7c7841db38aa7e/include/core/Ref.hpp

This solution works because Godot-CPP is in C++ and can then reproduce the layout of the internal Godot RefPtr (though I'm not really sure this is 100% safe if Godot is compiled with a different compiler than the GDNative plugin using Godot-CPP... @karroffel can you enlighten me on this ? ^^ )

Anyway, I guess we should provide a binding to the RefPtr in GDNative and fix the is_reference flag (maybe even provide this is_reference in the method's flags ?), what do you think ?

[karroffel]

The godot-cpp Ref is basically copied over from Godot, but it does not rely on the internal layout of Godot internal types. The godot-rust project deals with references just by calling reference() and unreference(), there should be no need for fancy type magic.

[touilleMan]

@karroffel I'm really not sure how godot-cpp can handle this correctly

The generated code for Object.get_script() looks like this:

Reference *Object::get_script() const {
	return (Reference *) ___godot_icall_Object(___mb.mb_get_script, (const Object *) this);
}

static inline Object *___godot_icall_Object(godot_method_bind *mb, const Object *inst) {
	godot_object *ret;
	ret = nullptr;
	const void *args[1] = {
	};

	godot::api->godot_method_bind_ptrcall(mb, inst->_owner, args, &ret);
	if (ret) {
		return (Object *) godot::nativescript_1_1_api->godot_nativescript_get_instance_binding_data(godot::_RegisterState::language_index, ret);
	}

	return (Object *) ret;
}

so it's pretty similar to what I'm doing:

def get_script(self,):
    
    cdef godot_object *__ret
    [...]
    gdapi10.godot_method_bind_ptrcall(
        __methbind__Object__get_script,
        self._gd_ptr,
        NULL,
        &__ret
    )
    [...]

However this end up with the following stack

0  Reference::unreference (this=0x7fffeef93288) at core/reference.cpp:88
#1  0x000000000140d9b5 in Ref<Reference>::unref (this=0x7fffffff5cf0) at ./core/reference.h:277
#2  0x000000000140da71 in Ref<Reference>::ref (this=0x7fffffff5cf0, p_from=...) at ./core/reference.h:70
#3  0x00000000014da2f3 in Ref<Reference>::operator= (this=0x7fffffff5cf0, p_from=...) at ./core/reference.h:151
#4  0x0000000003a98819 in PtrToArg<RefPtr>::encode (p_val=..., p_ptr=0x7fffffff5cf0) at ./core/reference.h:356
#5  0x0000000003aa4849 in MethodBind0RC<RefPtr>::ptrcall (this=0x56914a0, p_object=0x67e9c60, p_args=0x0, r_ret=0x7fffffff5cf0) at ./core/method_bind.gen.inc:600
#6  0x000000000193cd97 in godot_method_bind_ptrcall (p_method_bind=0x56914a0, p_instance=0x67e9c60, p_args=0x0, p_ret=0x7fffffff5cf0)
    at modules/gdnative/gdnative/gdnative.cpp:69
#7  0x00007fffeeff3120 in __pyx_pf_5godot_8bindings_6Object_48get_script ()
   from /home/emmanuel/projects/godot-python-2-electric-boogaloo/tests/bindings/pythonscript/lib/python3.7/site-packages/godot/bindings.so
[...]

Note the this=0x7fffeef93288 which is an invalid adress, so we end up with memory corruption :'(

[touilleMan]

I think I finally got the issue:
godot-cpp initialize the ret pointer to nullptr (so 0) where my code doesn't

After that, PtrToArg<RefPtr>::encode get called which create a reference object storing the pointer.

Finally this code got called:

godot/core/reference.h

Line 277 in b0eca58

if (reference && reference->unreference()) {

So the pointer is used if it is not null, hence causing the segfault :/

Fortunately, PR #35813 should fix this error prone behavior for Godot 4.0 ;-)