`Variant::iter_get` on empty `Array` crashes in release, but no errors reported in debug

allenwp commented 4 days ago

Godot version

v4.3.stable.official [77dcf97d8]

godot-cpp version

4.3 56571dc584ee7d14919996c6d58fb5b35e64af63

System information

Godot v4.3.stable - Windows 10.0.22631 - Vulkan (Mobile) - dedicated NVIDIA GeForce GTX 980 Ti (NVIDIA; 32.0.15.6603) - 13th Gen Intel(R) Core(TM) i7-13700K (24 Threads)

Issue description

Calling Variant::iter_get on an empty Array causes this crash, but only in release builds of the godot-cpp GDExtension:

Godot Engine v4.3.stable.official.77dcf97d8 - https://godotengine.org
Vulkan 1.3.289 - Forward Mobile - Using Device #0: NVIDIA - NVIDIA GeForce GTX 980 Ti

ERROR: FATAL: Index p_index = 0 is out of bounds (size() = 0).
   at: get (./core/templates/cowdata.h:205)

I cannot reproduce this release-only behaviour on builds of the Godot editor, so it appears to only behave this way on release builds of GDExtension.

This issue also happens with other types of Array, such as PackedVector4Array. This makes me suspect that this sort of behaviour could exist elsewhere in GDExtension...

Steps to reproduce

Write the following code:

Variant arrayAsVariant = Array();

Variant iterator;
bool iter_valid;
arrayAsVariant.iter_init(iterator, iter_valid);
Variant value = arrayAsVariant.iter_get(iterator, iter_valid);

Compile with scons dev_build=yes and note that there are no issues, warnings, or errors and value is null.

Compile with scons target=template_release and note that the error and crash occur.

Minimal reproduction project

Here is an example GDExtension: release-only-gdextension-crash.zip

Copy over godot-cpp 4.3 branch into godot-cpp folder
Run both scons dev_build=yes and scons target=template_release to generate GDExtension DLL files
Open the godot project in the demo folder
Export both debug and release builds of the game

Here is my compiler info: Using SCons-detected MSVC version 14.3, arch x86_64

dsnopek commented 1 day ago

Thanks!

I agree that it would make sense for there to be a warning or something when Godot protects against a crash in a debug build.

However, it looks like it's Godot (not godot-cpp) that's doing the protecting. In core/variant/variant_setget.cpp:

        case ARRAY: {
            const Array *arr = reinterpret_cast<const Array *>(_data._mem);
            int idx = r_iter;
#ifdef DEBUG_ENABLED
            if (idx < 0 || idx >= arr->size()) {
                r_valid = false;
                return Variant();
            }
#endif
            return arr->get(idx);
        } break;

So, perhaps the warning should be added to Godot, rather than godot-cpp as well?

We could certainly add something to godot-cpp (also within a DEBUG_ENABLED), but Godot module code can make this same mistake too.

allenwp commented 1 day ago

Oh, wow, from looking at that source file, there's a whole bunch of code in there that appears to behave differently in debug mode than release mode without any warnings or errors! 😬

But... this confuses me because when I tried copying this snippet of code into the editor source files and built the editor in release mode, I got the debug behaviour... Did I make a mistake when doing this test?

dsnopek commented 1 day ago

But... this confuses me because when I tried copying this snippet of code into the editor source files and built the editor in release mode, I got the debug behaviour... Did I make a mistake when doing this test?

The editor can't be built in release mode, it's always in debug mode.

allenwp commented 1 day ago

The editor can't be built in release mode, it's always in debug mode.

🤯 I had no idea! There's clearly more for me to learn about this subject.

In this case, I'll open an issue in godotengine/godot instead.

godotengine / godot-cpp