gdext
gdext copied to clipboard
`Array<T>` can hold values that are invalid representations of `T`
Problem
Array<T>
and Array<U>
are indirectly convertible, via Variant
.
Integer conversions
They are even equal if T
and U
map to the same variant type, like integers:
#[itest(focus)]
fn variant_array_conversions_ok() {
let i32_array: Array<i32> = array![1, 2, 4, 300, 500];
let i64_array: Array<i64> = array![1, 2, 4, 300, 500];
let i32_variant = i32_array.to_variant();
let i64_variant = i64_array.to_variant();
assert_eq!(i32_variant, i64_variant);
}
However, this can be exploited to craft arrays with values that are not valid for the static element types.
#[itest(focus)]
fn variant_array_conversions() {
let i32_array: Array<i32> = array![1, 2, 4, 300, 500];
let i8_back: Array<i8> = i32_variant.try_to();
//assert!(i8_back.is_err()); // NOT TRUE
dbg!(&i8_back); // Ok( "[1, 2, 4, 300, 500]" )
let i8_back = i8_back.unwrap();
let num = i8_back.get(4);
// ^ Panics only now: FromGodot::from_variant() failed: `i8` cannot store the given value: 500
}
Class casting
I have not tested yet if a similar issue exists with subclassing, i.e. Array<Gd<T>>
and Array<Gd<U>>
. Although this is probably more rigorously checked, since we have access to the class name as part of the array type -- whereas the exact integer type is lost, it's just VariantType::INT
.
Note that "array upcasting" is not safe, because arrays aren't covariant. If we allow converting Array<Gd<Node3D>>
to Array<Gd<Node>>
, then someone could attempt to store different Node
types in the Node3D
array.
Possible solutions
-
One option is to limit
ArrayElement
to the canonical type, which matches the Godot representation (i.e.i64
for integers). This would then only allow 1 type perVariantType
(except for objects), but it would be more explicit. -
We could iterate through each element and check if conversion is possible. This can be expensive, but we could do it a bit smarter:
- only in cases where there are multiple
ArrayElement
types mapping to the sameVariantType
(e.g. integers) - only if the type is not already the canonical type (storing as
i64
will always succeed) This would mean we would still allow non-canonicalArray<T>
usages, however with a slight performance hit. People who care about this could then fall back to the canonical types.
- only in cases where there are multiple
-
A complex system tracing arrays-in-variants via global metadata (then e.g.
i16
->i32
would be safe). It's not really something I'd like to get into though, as it has a million backdoors and edge cases, and I don't even know if it's possible to identify arrays in GDExtension. -
We accept the lowered type safety and resort to panics when accessing elements.
Variant
conversions are quite robust so they should catch these cases, but it's still not very nice to postpone/hide such logic errors.