Use CStr in runtime

We have effectively no control over neither class names, selector names, protocol names and so on, and external code _could_ theoretically set these to non-UTF-8 values (although unlikely). So panicking when reading them is not a good idea. Furthermore, using CStr avoids allocating when using the runtime, which is a really nice property. Fixes #564.
madsmtm · Sep 17, 2024 · 58cb3ac · 58cb3ac
1 parent 0cddc19
commit 58cb3ac
Show file tree

Hide file tree

Showing 29 changed files with 989 additions and 1,094 deletions.
diff --git a/crates/objc2/CHANGELOG.md b/crates/objc2/CHANGELOG.md
@@ -123,6 +123,25 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
   order.
 * **BREAKING**: Converted function signatures into using `extern "C-unwind"`
   where applicable. This allows Rust and Objective-C unwinding to interoperate.
+* **BREAKING**: Use `CStr` in methods in the `runtime` module, since it's both
+  more performant, and more correct. Use the new `c"my_str"` syntax to migrate.
+
+  Specifically, this includes:
+  - `ClassBuilder::new`.
+  - `ClassBuilder::root`.
+  - `ClassBuilder::add_ivar`.
+  - `ProtocolBuilder::new`.
+  - `Sel::register`.
+  - `Sel::name`.
+  - `Ivar::name`.
+  - `Ivar::type_encoding`.
+  - `Method::return_type`.
+  - `Method::argument_type`.
+  - `AnyClass::get`.
+  - `AnyClass::name`.
+  - `AnyClass::instance_variable`.
+  - `AnyProtocol::get`.
+  - `AnyProtocol::name`.
 
 
 ## 0.5.2 - 2024-05-21

