Dynamic calls with wrong argument count crash the engine (Release mode)

[Bromeon]

Tested versions

Godot 4.3-dev (I used 0f8c955, should now be on master).

System information

Windows 10

Issue description

Using GDScript method Object.call() or GDExtension functions object_method_bind_call and variant_call with an incorrect argument count can lead to irrecoverable engine crashes, in Release mode.

I haven't labeled this issue since I'm not sure if it's an actual bug or by design. Some observations:

• Object::call and Variant::call are used for dynamic calls, using the "variant call" convention (as opposed to "ptrcall"). I was under the assumption that these are checked (and one would use ptrcall for speed), but this might not be true.
• There is a GDExtensionCallError output parameter on both methods in the C API. No indication from docs that this would not be used in Release.
• Godot seems to not provide any other API to do dynamic calls.

If this is indeed by design, then we should clarify the following things:

1. Can reflection still be used in Release builds, and how?
   • In particular, are dynamic checked calls supported or is this feature absent for exported games?
   • What are the differences from Debug mode across different reflection APIs?
2. This behavior should be documented for both GDScript and GDExtension.
   • For the latter, we might especially want to highlight that GDExtensionCallError has no use in Release builds.

Steps to reproduce

Use a Godot 4.2 Release build (e.g. export templates).

Call Node2D::set_position dynamically through the GDExtension variant_call API. Deliberately do not provide the expected argument.

Rust code to visualize, but should be applicable to other bindings:

let node2d = Node2D::new_alloc();
let variant = Variant::from(node2d);
variant.call("set_position", &[]);

This will crash the engine with

ERROR: FATAL: Index p_index = -1 is out of bounds (size() = 0).
   at: CowData<class Variant>::get

and a weird exit code like -2147483645.

Affected code

Problem seems to be here, in line 446 to be exact:

godot/core/variant/binder_common.h

Lines 441 to 448 in 2d0ee20

	const Variant *args[sizeof...(P) == 0 ? 1 : sizeof...(P)]; //avoid zero sized array
	for (int32_t i = 0; i < (int32_t)sizeof...(P); i++) {
	if (i < p_argcount) {
	args[i] = p_args[i];
	} else {
	args[i] = &default_values[i - p_argcount + (dvs - missing)];
	}
	}

With a debugger, I see following values:

i = 0
p_argcount = 0
dvs = 0
missing = 1

Thus, i - p_argcount + (dvs - missing) is -1, which is an invalid index. This crashes at CRASH_BAD_INDEX here:

godot/core/templates/cowdata.h

Lines 157 to 161 in 2d0ee20

	_FORCE_INLINE_ const T &get(int p_index) const {
	CRASH_BAD_INDEX(p_index, size());
	return _ptr[p_index];
	}

Minimal reproduction project (MRP)

N/A

[AThousandShips]

Not knowing the logic behind this, and if it is an oversight or intentional, I'd be in favor of reducing the checks for a lot of things in release builds, as long as the overhead isn't minimal (doing an number conversion is fine I'd say), and also the reduction of unhelpful errors in release builds, unless in areas where user input is still relevant and it's not something that can be trivially worked out in the development phase

[dsnopek]

There's a lot of stuff in the Godot API like this, where there are checks in debug builds and then no checks in release builds, with the idea that you'll find the problem when testing debug builds, so release builds can remove the checks for performance (and consequently incorrect inputs can lead to crashes).

In some recent cases, when fixing crashes in the lead up to 4.2-stable, it was decided that the checks should happen in release too. (I tried for a while to find some examples of these PRs, but couldn't - if anyone remembers these links it would be appreciated!) The thinking was that perhaps Godot is too readily removing checks for "performance", when this may not have been really evaluated.

It would probably be good to try and evaluate this situation holistically, and really determine which checks truly have an important impact on performance, and also where we should or shouldn't remove checks. I think it would be reasonable to identify certain code paths as the "safe, but slow" or "fast, but unsafe" option. We already have "call" versus "validated call" in some places (although, not on Variant or Object, for some reason) and we could maybe have "call" always do the checks, but "validated call" skip them?