Merge #795

795: Remove or change APIs causing UB r=Bromeon a=Bromeon

Fixes #790 

Changes in commit message

Co-authored-by: Jan Haller <[email protected]>

Author: bors[bot]

examples/scene_create/src/lib.rs

@@ -141,10 +141,14 @@ fn update_panel(owner: &Spatial, num_children: i64) {
 
         // Put the Node
         let mut as_variant = Variant::from_object(panel_node);
-        match as_variant.call(
-            "set_num_children",
-            &[Variant::from_u64(num_children as u64)],
-        ) {
+        let result = unsafe {
+            as_variant.call(
+                "set_num_children",
+                &[Variant::from_u64(num_children as u64)],
+            )
+        };
+
+        match result {
             Ok(_) => godot_print!("Called Panel OK."),
             Err(_) => godot_print!("Error calling Panel"),
         }

gdnative-core/src/core_types/dictionary.rs

@@ -1,8 +1,6 @@
 use std::iter::{Extend, FromIterator};
 use std::marker::PhantomData;
 
-use gdnative_impl_proc_macros::doc_variant_collection_safety;
-
 use crate::core_types::GodotString;
 use crate::private::get_api;
 use crate::sys;
@@ -195,11 +193,6 @@ impl<Access: ThreadAccess> Dictionary<Access> {
         unsafe { VariantArray::<Unique>::from_sys((get_api().godot_dictionary_values)(self.sys())) }
     }
 
-    #[inline]
-    pub fn get_next(&self, key: &Variant) -> &Variant {
-        unsafe { Variant::cast_ref((get_api().godot_dictionary_next)(self.sys(), key.sys())) }
-    }
-
     /// Return a hashed i32 value representing the dictionary's contents.
     #[inline]
     pub fn hash(&self) -> i32 {
@@ -264,41 +257,6 @@ impl Dictionary<Shared> {
     pub fn new_shared() -> Self {
         Dictionary::<Unique>::new().into_shared()
     }
-
-    /// Inserts or updates the value of the element corresponding to the key.
-    ///
-    #[doc_variant_collection_safety]
-    #[inline]
-    pub unsafe fn insert<K, V>(&self, key: K, val: V)
-    where
-        K: OwnedToVariant + ToVariantEq,
-        V: OwnedToVariant,
-    {
-        (get_api().godot_dictionary_set)(
-            self.sys_mut(),
-            key.owned_to_variant().sys(),
-            val.owned_to_variant().sys(),
-        )
-    }
-
-    /// Erase a key-value pair in the `Dictionary` by the specified key.
-    ///
-    #[doc_variant_collection_safety]
-    #[inline]
-    pub unsafe fn erase<K>(&self, key: K)
-    where
-        K: ToVariant + ToVariantEq,
-    {
-        (get_api().godot_dictionary_erase)(self.sys_mut(), key.to_variant().sys())
-    }
-
-    /// Clears the `Dictionary`, removing all key-value pairs.
-    ///
-    #[doc_variant_collection_safety]
-    #[inline]
-    pub unsafe fn clear(&self) {
-        (get_api().godot_dictionary_clear)(self.sys_mut())
-    }
 }
 
 /// Operations allowed on Dictionaries that may only be shared on the current thread.

gdnative-core/src/core_types/mod.rs

@@ -1,4 +1,4 @@
-//! Types that represent core data types of Godot.
+//! Types that represent [core types](https://docs.godotengine.org/en/stable/development/cpp/core_types.html) of Godot.
 //!
 //! In contrast to generated Godot class types from the `api` module, the types in here are hand-written in idiomatic Rust and
 //! are the counterparts to built-in types in GDScript.

gdnative-core/src/core_types/variant.rs

@@ -792,27 +792,31 @@ impl Variant {
         unsafe { (get_api().godot_variant_has_method)(&self.0, &method.0) }
     }
 
+    /// Invokes a method on the held object.
+    ///
+    /// # Safety
+    /// This method may invoke [Object::call()] internally, which is unsafe, as it allows
+    /// execution of arbitrary code (including user-defined code in GDScript or unsafe Rust).
     #[inline]
-    pub fn call(
+    pub unsafe fn call(
         &mut self,
         method: impl Into<GodotString>,
         args: &[Variant],
     ) -> Result<Variant, CallError> {
         let method = method.into();
-        unsafe {
-            let api = get_api();
-            let mut err = sys::godot_variant_call_error::default();
-            let mut arg_refs = args.iter().map(Variant::sys).collect::<Vec<_>>();
-            let variant = (api.godot_variant_call)(
-                &mut self.0,
-                &method.0,
-                arg_refs.as_mut_ptr(),
-                args.len() as i32,
-                &mut err,
-            );
 
-            CallError::from_sys(err.error).map(|_| Variant::from_sys(variant))
-        }
+        let api = get_api();
+        let mut err = sys::godot_variant_call_error::default();
+        let mut arg_refs = args.iter().map(Variant::sys).collect::<Vec<_>>();
+        let variant = (api.godot_variant_call)(
+            &mut self.0,
+            &method.0,
+            arg_refs.as_mut_ptr(),
+            args.len() as i32,
+            &mut err,
+        );
+
+        CallError::from_sys(err.error).map(|_| Variant::from_sys(variant))
     }
 
     /// Evaluates a variant operator on `self` and `rhs` and returns the result on success.

gdnative-core/src/core_types/variant_array.rs

@@ -1,8 +1,6 @@
 use std::iter::{Extend, FromIterator};
 use std::marker::PhantomData;
 
-use gdnative_impl_proc_macros::doc_variant_collection_safety;
-
 use crate::private::get_api;
 use crate::sys;
 
@@ -44,12 +42,14 @@ impl<Access: ThreadAccess> VariantArray<Access> {
     /// Sets the value of the element at the given offset.
     #[inline]
     pub fn set<T: OwnedToVariant>(&self, idx: i32, val: T) {
+        self.check_bounds(idx);
         unsafe { (get_api().godot_array_set)(self.sys_mut(), idx, val.owned_to_variant().sys()) }
     }
 
     /// Returns a copy of the element at the given offset.
     #[inline]
     pub fn get(&self, idx: i32) -> Variant {
+        self.check_bounds(idx);
         unsafe { Variant((get_api().godot_array_get)(self.sys(), idx)) }
     }
 
@@ -63,6 +63,7 @@ impl<Access: ThreadAccess> VariantArray<Access> {
     /// `Variant` is reference-counted and thus cheaply cloned. Consider using `get` instead.
     #[inline]
     pub unsafe fn get_ref(&self, idx: i32) -> &Variant {
+        self.check_bounds(idx);
         Variant::cast_ref((get_api().godot_array_operator_index_const)(
             self.sys(),
             idx,
@@ -79,6 +80,7 @@ impl<Access: ThreadAccess> VariantArray<Access> {
     #[inline]
     #[allow(clippy::mut_from_ref)]
     pub unsafe fn get_mut_ref(&self, idx: i32) -> &mut Variant {
+        self.check_bounds(idx);
         Variant::cast_mut_ref((get_api().godot_array_operator_index)(self.sys_mut(), idx))
     }
 
@@ -220,6 +222,15 @@ impl<Access: ThreadAccess> VariantArray<Access> {
         std::mem::forget(self);
         VariantArray::from_sys(sys)
     }
+
+    fn check_bounds(&self, idx: i32) {
+        assert!(
+            idx >= 0 && idx < self.len(),
+            "Index {} out of bounds (len {})",
+            idx,
+            self.len()
+        );
+    }
 }
 
 /// Operations allowed on Dictionaries that can only be referenced to from the current thread.
@@ -335,78 +346,6 @@ impl VariantArray<Shared> {
     pub fn new_shared() -> Self {
         VariantArray::<Unique>::new().into_shared()
     }
-
-    /// Clears the array, resizing to 0.
-    ///
-    #[doc_variant_collection_safety]
-    #[inline]
-    pub unsafe fn clear(&self) {
-        (get_api().godot_array_clear)(self.sys_mut());
-    }
-
-    /// Removes the element at `idx`.
-    ///
-    #[doc_variant_collection_safety]
-    #[inline]
-    pub unsafe fn remove(&self, idx: i32) {
-        (get_api().godot_array_remove)(self.sys_mut(), idx)
-    }
-
-    /// Removed the first occurrence of `val`.
-    ///
-    #[doc_variant_collection_safety]
-    #[inline]
-    pub unsafe fn erase<T: ToVariant>(&self, val: T) {
-        (get_api().godot_array_erase)(self.sys_mut(), val.to_variant().sys())
-    }
-
-    /// Resizes the array, filling with `Nil` if necessary.
-    ///
-    #[doc_variant_collection_safety]
-    #[inline]
-    pub unsafe fn resize(&self, size: i32) {
-        (get_api().godot_array_resize)(self.sys_mut(), size)
-    }
-
-    /// Appends an element at the end of the array.
-    ///
-    #[doc_variant_collection_safety]
-    #[inline]
-    pub unsafe fn push<T: OwnedToVariant>(&self, val: T) {
-        (get_api().godot_array_push_back)(self.sys_mut(), val.owned_to_variant().sys());
-    }
-
-    /// Removes an element at the end of the array.
-    ///
-    #[doc_variant_collection_safety]
-    #[inline]
-    pub unsafe fn pop(&self) -> Variant {
-        Variant((get_api().godot_array_pop_back)(self.sys_mut()))
-    }
-
-    /// Appends an element to the front of the array.
-    ///
-    #[doc_variant_collection_safety]
-    #[inline]
-    pub unsafe fn push_front<T: OwnedToVariant>(&self, val: T) {
-        (get_api().godot_array_push_front)(self.sys_mut(), val.owned_to_variant().sys());
-    }
-
-    /// Removes an element at the front of the array.
-    ///
-    #[doc_variant_collection_safety]
-    #[inline]
-    pub unsafe fn pop_front(&self) -> Variant {
-        Variant((get_api().godot_array_pop_front)(self.sys_mut()))
-    }
-
-    /// Insert a new int at a given position in the array.
-    ///
-    #[doc_variant_collection_safety]
-    #[inline]
-    pub unsafe fn insert<T: OwnedToVariant>(&self, at: i32, val: T) {
-        (get_api().godot_array_insert)(self.sys_mut(), at, val.owned_to_variant().sys())
-    }
 }
 
 /// Operations allowed on Dictionaries that may only be shared on the current thread.
@@ -675,12 +614,24 @@ godot_test!(test_array {
 
 godot_test!(
     test_array_debug {
+        use std::panic::catch_unwind;
+
         let arr = VariantArray::new(); // []
         arr.push(&Variant::from_str("hello world"));
         arr.push(&Variant::from_bool(true));
         arr.push(&Variant::from_i64(42));
 
         assert_eq!(format!("{:?}", arr), "[GodotString(hello world), Bool(True), I64(42)]");
+
+        let set = catch_unwind(|| { arr.set(3, 7i64); });
+        let get = catch_unwind(|| { arr.get(3); });
+        let get_ref = catch_unwind(|| { unsafe { arr.get_ref(3) }; });
+        let get_mut_ref = catch_unwind(|| { unsafe { arr.get_mut_ref(3) }; });
+
+        assert!(set.is_err(), "set() out of bounds causes panic");
+        assert!(get.is_err(), "get() out of bounds causes panic");
+        assert!(get_ref.is_err(), "get_mut() out of bounds causes panic");
+        assert!(get_mut_ref.is_err(), "get_mut_ref() out of bounds causes panic");
     }
 );
 

impl/proc_macros/src/doc.rs

@@ -1,8 +1,12 @@
-use proc_macro2::TokenStream;
-use quote::ToTokens;
 use syn::visit_mut::VisitMut;
-use syn::{Attribute, Item, ItemFn, ItemImpl};
+use syn::{Attribute, ItemFn, ItemImpl};
+
+/*
+Leaving code commented-out, as this might be very useful elsewhere
 
+use proc_macro2::TokenStream;
+use quote::ToTokens;
+use syn::{Item};
 pub fn variant_collection_safety(
     _attr: proc_macro::TokenStream,
     item: proc_macro::TokenStream,
@@ -31,6 +35,7 @@ pub fn variant_collection_safety(
     visit.visit_item_mut(&mut item);
     Ok(item.to_token_stream())
 }
+*/
 
 struct IncludeDocs<'a> {
     docs: &'a [&'a str],

impl/proc_macros/src/lib.rs

@@ -24,12 +24,16 @@ pub fn decl_typed_array_element(input: TokenStream) -> TokenStream {
         .into()
 }
 
+/*
+Leaving code commented-out, as this might be very useful elsewhere
+
 #[proc_macro_attribute]
 pub fn doc_variant_collection_safety(attr: TokenStream, item: TokenStream) -> TokenStream {
     self::doc::variant_collection_safety(attr, item)
         .unwrap_or_else(to_compile_errors)
         .into()
 }
+*/
 
 fn to_compile_errors(error: syn::Error) -> proc_macro2::TokenStream {
     let compile_error = error.to_compile_error();

test/src/test_variant_call_args.rs

@@ -57,34 +57,33 @@ fn test_variant_call_args() -> bool {
 
         let mut base = obj.into_base().into_shared().to_variant();
 
-        assert_eq!(Some(42), base.call("zero", &[]).unwrap().try_to_i64());
+        assert_eq!(Some(42), call_i64(&mut base, "zero", &[]));
 
         assert_eq!(
             Some(126),
-            base.call("one", &[Variant::from_i64(3),])
-                .unwrap()
-                .try_to_i64()
+            call_i64(&mut base, "one", &[Variant::from_i64(3)])
         );
 
         assert_eq!(
             Some(-10),
-            base.call("two", &[Variant::from_i64(-1), Variant::from_i64(32),])
-                .unwrap()
-                .try_to_i64()
+            call_i64(
+                &mut base,
+                "two",
+                &[Variant::from_i64(-1), Variant::from_i64(32)]
+            )
         );
 
         assert_eq!(
             Some(-52),
-            base.call(
+            call_i64(
+                &mut base,
                 "three",
                 &[
                     Variant::from_i64(-2),
                     Variant::from_i64(4),
                     Variant::from_i64(8),
                 ]
             )
-            .unwrap()
-            .try_to_i64()
         );
     })
     .is_ok();
@@ -95,3 +94,9 @@ fn test_variant_call_args() -> bool {
 
     ok
 }
+
+fn call_i64(variant: &mut Variant, method: &str, args: &[Variant]) -> Option<i64> {
+    let result = unsafe { variant.call(method, args) };
+
+    result.unwrap().try_to_i64()
+}