diff --git a/crates/objc2/examples/class_with_lifetime.rs b/crates/objc2/examples/class_with_lifetime.rs
@@ -37,21 +37,21 @@ impl<'a> MyObject<'a> {
         // interior mutability instead.
         let number = Cell::from_mut(number);
 
-        let ivar = Self::class().instance_variable("number").unwrap();
+        let ivar = Self::class().instance_variable(c"number").unwrap();
         // SAFETY: The ivar is added with the same type below, and the
         // lifetime of the reference is properly bound to the class.
         unsafe { ivar.load_ptr::<Ivar<'_>>(&this.superclass).write(number) };
         this
     }
 
     fn get(&self) -> u8 {
-        let ivar = Self::class().instance_variable("number").unwrap();
+        let ivar = Self::class().instance_variable(c"number").unwrap();
         // SAFETY: The ivar is added with the same type below, and is initialized in `new`
         unsafe { ivar.load::<Ivar<'_>>(&self.superclass).get() }
     }
 
     fn set(&self, number: u8) {
-        let ivar = Self::class().instance_variable("number").unwrap();
+        let ivar = Self::class().instance_variable(c"number").unwrap();
         // SAFETY: The ivar is added with the same type below, and is initialized in `new`
         unsafe { ivar.load::<Ivar<'_>>(&self.superclass).set(number) };
     }
@@ -63,19 +63,19 @@ unsafe impl<'a> ClassType for MyObject<'a> {
     const NAME: &'static str = "MyObject";
 
     fn class() -> &'static AnyClass {
-        // TODO: Use std::lazy::LazyCell
+        // NOTE: Use std::lazy::LazyCell if in MSRV
         static REGISTER_CLASS: Once = Once::new();
 
         REGISTER_CLASS.call_once(|| {
             let superclass = NSObject::class();
-            let mut builder = ClassBuilder::new(Self::NAME, superclass).unwrap();
+            let mut builder = ClassBuilder::new(c"MyObject", superclass).unwrap();
 
-            builder.add_ivar::<Ivar<'_>>("number");
+            builder.add_ivar::<Ivar<'_>>(c"number");
 
             let _cls = builder.register();
         });
 
-        AnyClass::get(Self::NAME).unwrap()
+        AnyClass::get(c"MyObject").unwrap()
     }
 
     fn as_super(&self) -> &Self::Super {

diff --git a/crates/objc2/examples/introspection.rs b/crates/objc2/examples/introspection.rs
@@ -22,14 +22,14 @@ fn main() {
     // Note: You should not rely on the `isa` ivar being available,
     // this is only for demonstration.
     let ivar = cls
-        .instance_variable("isa")
+        .instance_variable(c"isa")
         .expect("no ivar with name 'isa' found on NSObject");
     println!(
-        "Instance variable {} has type encoding {:?}",
+        "Instance variable {:?} has type encoding {:?}",
         ivar.name(),
         ivar.type_encoding()
     );
-    assert!(<*const AnyClass>::ENCODING.equivalent_to_str(ivar.type_encoding()));
+    assert!(<*const AnyClass>::ENCODING.equivalent_to_str(ivar.type_encoding().to_str().unwrap()));
 
     // Inspect a method of the class
     let method = cls.instance_method(sel!(hash)).unwrap();
@@ -39,7 +39,7 @@ fn main() {
     );
     let hash_return = method.return_type();
     println!("-[NSObject hash] return type: {hash_return:?}");
-    assert!(usize::ENCODING.equivalent_to_str(&hash_return));
+    assert!(usize::ENCODING.equivalent_to_str(hash_return.to_str().unwrap()));
 
     // Create an instance
     let obj = NSObject::new();

diff --git a/crates/objc2/src/__macro_helpers/common_selectors.rs b/crates/objc2/src/__macro_helpers/common_selectors.rs
@@ -76,6 +76,7 @@ fn cxx_destruct_sel() -> Sel {
 
 #[cfg(test)]
 mod tests {
+    use alloc::ffi::CString;
     use core::sync::atomic::{AtomicBool, Ordering};
 
     use crate::rc::Retained;
@@ -90,7 +91,8 @@ mod tests {
     fn test_destruct_dynamic() {
         static HAS_RUN: AtomicBool = AtomicBool::new(false);
 
-        let mut builder = ClassBuilder::new("TestCxxDestruct", NSObject::class()).unwrap();
+        let name = CString::new("TestCxxDestruct").unwrap();
+        let mut builder = ClassBuilder::new(&name, NSObject::class()).unwrap();
 
         unsafe extern "C" fn destruct(_: *mut NSObject, _: Sel) {
             HAS_RUN.store(true, Ordering::Relaxed);

diff --git a/crates/objc2/src/__macro_helpers/declare_class.rs b/crates/objc2/src/__macro_helpers/declare_class.rs
@@ -1,3 +1,4 @@
+use alloc::ffi::CString;
 #[cfg(debug_assertions)]
 use alloc::vec::Vec;
 use core::marker::PhantomData;
@@ -139,9 +140,16 @@ pub struct ClassBuilderHelper<T: ?Sized> {
     p: PhantomData<T>,
 }
 
+// Outlined for code size
 #[track_caller]
-fn failed_declaring_class(name: &str) -> ! {
-    panic!("could not create new class {name}. Perhaps a class with that name already exists?")
+fn create_builder(name: &str, superclass: &AnyClass) -> ClassBuilder {
+    let c_name = CString::new(name).expect("class name must be UTF-8");
+    match ClassBuilder::new(&c_name, superclass) {
+        Some(builder) => builder,
+        None => panic!(
+            "could not create new class {name}. Perhaps a class with that name already exists?"
+        ),
+    }
 }
 
 impl<T: DeclaredClass> ClassBuilderHelper<T> {
@@ -152,10 +160,7 @@ impl<T: DeclaredClass> ClassBuilderHelper<T> {
     where
         T::Super: ClassType,
     {
-        let mut builder = match ClassBuilder::new(T::NAME, <T::Super as ClassType>::class()) {
-            Some(builder) => builder,
-            None => failed_declaring_class(T::NAME),
-        };
+        let mut builder = create_builder(T::NAME, <T::Super as ClassType>::class());
 
         setup_dealloc::<T>(&mut builder);
 

diff --git a/crates/objc2/src/__macro_helpers/declared_ivars.rs b/crates/objc2/src/__macro_helpers/declared_ivars.rs
@@ -46,7 +46,9 @@
 //! [unsound-read-padding]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=ea068e8d9e55801aa9520ea914eb2822
 
 use alloc::borrow::Cow;
+use alloc::ffi::CString;
 use alloc::format;
+use core::ffi::CStr;
 use core::mem;
 use core::ptr::{self, NonNull};
 
@@ -239,17 +241,27 @@ where
 pub(crate) fn register_with_ivars<T: DeclaredClass>(
     mut builder: ClassBuilder,
 ) -> (&'static AnyClass, isize, isize) {
-    let (ivar_name, drop_flag_name): (Cow<'static, str>, Cow<'static, str>) = {
+    let (ivar_name, drop_flag_name): (Cow<'static, CStr>, Cow<'static, CStr>) = {
         if cfg!(feature = "gnustep-1-7") {
             // GNUStep does not support a subclass having an ivar with the
             // same name as a superclass, so let's use the class name as the
             // ivar name to ensure uniqueness.
             (
-                format!("{}_ivars", T::NAME).into(),
-                format!("{}_drop_flag", T::NAME).into(),
+                CString::new(format!("{}_ivars", T::NAME)).unwrap().into(),
+                CString::new(format!("{}_drop_flag", T::NAME))
+                    .unwrap()
+                    .into(),
             )
         } else {
-            ("ivars".into(), "drop_flag".into())
+            // SAFETY: The byte slices are NUL-terminated, and do not contain
+            // interior NUL bytes.
+            // TODO: Use `c"my_str"` syntax once in MSRV
+            unsafe {
+                (
+                    CStr::from_bytes_with_nul_unchecked(b"ivars\0").into(),
+                    CStr::from_bytes_with_nul_unchecked(b"drop_flag\0").into(),
+                )
+            }
         }
     };
 
@@ -670,7 +682,8 @@ mod tests {
         } else {
             "ivars"
         };
-        assert!(IvarZst::class().instance_variable(ivar_name).is_none());
+        let ivar_name = CString::new(ivar_name).unwrap();
+        assert!(IvarZst::class().instance_variable(&ivar_name).is_none());
 
         let obj = unsafe { init(IvarZst::alloc()) };
         #[cfg(feature = "std")]
@@ -706,8 +719,9 @@ mod tests {
         } else {
             "ivars"
         };
+        let ivar_name = CString::new(ivar_name).unwrap();
         let ivar = HasIvarWithHighAlignment::class()
-            .instance_variable(ivar_name)
+            .instance_variable(&ivar_name)
             .unwrap();
         assert_eq!(ivar.offset(), 16);
     }

diff --git a/crates/objc2/src/exception.rs b/crates/objc2/src/exception.rs
@@ -21,6 +21,7 @@
 
 #[cfg(feature = "exception")]
 use core::ffi::c_void;
+use core::ffi::CStr;
 use core::fmt;
 #[cfg(feature = "exception")]
 use core::mem;
@@ -82,7 +83,8 @@ impl Exception {
             let obj: *const Exception = self;
             let obj = unsafe { obj.cast::<NSObject>().as_ref().unwrap() };
             // Get class dynamically instead of with `class!` macro
-            Some(obj.isKindOfClass(AnyClass::get("NSException")?))
+            let name = CStr::from_bytes_with_nul(b"NSException\0").unwrap();
+            Some(obj.isKindOfClass(AnyClass::get(name)?))
         } else {
             Some(false)
         }

diff --git a/crates/objc2/src/rc/id.rs b/crates/objc2/src/rc/id.rs
@@ -602,7 +602,7 @@ impl<T: Message> Retained<T> {
     /// use objc2::rc::Retained;
     /// use objc2::runtime::{AnyClass, AnyObject, Sel};
     ///
-    /// let mut builder = ClassBuilder::new("ExampleObject", class!(NSObject)).unwrap();
+    /// let mut builder = ClassBuilder::new(c"ExampleObject", class!(NSObject)).unwrap();
     ///
     /// extern "C-unwind" fn get(cls: &AnyClass, _cmd: Sel) -> *mut AnyObject {
     ///     let obj: Retained<AnyObject> = unsafe { msg_send_id![cls, new] };