`ScriptInstance` implementation is currently unsound

[lilizoey]

In our script instance implementation, we pass a property list and method list to godot depending on slices returned from get_property_list and get_method_list. Godot will then later call free_property_list_func and free_method_list_func on each of these. In these methods we again call get_property_list/get_method_list to find the length of the array, and call Vec::from_raw_parts(ptr, len, len) on the pointer.

This is unsound, since there is nothing stopping the user from implementing get_property_list or get_method_list such that they return slices of different lengths each time. For instance we could do something like:

fn get_property_list(&self) -> &[PropertyInfo] {
  static COUNTER: AtomicUsize = AtomicUsize::new();
  let v = Box::leak(repeat_fn(|| PropertyInfo { .. /* initialize here */ }).take(COUNTER.fetch_add(1)).collect::<Vec<_>>());
  &*v
}

This would make the free callback always try to create a Vec with a too big length, thus UB.

Solutions

In godot 4.3 we can use the new free-callbacks, which pass along the length of the array. So that means we have at least these options:

1. Deprecate and eventually remove ScriptInstance support from all versions prior to 4.3, use new callbacks in 4.3
2. Make a sound implementation in versions prior to 4.3, use the new callbacks in 4.3
3. Make a sound implementation that works in all versions

I think option 1 is best. As making a sound implementation is tricky (you can use a sentinel value for the length, or some kind of header struct trick). And in 4.3 we wont need anything fancy for the length since the length is simply passed to us by Godot.

[lilizoey]

I am gonna work on fixing this after #625 is done. Also @TitanNano since you made the initial implementation, just wondering if you had any input here too.

[TitanNano]

@lilizoey just leaving a quick comment so I don't forget. I am currently working on a patch that will switch ScriptInstance to receive a GdCell instead of self. That change will also include changes to how these lists are handled, as the current approach is no longer compatible.

I will try to share a draft soonish.

[Bromeon]

I am currently working on a patch that will switch ScriptInstance to receive a GdCell instead of self.

Note that GdCell is private, and it would be nice if we could keep it that way. The current way how the mechanism with base() + base_mut() works is reasonably simple, and a user doesn't need to know the intricacies of the reborrow rabbit hole in order to use it correctly.

However, exposing GdCell would burden users with significant complexity. This aspect was often criticized in gdnative, and people ran into issues all the time. Keep in mind that most people are here to develop games/plugins, and it's our responsibility to provide the simplest possible API that achieves this "well enough". See also Philosophy.

So please consider discussing the design in a GitHub issue before spending a lot of time on implementation.