ScriptInstance implementation is currently unsound #647
Description
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:
- Deprecate and eventually remove
ScriptInstancesupport from all versions prior to 4.3, use new callbacks in 4.3 - Make a sound implementation in versions prior to 4.3, use the new callbacks in 4.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.