Exploit fact that FFI array-get returns null pointer and no longer prints an error on out-of-bounds

Author: Bromeon

godot-core/src/builtin/array.rs

@@ -78,11 +78,17 @@ impl<T: VariantMetadata> Array<T> {
     }
 
     /// Returns the number of elements in the array. Equivalent of `size()` in Godot.
+    ///
+    /// Retrieving the size incurs an FFI call. If you know the size hasn't changed, you may consider storing
+    /// it in a variable. For loops, prefer iterators.
     pub fn len(&self) -> usize {
         to_usize(self.as_inner().size())
     }
 
     /// Returns `true` if the array is empty.
+    ///
+    /// Checking for emptiness incurs an FFI call. If you know the size hasn't changed, you may consider storing
+    /// it in a variable. For loops, prefer iterators.
     pub fn is_empty(&self) -> bool {
         self.as_inner().is_empty()
     }
@@ -150,10 +156,24 @@ impl<T: VariantMetadata> Array<T> {
     ///
     /// If `index` is out of bounds.
     fn ptr(&self, index: usize) -> *const Variant {
-        self.check_bounds(index);
+        let ptr = self.ptr_or_null(index);
+        assert!(
+            !ptr.is_null(),
+            "Array index {index} out of bounds (len {len})",
+            len = self.len(),
+        );
+        ptr
+    }
+
+    /// Returns a pointer to the element at the given index, or null if out of bounds.
+    fn ptr_or_null(&self, index: usize) -> *const Variant {
+        // SAFETY: array_operator_index_const returns null for invalid indexes.
+        let variant_ptr = unsafe {
+            let index = to_i64(index);
+            interface_fn!(array_operator_index_const)(self.sys(), index)
+        };
 
-        // SAFETY: We just checked that the index is not out of bounds.
-        unsafe { self.ptr_unchecked(index) }
+        Variant::ptr_from_sys(variant_ptr)
     }
 
     /// Returns a mutable pointer to the element at the given index.
@@ -162,29 +182,23 @@ impl<T: VariantMetadata> Array<T> {
     ///
     /// If `index` is out of bounds.
     fn ptr_mut(&self, index: usize) -> *mut Variant {
-        self.check_bounds(index);
-
-        // SAFETY: We just checked that the index is not out of bounds.
-        unsafe { self.ptr_mut_unchecked(index) }
+        let ptr = self.ptr_mut_or_null(index);
+        assert!(
+            !ptr.is_null(),
+            "Array index {index} out of bounds (len {len})",
+            len = self.len(),
+        );
+        ptr
     }
 
-    /// Returns a pointer to the element at the given index.
-    ///
-    /// # Safety
-    ///
-    /// Calling this with an out-of-bounds index is undefined behavior.
-    unsafe fn ptr_unchecked(&self, index: usize) -> *const Variant {
-        let variant_ptr = interface_fn!(array_operator_index_const)(self.sys(), to_i64(index));
-        Variant::ptr_from_sys(variant_ptr)
-    }
+    /// Returns a pointer to the element at the given index, or null if out of bounds.
+    fn ptr_mut_or_null(&self, index: usize) -> *mut Variant {
+        // SAFETY: array_operator_index returns null for invalid indexes.
+        let variant_ptr = unsafe {
+            let index = to_i64(index);
+            interface_fn!(array_operator_index)(self.sys(), index)
+        };
 
-    /// Returns a mutable pointer to the element at the given index.
-    ///
-    /// # Safety
-    ///
-    /// Calling this with an out-of-bounds index is undefined behavior.
-    unsafe fn ptr_mut_unchecked(&self, index: usize) -> *mut Variant {
-        let variant_ptr = interface_fn!(array_operator_index)(self.sys(), to_i64(index));
         Variant::ptr_from_sys_mut(variant_ptr)
     }
 
@@ -365,6 +379,7 @@ impl<T: VariantMetadata + FromVariant> Array<T> {
     ///
     /// If `index` is out of bounds.
     pub fn get(&self, index: usize) -> T {
+        // Panics on out-of-bounds
         let ptr = self.ptr(index);
 
         // SAFETY: `ptr()` just verified that the index is not out of bounds.
@@ -702,9 +717,11 @@ impl<T: VariantMetadata + ToVariant> From<&[T]> for Array<T> {
             return array;
         }
         array.resize(len);
-        let ptr = array.ptr_mut(0);
+
+        let ptr = array.ptr_mut_or_null(0);
         for (i, element) in slice.iter().enumerate() {
             // SAFETY: The array contains exactly `len` elements, stored contiguously in memory.
+            // Also, the pointer is non-null, as we checked for emptiness above.
             unsafe {
                 *ptr.offset(to_isize(i)) = element.to_variant();
             }
@@ -767,10 +784,11 @@ impl<'a, T: VariantMetadata + FromVariant> Iterator for Iter<'a, T> {
         if self.next_idx < self.array.len() {
             let idx = self.next_idx;
             self.next_idx += 1;
-            // Using `ptr_unchecked` rather than going through `get()` so we can avoid a second
-            // bounds check.
-            // SAFETY: We just checked that the index is not out of bounds.
-            let variant = unsafe { &*self.array.ptr_unchecked(idx) };
+
+            let element_ptr = self.array.ptr_or_null(idx);
+
+            // SAFETY: We just checked that the index is not out of bounds, so the pointer won't be null.
+            let variant = unsafe { &*element_ptr };
             let element = T::from_variant(variant);
             Some(element)
         } else {

godot-core/src/builtin/dictionary.rs

@@ -192,6 +192,8 @@ impl Dictionary {
     /// _Godot equivalent: `dict[key] = value`_
     pub fn set<K: ToVariant, V: ToVariant>(&mut self, key: K, value: V) {
         let key = key.to_variant();
+
+        // SAFETY: always returns a valid pointer to a value in the dictionary; either pre-existing or newly inserted.
         unsafe {
             *self.get_ptr_mut(key) = value.to_variant();
         }
@@ -226,12 +228,16 @@ impl Dictionary {
 
     /// Get the pointer corresponding to the given key in the dictionary.
     ///
-    /// If there exists no value at the given key, a `NIL` variant will be created.
+    /// If there exists no value at the given key, a `NIL` variant will be inserted for that key.
     fn get_ptr_mut<K: ToVariant>(&mut self, key: K) -> *mut Variant {
         let key = key.to_variant();
+
+        // SAFETY: accessing an unknown key _mutably_ creates that entry in the dictionary, with value `NIL`.
         let ptr = unsafe {
             interface_fn!(dictionary_operator_index)(self.sys_mut(), key.var_sys_const())
         };
+
+        // Never a null pointer, since entry either existed already or was inserted above.
         Variant::ptr_from_sys_mut(ptr)
     }
 }

godot-core/src/builtin/variant/mod.rs

@@ -200,16 +200,17 @@ impl Variant {
         sys::to_const_ptr(self.var_sys())
     }
 
-    pub(crate) fn ptr_from_sys(variant_ptr: sys::GDExtensionVariantPtr) -> *const Variant {
-        assert!(!variant_ptr.is_null(), "ptr_from_sys: null variant pointer");
+    /// Converts to variant pointer; can be a null pointer.
+    pub(crate) fn ptr_from_sys(
+        variant_ptr: sys::GDExtensionVariantPtr,
+    ) -> *const Variant {
         variant_ptr as *const Variant
     }
 
-    pub(crate) fn ptr_from_sys_mut(variant_ptr: sys::GDExtensionVariantPtr) -> *mut Variant {
-        assert!(
-            !variant_ptr.is_null(),
-            "ptr_from_sys_mut: null variant pointer"
-        );
+    /// Converts to variant mut pointer; can be a null pointer.
+    pub(crate) fn ptr_from_sys_mut(
+        variant_ptr: sys::GDExtensionVariantPtr,
+    ) -> *mut Variant {
         variant_ptr as *mut Variant
     }
 }