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

Passing null to a virtual function call causes a null pointer dereference (GDExtension) #1056

Closed
ZNixian opened this issue Mar 1, 2023 · 0 comments · Fixed by #1405
Closed
Labels
bug This has been identified as a bug

Comments

@ZNixian
Copy link

ZNixian commented Mar 1, 2023

I've got a project that overrides ScriptLanguageExtension::_complete_code, one of whose parameters (the third one) is an Object* pointer. Godot passes null to this argument, which causes a fault as Object::get_instance_binding is ultimately called on this pointer:

// Object::get_instance_binding locks a mutex, which is in the zero-page as the object is a null pointer
#0  0x00007ffff7d53b04 in pthread_mutex_lock () from /usr/lib/libc.so.6
#1  0x0000555555a51e2f in __gthread_mutex_lock (__mutex=0x148) at /usr/include/c++/12.2.1/x86_64-pc-linux-gnu/bits/gthr-default.h:749
#2  std::mutex::lock (this=this@entry=0x148) at /usr/include/c++/12.2.1/bits/std_mutex.h:100
// Note this=0x0 - this is called on a null object
#3  0x00005555596d8f29 in Object::get_instance_binding (this=0x0, p_token=0x55555c70c2f0, p_callbacks=0x7ffff49f1690 <godot::Object::___binding_callbacks>) at core/object/object.cpp:1700
// The interface between godot-cpp and the engine, converting the object pointer
#4  0x00005555596ba202 in gdextension_object_get_instance_binding (p_object=<optimized out>, p_token=<optimized out>, p_callbacks=<optimized out>) at core/extension/gdextension_interface.cpp:928
#5  0x00007ffff4904b2b in godot::PtrToArg<godot::Object*>::convert (p_ptr=0x7ffffffface0) at /home/znix/static/programming/c++/gdwren/godot-cpp/include/godot_cpp/core/method_ptrcall.hpp:171
// The result of BIND_VIRTUAL_METHOD and it's associated calls
#6  godot::call_with_ptr_args_retc_helper<WrenLanguage, godot::Dictionary, godot::String const&, godot::String const&, godot::Object*, 0ul, 1ul, 2ul> (p_instance=0x55555fd52380, p_method=&virtual table offset 224, 
    p_args=0x7fffffffad40, r_ret=0x7ffffffface8) at /home/znix/static/programming/c++/gdwren/godot-cpp/include/godot_cpp/core/binder_common.hpp:209
#7  0x00007ffff4901112 in godot::call_with_ptr_args<WrenLanguage, godot::Dictionary, godot::String const&, godot::String const&, godot::Object*> (p_instance=0x55555fd52380, p_method=&virtual table offset 224, p_args=0x7fffffffad40, 
    r_ret=0x7ffffffface8) at /home/znix/static/programming/c++/gdwren/godot-cpp/include/godot_cpp/core/binder_common.hpp:229
#8  0x00007ffff48fb64d in godot::ScriptLanguageExtension::register_virtuals<WrenLanguage, godot::ScriptLanguageExtension>()::{lambda(void*, void const* const*, void*)#24}::operator()(void*, void const* const*, void*) const (
    __closure=0x0, p_instance=0x55555fd52380, p_args=0x7fffffffad40, p_ret=0x7ffffffface8) at /home/znix/static/programming/c++/gdwren/cmake-build-debug/godot-cpp/gen/include/godot_cpp/classes/script_language_extension.hpp:231
#9  0x00007ffff48fb680 in godot::ScriptLanguageExtension::register_virtuals<WrenLanguage, godot::ScriptLanguageExtension>()::{lambda(void*, void const* const*, void*)#24}::_FUN(void*, void const* const*, void*) ()
    at /home/znix/static/programming/c++/gdwren/cmake-build-debug/godot-cpp/gen/include/godot_cpp/classes/script_language_extension.hpp:231
// The in-engine caller
#10 0x00005555591ffc2e in ScriptLanguageExtension::_gdvirtual__complete_code_call<true> (r_ret=..., arg3=0x0, arg2=..., arg1=..., this=0x55555fd523c0) at ./core/object/script_language_extension.h:351
#11 ScriptLanguageExtension::complete_code (this=this@entry=0x55555fd523c0, p_code=..., p_path=..., p_owner=p_owner@entry=0x0, r_options=r_options@entry=0x7fffffffaea0, r_force=@0x7fffffffae9f: false, r_call_hint=...)
    at ./core/object/script_language_extension.h:355

The following diff did fix it, but it feels fairly hacky - I'm not sure if this is the right place for checking for null-ness, or if null is even supposed to be a valid value:

diff --git a/include/godot_cpp/core/method_ptrcall.hpp b/include/godot_cpp/core/method_ptrcall.hpp
index 6b092bb..e917925 100644
--- a/include/godot_cpp/core/method_ptrcall.hpp
+++ b/include/godot_cpp/core/method_ptrcall.hpp
@@ -168,7 +168,10 @@ MAKE_PTRARG_BY_REFERENCE(Variant);
 template <class T>
 struct PtrToArg<T *> {
        _FORCE_INLINE_ static T *convert(const void *p_ptr) {
-               return reinterpret_cast<T *>(godot::internal::gde_interface->object_get_instance_binding(*reinterpret_cast<GDExtensionObjectPtr *>(const_cast<void *>(p_ptr)), godot::internal::token, &T::___binding_callbacks));
+               GDExtensionObjectPtr objPtr = *reinterpret_cast<GDExtensionObjectPtr *>(const_cast<void *>(p_ptr));
+               if (objPtr == nullptr)
+                       return nullptr;
+               return reinterpret_cast<T *>(godot::internal::gde_interface->object_get_instance_binding(objPtr, godot::internal::token, &T::___binding_callbacks));
        }
        typedef Object *EncodeT;
        _FORCE_INLINE_ static void encode(T *p_var, void *p_ptr) {
@@ -179,7 +182,10 @@ struct PtrToArg<T *> {
 template <class T>
 struct PtrToArg<const T *> {
        _FORCE_INLINE_ static const T *convert(const void *p_ptr) {
-               return reinterpret_cast<const T *>(godot::internal::gde_interface->object_get_instance_binding(*reinterpret_cast<GDExtensionObjectPtr *>(const_cast<void *>(p_ptr)), godot::internal::token, &T::___binding_callbacks));
+               GDExtensionObjectPtr objPtr = *reinterpret_cast<GDExtensionObjectPtr *>(const_cast<void *>(p_ptr));
+               if (objPtr == nullptr)
+                       return nullptr;
+               return reinterpret_cast<const T *>(godot::internal::gde_interface->object_get_instance_binding(objPtr, godot::internal::token, &T::___binding_callbacks));
        }
        typedef const Object *EncodeT;
        _FORCE_INLINE_ static void encode(T *p_var, void *p_ptr) {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug
Projects
None yet
2 participants