Add error handling for array get method

[arthurlw]

Issue: The Array::get(int p_idx) function in Godot calls operator[](p_idx), which does not perform bounds checking, leading to a crash when accessing an out-of-bounds index.

Solution: Add ERR_FAIL_INDEX_MSG(p_idx, size(), Variant()); in get() and operator[] to prevent invalid memory access and ensure safe error handling.

Fixes #103328

PR adds bounds checking to both Array::get(int p_idx) and Array::operator[](int p_idx) so that invalid accesses are caught and handled with a proper error message. operator[] could probably be reverted to its previous state for performance reasons if necessary.

[Ivorforce]

Hello, and welcome!
I can't believe I overlooked this after skimming the method so often. I also can't believe this potential crash has existed in Godot since it's been made open source (I think?).

Unfortunately, your fix won't work, and I'm pretty sure it won't compile either: ERR_FAIL_INDEX_MSG will only work in void functions. In non-void functions, ERR_FAIL_INDEX_V_MSG would be required.
However, there is a deeper problem to the design of this function: Since the functions return Variant&, i.e. a reference to a Variant, it is impossible to return anything meaningful on index error. A static Variant holding NIL may be appropriate a first glance, but isn't really because it would be mutable and technically susceptible to having its innards exchanged by the caller.

Here's my 2c:
The functions should be changed to return Variant (no ref) instead.
We have another long-standing issue stemming from returning a reference (#93702), and this would make it possible to fix both in an appropriate manner.

[arthurlw]

Thanks for the clarification! Returning a Variant holding NIL makes sense, but wouldn't that mean we won't catch the out of bounds error?

[Ivorforce]

Returning a Variant holding NIL makes sense, but wouldn't that mean we won't catch the out of bounds error?

Good question!
ERR_FAIL_INDEX_V_MSG would still log an error and show it in the Godot console. Note that Godot doesn't use c++ exceptions, so it's impossible to exit a function early without returning a legal value. ERR_FAIL_INDEX_MSG does just the same: It logs an error, and then just returns void!

[arthurlw]

I see. Would changing the return type to a Variant cause issues in other parts of the file that should be noted?

[Ivorforce]

I don't know. It might cause compile issues, and definitely will cause some performance regressions, but I'd have to try it out to know for sure.

[arthurlw]

Alright, I will update the pull request and change the return type to Variant then.

[Ivorforce]

Looks better!
Have you tested that the code compiles and runs?

[Ivorforce]

This check is not required because operator[] already does the check. You'd do it twice if you have it here, too :)

[Ivorforce]

Please make sure you set your code editor to use tabs for indentation. I highly recommend using an IDE to edit the code, because an IDE will adjust the setting automatically for you, depending on existing indentation in the respective file.

[arthurlw]

I was testing the code, and compiling it causes errors in other parts of the codebase, mostly because they expect a reference type instead of a value. For example:

• Line 595 in /core/variant/array.cpp:
  argptrs[0] = &get(i);

I'm not entirely sure on how to proceed given that I don't have much experience with the codebase.

[bruvzg]

[] operators are fully expected to crash if index is wrong, you can't add checks like this since operators should return reference to a persistent value (not Variant() which is a local variable on stack).

I guess we can return a reference to a garbage variable, like read_only do (a global null instance of Variant, but this will make sense only if in's immutable, which is currently not supported by Variant), but this likely will lead to more hard to debug issues, rater than helping.

I would suggest using CRASH_BAD_INDEX(p_index, size()); like CowData do.

[bruvzg]

This is wrong, operator return writable reference, not a copy.

[bruvzg]

Same.

[Ivorforce]

I would suggest using CRASH_BAD_INDEX(p_index, size()); like CowData do.

Array already calls CRASH_BAD_INDEX from CowData on bad index, because it uses CowData internally. Adding it again here wouldn't help, especially to solve #103328.

[] operators are fully expected to crash if index is wrong, you can't add checks like this since operators should return reference to a persistent value (not Variant() which is a local variable on stack).

This may be true in normal circumstances, but the current design can already lead to hard to debug issues (#93702). Personally, I think it's time to rethink how Array and Dictionary interfaces are defined to work better with both GDScript and C++. But this is definitely outside the scope of a small PR to fix, and should be discussed with the core team.

[Ivorforce]

I've only just realized the functions have only been bound to 4.4. They were bound differently than [] subscript is bound, leading to a discrepancy of error handling behavior.

I no longer think this is the right spot to fix the problem. I've added a comment to #103328 with my thoughts.

[bruvzg]

Anyway, I completely missed that #103328 only occurs on .get, not normal array access. This is because get is bound directly while the [] subscript is bound through variant_setget.cpp, where bounds checks are performed. I think a better short-term fix for this issue would be to bind .get through variant_setget.cpp also.

For scripting access, probably yes. Adding a wrapper with bounds check is probably fine (for both Array and Packed*Arrays which already have VARCALL_PACKED_GETTER wrapper, but w/o bounds check).

For C++ caller is responsible for checking bounds, and I do not think any checks needed.

[bruvzg]

Something like

diff --git a/core/variant/variant_call.cpp b/core/variant/variant_call.cpp
index 7521632974..badcf7499f 100644
--- a/core/variant/variant_call.cpp
+++ b/core/variant/variant_call.cpp
@@ -659,6 +659,7 @@ static _FORCE_INLINE_ void vc_ptrcall(void (*method)(T *, P...), void *p_base, c
 
 #define VARCALL_PACKED_GETTER(m_packed_type, m_return_type)                                       \
 	static m_return_type func_##m_packed_type##_get(m_packed_type *p_instance, int64_t p_index) { \
+		ERR_FAIL_INDEX_V(p_index, p_instance->size(), m_return_type());                           \
 		return p_instance->get(p_index);                                                          \
 	}
 
@@ -674,6 +675,11 @@ struct _VariantCall {
 	VARCALL_PACKED_GETTER(PackedVector3Array, Vector3)
 	VARCALL_PACKED_GETTER(PackedVector4Array, Vector4)
 
+	static Variant func_Array_get(Array *p_instance, int64_t p_index) {
+		ERR_FAIL_INDEX_V(p_index, p_instance->size(), Variant());
+		return p_instance->get(p_index);
+	}
+
 	static String func_PackedByteArray_get_string_from_ascii(PackedByteArray *p_instance) {
 		String s;
 		if (p_instance->size() > 0) {
@@ -2354,7 +2360,7 @@ static void _register_variant_builtin_methods_array() {
 	bind_method(Array, clear, sarray(), varray());
 	bind_method(Array, hash, sarray(), varray());
 	bind_method(Array, assign, sarray("array"), varray());
-	bind_method(Array, get, sarray("index"), varray());
+	bind_function(Array, get, _VariantCall::func_Array_get, sarray("index"), varray());
 	bind_method(Array, set, sarray("index", "value"), varray());
 	bind_method(Array, push_back, sarray("value"), varray());
 	bind_method(Array, push_front, sarray("value"), varray());

Opened #103362

[akien-mga]

Superseded by #103362. Thanks for the contribution!