gdext icon indicating copy to clipboard operation
gdext copied to clipboard

`Array<T>` can hold values that are invalid representations of `T`

Open Bromeon opened this issue 7 months ago • 2 comments

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

  1. 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 per VariantType (except for objects), but it would be more explicit.

  2. 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 same VariantType (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-canonical Array<T> usages, however with a slight performance hit. People who care about this could then fall back to the canonical types.
  3. 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.

  4. 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.

Bromeon avatar Jul 21 '24 21:07 Bromeon