Merge #844

844: `Rid` and APIs accepting it are now unsafe r=Bromeon a=Bromeon

Fixes #836.

Makes all GDNative API methods accepting at least one `Rid` parameter `unsafe`.

Overall changes in `Rid`:
* Remove `operator_less()`
* Rename `is_valid()` -> `is_occupied()`
* `get_id()` is now `unsafe` + null-checked (if the RID is dangling, Godot dereferences an invalid pointer)
* Fix `PartialOrd` impl
* Docs

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

Author: bors[bot]

bindings_generator/src/api.rs

@@ -324,7 +324,7 @@ impl GodotArgument {
     }
 }
 
-#[derive(Clone)]
+#[derive(Clone, Eq, PartialEq)]
 pub enum Ty {
     Void,
     String,

bindings_generator/src/methods.rs

@@ -344,20 +344,19 @@ pub(crate) fn generate_methods(
         let icall_name = method_sig.function_name();
         let icall = format_ident!("{}", icall_name);
 
-        icalls.insert(icall_name.clone(), method_sig);
-
-        let rusty_name = rust_safe_name(rusty_method_name);
-
         let maybe_unsafe: TokenStream;
         let maybe_unsafe_reason: &str;
-        if UNSAFE_OBJECT_METHODS.contains(&(&class.name, method_name)) {
+        if let Some(unsafe_reason) = unsafe_reason(class, method_name, &method_sig) {
             maybe_unsafe = quote! { unsafe };
-            maybe_unsafe_reason = "\n# Safety\nThis function bypasses Rust's static type checks \
-                (aliasing, thread boundaries, calls to free(), ...).";
+            maybe_unsafe_reason = unsafe_reason;
         } else {
             maybe_unsafe = TokenStream::default();
             maybe_unsafe_reason = "";
-        };
+        }
+
+        icalls.insert(icall_name.clone(), method_sig);
+
+        let rusty_name = rust_safe_name(rusty_method_name);
 
         let method_bind_fetch = {
             let method_table = format_ident!("{}MethodTable", class.name);
@@ -394,6 +393,27 @@ pub(crate) fn generate_methods(
     result
 }
 
+/// Returns a message as to why this method would be unsafe; or None if the method is safe
+fn unsafe_reason(
+    class: &GodotClass,
+    method_name: &str,
+    method_sig: &MethodSig,
+) -> Option<&'static str> {
+    if UNSAFE_OBJECT_METHODS.contains(&(&class.name, method_name)) {
+        Some(
+            "\n# Safety\
+             \nThis function bypasses Rust's static type checks (aliasing, thread boundaries, calls to free(), ...).",
+        )
+    } else if method_sig.arguments.contains(&Ty::Rid) {
+        Some(
+            "\n# Safety\
+             \nThis function has parameters of type `Rid` (resource ID). \
+             RIDs are untyped and interpreted as raw pointers by the engine, so passing an incorrect RID can cause UB.")
+    } else {
+        None
+    }
+}
+
 fn ret_recover(ty: &Ty, icall_ty: IcallType) -> TokenStream {
     match icall_ty {
         IcallType::Ptr => ty.to_return_post(),

gdnative-core/src/core_types/rid.rs

@@ -1,32 +1,57 @@
 use crate::private::get_api;
 use crate::sys;
 use std::cmp::Ordering;
-use std::cmp::{Eq, PartialEq};
 use std::mem::transmute;
 
-/// The RID type is used to access the unique integer ID of a resource.
-/// They are opaque, so they do not grant access to the associated resource by themselves.
+// Note: for safety design, consult:
+// * https://github.com/godotengine/godot/blob/3.x/core/rid.h
+// * https://github.com/godotengine/godot/blob/3.x/modules/gdnative/include/gdnative/rid.h
+
+/// A RID ("resource ID") is an opaque handle that refers to a Godot `Resource`.
+///
+/// RIDs do not grant access to the resource itself. Instead, they can be used in lower-level resource APIs
+/// such as the [servers]. See also [Godot API docs for `RID`][docs].
+///
+/// Note that RIDs are highly unsafe to work with (especially with a Release build of Godot):
+/// * They are untyped, i.e. Godot does not recognize if they represent the correct resource type.
+/// * The internal handle is interpreted as a raw pointer by Godot, meaning that passing an invalid or wrongly
+///   typed RID is instant undefined behavior.
+///
+/// For this reason, GDNative methods accepting `Rid` parameters are marked `unsafe`.
+///
+/// [servers]: https://docs.godotengine.org/en/stable/tutorials/optimization/using_servers.html
+/// [docs]: https://docs.godotengine.org/en/stable/classes/class_rid.html
 #[derive(Copy, Clone, Debug)]
 pub struct Rid(pub(crate) sys::godot_rid);
 
 impl Rid {
+    /// Creates an empty, invalid RID.
     #[inline]
     pub fn new() -> Self {
         Rid::default()
     }
 
+    /// Returns the ID of the referenced resource.
+    ///
+    /// # Panics
+    /// When this instance is empty, i.e. `self.is_occupied()` is false.
+    ///
+    /// # Safety
+    /// RIDs are untyped and interpreted as raw pointers by the engine.
+    /// If this method is called on an invalid resource ID, the behavior is undefined.
+    /// This can happen when the resource behind the RID is no longer alive.
     #[inline]
-    pub fn get_id(self) -> i32 {
-        unsafe { (get_api().godot_rid_get_id)(&self.0) }
-    }
-
-    #[inline]
-    pub fn operator_less(self, b: Rid) -> bool {
-        unsafe { (get_api().godot_rid_operator_less)(&self.0, &b.0) }
+    pub unsafe fn get_id(self) -> i32 {
+        assert!(self.is_occupied());
+        (get_api().godot_rid_get_id)(&self.0)
     }
 
+    /// Check if this RID is non-empty. This does **not** mean it's valid or safe to use!
+    ///
+    /// This simply checks if the handle has not been initialized with the empty default.
+    /// It does not give any indication about whether it points to a valid resource.
     #[inline]
-    pub fn is_valid(self) -> bool {
+    pub fn is_occupied(self) -> bool {
         self.to_u64() != 0
     }
 
@@ -73,14 +98,12 @@ impl_basic_traits_as_sys! {
 impl PartialOrd for Rid {
     #[inline]
     fn partial_cmp(&self, other: &Rid) -> Option<Ordering> {
-        unsafe {
-            let native = (get_api().godot_rid_operator_less)(&self.0, &other.0);
-
-            if native {
-                Some(Ordering::Less)
-            } else {
-                Some(Ordering::Greater)
-            }
+        if PartialEq::eq(self, other) {
+            Some(Ordering::Equal)
+        } else if unsafe { (get_api().godot_rid_operator_less)(self.sys(), other.sys()) } {
+            Some(Ordering::Less)
+        } else {
+            Some(Ordering::Greater)
         }
     }
 }

gdnative-core/src/export/mod.rs

@@ -1,4 +1,4 @@
-//! Functionality for user-defined types exported to the engine (native scripts)
+//! Functionality for user-defined types exported to the engine (native scripts).
 //!
 //! NativeScript allows users to have their own scripts in a native language (in this case Rust).
 //! It is _not_ the same as GDNative, the native interface to call into Godot.
@@ -8,32 +8,7 @@
 //! If you are looking for how to manage Godot core types or classes (objects), check
 //! out the [`core_types`][crate::core_types] and [`object`][crate::object] modules, respectively.
 //!
-//! ## Init and exit hooks
-//!
-//! Three endpoints are automatically invoked by the engine during startup and shutdown:
-//!
-//! * [`godot_gdnative_init`],
-//! * [`godot_nativescript_init`],
-//! * [`godot_gdnative_terminate`],
-//!
-//! All three must be present. To quickly define all three endpoints using the default names,
-//! use [`godot_init`].
-//!
-//! ## Registering script classes
-//!
-//! [`InitHandle`] is the registry of all your exported symbols.
-//! To register script classes, call [`InitHandle::add_class()`] or [`InitHandle::add_tool_class()`]
-//! in your [`godot_nativescript_init`] or [`godot_init`] callback:
-//!
-//! ```ignore
-//! use gdnative::prelude::*;
-//!
-//! fn init(handle: InitHandle) {
-//!     handle.add_class::<HelloWorld>();
-//! }
-//!
-//! godot_init!(init);
-//! ```
+//! To handle initialization and shutdown of godot-rust, take a look at the [`init`][crate::init] module.
 //!
 //! For full examples, see [`examples`](https://github.com/godot-rust/godot-rust/tree/master/examples)
 //! in the godot-rust repository.

gdnative-core/src/init/mod.rs

@@ -1,4 +1,40 @@
 //! Global initialization and termination of the library.
+//!
+//! This module provides all the plumbing required for global initialization and shutdown of godot-rust.
+//!
+//! ## Init and exit hooks
+//!
+//! Three endpoints are automatically invoked by the engine during startup and shutdown:
+//!
+//! * [`godot_gdnative_init`],
+//! * [`godot_nativescript_init`],
+//! * [`godot_gdnative_terminate`],
+//!
+//! All three must be present. To quickly define all three endpoints using the default names,
+//! use [`godot_init`].
+//!
+//! ## Registering script classes
+//!
+//! [`InitHandle`] is the registry of all your exported symbols.
+//! To register script classes, call [`InitHandle::add_class()`] or [`InitHandle::add_tool_class()`]
+//! in your `godot_nativescript_init` or `godot_init` callback:
+//!
+//! ```no_run
+//! use gdnative::prelude::*;
+//!
+//! #[derive(NativeClass)]
+//! # #[no_constructor]
+//! struct HelloWorld { /* ... */ }
+//!
+//! #[methods]
+//! impl HelloWorld { /* ... */ }
+//!
+//! fn init(handle: InitHandle) {
+//!     handle.add_class::<HelloWorld>();
+//! }
+//!
+//! godot_init!(init);
+//! ```
 
 mod info;
 mod init_handle;