godotengine / godot

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

Calling gdnative function with no arguments when the C++ signature has arguments causes crash/backtrace. #22533

Closed spongeboburu closed 5 years ago

spongeboburu commented 6 years ago

Godot version: 3.1 master

OS/device including version: Fedora 28, 64-bit

Issue description: If I call a gdnative plugin from gdscript and the C++ signature has arguments, but none are specified in the calling gdscript then the scene crashes and the editor dumps a backtrace:

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] /lib64/libc.so.6(+0x36f30) [0x7f65c1355f30] (??:0)
[2] godot_variant godot::__wrapped_method<godot::gdinterpolation, void, int>(void*, void*, void*, int, godot_variant**) (??:0)
[3] NativeScriptInstance::call(StringName const&, Variant const**, int, Variant::CallError&) ($HOME/projects/godot-spongeboburu/modules/gdnative/nativescript/nativescript.cpp:736)
[4] Object::call(StringName const&, Variant const**, int, Variant::CallError&) ($HOME/projects/godot-spongeboburu/core/object.cpp:947 (discriminator 1))
[5] Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) ($HOME/projects/godot-spongeboburu/core/variant_call.cpp:1049 (discriminator 1))
[6] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) ($HOME/projects/godot-spongeboburu/modules/gdscript/gdscript.h:427)
[7] GDScriptInstance::_ml_call_reversed(GDScript*, StringName const&, Variant const**, int) ($HOME/projects/godot-spongeboburu/modules/gdscript/gdscript.cpp:1189)
[8] GDScriptInstance::call_multilevel_reversed(StringName const&, Variant const**, int) ($HOME/projects/godot-spongeboburu/modules/gdscript/gdscript.cpp:1198)
[9] Node::_notification(int) ($HOME/projects/godot-spongeboburu/scene/main/node.cpp:139)
[10] Node::_notificationv(int, bool) ($HOME/projects/godot-spongeboburu/./scene/main/node.h:46 (discriminator 14))
[11] CanvasItem::_notificationv(int, bool) ($HOME/projects/godot-spongeboburu/./scene/2d/canvas_item.h:140 (discriminator 1))
[12] Node2D::_notificationv(int, bool) ($HOME/projects/godot-spongeboburu/./scene/2d/node_2d.h:38 (discriminator 1))
[13] Sprite::_notificationv(int, bool) ($HOME/projects/godot-spongeboburu/scene/2d/sprite.h:39 (discriminator 1))
[14] Object::notification(int, bool) ($HOME/projects/godot-spongeboburu/core/object.cpp:980)
[15] Node::_propagate_ready() ($HOME/projects/godot-spongeboburu/scene/main/node.cpp:184)
[16] Node::_propagate_ready() ($HOME/projects/godot-spongeboburu/scene/main/node.cpp:173)
[17] Node::_propagate_ready() ($HOME/projects/godot-spongeboburu/scene/main/node.cpp:173)
[18] Node::_set_tree(SceneTree*) ($HOME/projects/godot-spongeboburu/scene/main/node.cpp:2480)
[19] SceneTree::init() ($HOME/projects/godot-spongeboburu/scene/main/scene_tree.cpp:457)
[20] OS_X11::run() ($HOME/projects/godot-spongeboburu/platform/x11/os_x11.cpp:2817)
[21] $HOME/projects/godot-spongeboburu/bin/godot.x11.tools.64(main+0xb5) [0xefeedb] ($HOME/projects/godot-spongeboburu/platform/x11/godot_x11.cpp:56)
[22] /lib64/libc.so.6(__libc_start_main+0xeb) [0x7f65c134211b] (??:0)
[23] $HOME/projects/godot-spongeboburu/bin/godot.x11.tools.64(_start+0x2a) [0xefed6a] (??:?)
-- END OF BACKTRACE --

Im thinking the expected behavior is to show an error about the minimum required arguments.

Also as a side note there is no upper limit check either. So in C++ (int arg1, int arg2) and gdscript its a valid call to do myunc(1,2,3,4,5,6,7,8,9) and so on.

Although it is my first time trying gdnative and my C++ is not very good, so there might be something obvious I missed.

Steps to reproduce: C++ signature for your gdtype subclass:

void test(int arg1);

Try to call it from gdscript:

var MyPlugin = load("res://bin/myplugin.gdns")
var my_plugin = MyPlugin.new()
my_plugin.test(1, 2, 3, 4, 5) # shouldn't work
my_plugin.test() # would like an error, but crashes instead
akien-mga commented 6 years ago

CC @karroffel

karroffel commented 6 years ago

In theory you can issue an error, it's not exposed to NativeScript yet AFAIK.

The problem here is in the C++ bindings, which don't do any validation of arguments yet. Other bindings such as the Rust bindings already do it, so this is not a GDNative/NativeScript problem but a library side validation problem.

There already is an issue about that and it shouldn't be too hard to actually implement it, but nobody got around to doing it yet. (Zylann did at one point, but the bindings moved on from then)

spongeboburu commented 6 years ago

@karroffel

Should we mark this as a duplicate with the issue you are referring to and close this one?

akien-mga commented 5 years ago

There already is an issue about that and it shouldn't be too hard to actually implement it, but nobody got around to doing it yet.

akien-mga commented 5 years ago

I guess @karroffel meant GodotNativeTools/godot-cpp#209, so closing as a dulicate.