godotengine / godot

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

Executing `PluginScript.new().instance_has(BoxShape.new())` crashes Godot #46020

Closed qarmin closed 3 years ago

qarmin commented 3 years ago

Godot version: Godot 3.2.4 rc 2

Issue description: Executing

PluginScript.new().instance_has(BoxShape.new())

crashes with backtrace

modules/gdnative/pluginscript/pluginscript_script.cpp:207:17: runtime error: member call on null pointer of type 'struct PluginScriptLanguage'
modules/gdnative/pluginscript/pluginscript_script.cpp:207:17: runtime error: member access within null pointer of type 'struct PluginScriptLanguage'
handle_crash: Program crashed with signal 11
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] godots() [0x16f757a] (/mnt/Miecz/godot3.2/platform/x11/crash_handler_x11.cpp:54)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x46210) [0x7f81438fe210] (??:0)
[3] PluginScript::instance_has(Object const*) const (/mnt/Miecz/godot3.2/modules/gdnative/pluginscript/pluginscript_script.cpp:207)
[4] MethodBind1RC<bool, Object const*>::call(Object*, Variant const**, int, Variant::CallError&) (/mnt/Miecz/godot3.2/./core/method_bind.gen.inc:1333 (discriminator 8))
[5] Object::call(StringName const&, Variant const**, int, Variant::CallError&) (/mnt/Miecz/godot3.2/core/object.cpp:919 (discriminator 1))
[6] Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) (/mnt/Miecz/godot3.2/core/variant_call.cpp:1129 (discriminator 1))
[7] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) (/mnt/Miecz/godot3.2/modules/gdscript/gdscript_function.cpp:1089)
[8] GDScriptInstance::_ml_call_reversed(GDScript*, StringName const&, Variant const**, int) (/mnt/Miecz/godot3.2/modules/gdscript/gdscript.cpp:1269)
[9] GDScriptInstance::call_multilevel_reversed(StringName const&, Variant const**, int) (/mnt/Miecz/godot3.2/modules/gdscript/gdscript.cpp:1278)
[10] Node::_notification(int) (/mnt/Miecz/godot3.2/scene/main/node.cpp:152)
[11] Node::_notificationv(int, bool) (/mnt/Miecz/godot3.2/./scene/main/node.h:46 (discriminator 14))
[12] CanvasItem::_notificationv(int, bool) (/mnt/Miecz/godot3.2/./scene/2d/canvas_item.h:166 (discriminator 3))
[13] Node2D::_notificationv(int, bool) (/mnt/Miecz/godot3.2/./scene/2d/node_2d.h:38 (discriminator 3))
[14] Object::notification(int, bool) (/mnt/Miecz/godot3.2/core/object.cpp:931)
[15] Node::_propagate_ready() (/mnt/Miecz/godot3.2/scene/main/node.cpp:197)
[16] Node::_propagate_ready() (/mnt/Miecz/godot3.2/scene/main/node.cpp:186 (discriminator 2))
[17] Node::_set_tree(SceneTree*) (/mnt/Miecz/godot3.2/scene/main/node.cpp:2560)
[18] SceneTree::init() (/mnt/Miecz/godot3.2/scene/main/scene_tree.cpp:464)
[19] OS_X11::run() (/mnt/Miecz/godot3.2/platform/x11/os_x11.cpp:3628)
[20] godots(main+0x331) [0x16ee467] (/mnt/Miecz/godot3.2/platform/x11/godot_x11.cpp:57)
[21] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0x7f81438df0b3] (??:0)
[22] godots(_start+0x2e) [0x16ee07e] (??:?)
pfertyk commented 3 years ago

I'd like to submit a fix. The PR is to master, but it should be easy to cherry-pick to 3.x. The only difference is that BoxShape was renamed to BoxShape3D, but that doesn't affect the fix :)

The problem was that _language is null when we invoke instance_has. I've noticed that reload method checks if _language was properly initialized and throws an error otherwise, so I've repeated that solution in instance_has.

My fix will prevent Godot from crashing. However, this does not solve the problem of _language not being initialized. Being a fresh Godot contributor, I'm not sure where it should happen. The value is assigned in init but I don't know whose responsability is it to call init :) Do you think my fix is enough or should I look into it and find a way to initialize _language correctly?

akien-mga commented 3 years ago

@pfertyk Your fix seems correct. PluginScript assumes that a call to init should happen first before the rest of the API can be initialized, so using "reverse asserts" like we have with ERR_FAIL_COND is the canonical way to prevent calling such APIs before they're initialized properly.