Make getting references bound by the autorelease pool unsafe

The current design of `autoreleasepool` does not prevent the user from
"smuggling" the lifetime from an outer pool and using that inside an
inner pool, even with the unstable auto traits guard.

So let's make using the pool to construct a reference `unsafe` (making
the invocation of `autoreleasepool` itself `unsafe` would be cumbersome
for a lot of applications, especially so because of the extra nesting).

Fixes #540, see that for details
on why this is unsound.

We _might_ be able to make this pattern safe again with something like
contexts and capabilities, but that's future work.

Author: madsmtm

crates/objc2/CHANGELOG.md

@@ -114,6 +114,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
   Classes now always use interior mutability.
 * **BREAKING**: Removed `DerefMut` implementation for `Retained<T>` when the
   `Retained` was mutable.
+* **BREAKING**: Mark `Retained::autorelease` as `unsafe`, since we cannot
+  ensure that the given pool is actually the innermost pool.
 
 ### Fixed
 * Remove an incorrect assertion when adding protocols to classes in an unexpected

crates/objc2/src/macros/mod.rs

@@ -1153,16 +1153,16 @@ macro_rules! msg_send_bool {
 /// return a `Result<Retained<T>, Retained<E>>`, see below.
 ///
 /// The `retain`, `release` and `autorelease` selectors are not supported, use
-/// [`Retained::retain`], [`Retained::drop`] and [`Retained::autorelease`] for
-/// that.
+/// [`Retained::retain`], [`Retained::drop`] and [`Retained::autorelease_ptr`]
+/// for that.
 ///
 /// [sel-families]: https://clang.llvm.org/docs/AutomaticReferenceCounting.html#arc-method-families
 /// [`MessageReceiver`]: crate::runtime::MessageReceiver
 /// [`Retained::retain_autoreleased`]: crate::rc::Retained::retain_autoreleased
 /// [arc-retainable]: https://clang.llvm.org/docs/AutomaticReferenceCounting.html#retainable-object-pointers-as-operands-and-arguments
 /// [`Retained::retain`]: crate::rc::Retained::retain
 /// [`Retained::drop`]: crate::rc::Retained::drop
-/// [`Retained::autorelease`]: crate::rc::Retained::autorelease
+/// [`Retained::autorelease_ptr`]: crate::rc::Retained::autorelease_ptr
 ///
 ///
 /// # Errors

crates/objc2/src/rc/autorelease.rs

@@ -108,16 +108,21 @@ impl Drop for Pool {
 /// use objc2::runtime::NSObject;
 /// use objc2::msg_send;
 ///
-/// fn needs_lifetime_from_pool<'p>(pool: AutoreleasePool<'p>) -> &'p NSObject {
+/// unsafe fn needs_lifetime_from_pool<'p>(pool: AutoreleasePool<'p>) -> &'p NSObject {
 ///     let obj = NSObject::new();
 ///     // Do action that returns an autoreleased object
 ///     let description: *mut NSObject = unsafe { msg_send![&obj, description] };
-///     // Bound the lifetime of the reference to the pool
+///     // Bound the lifetime of the reference to that of the pool.
+///     //
+///     // Note that this only helps ensuring soundness, our function is still
+///     // unsafe because the pool cannot be guaranteed to be the innermost
+///     // pool.
 ///     unsafe { pool.ptr_as_ref(description) }
 /// }
 ///
 /// autoreleasepool(|pool| {
-///     let obj = needs_lifetime_from_pool(pool);
+///     // SAFETY: The given pool is the innermost pool.
+///     let obj = unsafe { needs_lifetime_from_pool(pool) };
 ///     println!("{obj:?}");
 /// });
 /// ```
@@ -178,10 +183,8 @@ impl<'pool> AutoreleasePool<'pool> {
         }
     }
 
-    /// This will be removed in a future version.
     #[inline]
-    #[doc(hidden)]
-    pub fn __verify_is_inner(self) {
+    pub(crate) fn __verify_is_inner(self) {
         #[cfg(all(debug_assertions, not(feature = "unstable-autoreleasesafe")))]
         if let Some(pool) = &self.inner {
             POOLS.with(|c| {
@@ -200,11 +203,24 @@ impl<'pool> AutoreleasePool<'pool> {
     /// objects, since it binds the lifetime of the reference to the pool, and
     /// does some extra checks when debug assertions are enabled.
     ///
+    /// Note that this is helpful, but not sufficient, for ensuring that the
+    /// lifetime of the reference does not exceed the lifetime of the
+    /// autorelease pool. When calling this, you must also ensure that the
+    /// pool is actually the current innermost pool.
+    ///
+    ///
+    /// # Panics
+    ///
+    /// If the pool is not the innermost pool, this function may panic when
+    /// the `"std"` Cargo feature and debug assertions are enabled.
+    ///
     ///
     /// # Safety
     ///
     /// This is equivalent to `&*ptr`, and shares the unsafety of that, except
     /// the lifetime is bound to the pool instead of being unbounded.
+    ///
+    /// The pool must be the innermost pool for the lifetime to be correct.
     #[inline]
     pub unsafe fn ptr_as_ref<T: ?Sized>(self, ptr: *const T) -> &'pool T {
         self.__verify_is_inner();
@@ -304,51 +320,66 @@ impl !AutoreleaseSafe for AutoreleasePool<'_> {}
 /// see [Apple's documentation][apple-autorelease] for general information on
 /// when to use those.
 ///
-/// The pool is passed as a parameter to the closure to give you a lifetime
+/// [The pool] is passed as a parameter to the closure to give you a lifetime
 /// parameter that autoreleased objects can refer to.
 ///
 /// Note that this is mostly useful for preventing leaks (as any Objective-C
 /// method may autorelease internally - see also [`autoreleasepool_leaking`]).
 /// If implementing an interface to an object, you should try to return
 /// retained pointers with [`msg_send_id!`] wherever you can instead, since
-/// it is usually more efficient, and having to use this function can be quite
-/// cumbersome for users.
+/// it is usually more efficient, safer, and having to use this function can
+/// be quite cumbersome for users.
 ///
 /// [apple-autorelease]: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmAutoreleasePools.html
+/// [The pool]: AutoreleasePool
 /// [`msg_send_id!`]: crate::msg_send_id
 ///
 ///
 /// # Restrictions
 ///
-/// The given parameter must not be used in an inner `autoreleasepool` - doing
-/// so will panic with debug assertions enabled, and be a compile error in a
-/// future release.
+/// The given parameter must not be used inside a nested `autoreleasepool`,
+/// since doing so will give the objects that it is used with an incorrect
+/// lifetime bound.
 ///
-/// Note that this means that **this function is currently unsound**, since it
-/// doesn't disallow wrong usage in all cases. Enabling the assertions in
-/// release mode would be prohibitively expensive though, so this is the
-/// least-bad solution.
+/// You can try to enable the `"unstable-autoreleasesafe"` Cargo feature using
+/// nightly Rust - if your use of this function compiles with that, it is more
+/// likely to be correct, though note that Rust does not have a way to express
+/// the lifetimes involved, so we cannot detect all invalid usage. See issue
+/// [#540] for details.
 ///
-/// You can try to compile your crate with the `"unstable-autoreleasesafe"`
-/// crate feature enabled on nightly Rust - if your crate compiles with that,
-/// its autoreleasepool usage is guaranteed to be correct.
+/// [#540]: https://github.com/madsmtm/objc2/issues/540
 ///
 ///
 /// # Examples
 ///
-/// Basic usage:
+/// Use an external API, and ensure that the memory that it used is cleaned
+/// up afterwards.
+///
+/// ```
+/// use objc2::rc::autoreleasepool;
+/// # fn example_function() {}
+/// # #[cfg(for_illustrative_purposes)]
+/// use example_crate::example_function;
+///
+/// autoreleasepool(|_| {
+///     // Call `example_function` in the context of a pool
+///     example_function();
+/// }); // Memory released into the pool is cleaned up when the scope ends
+/// ```
+///
+/// Autorelease an object into an autorelease pool:
 ///
 /// ```
 /// use objc2::rc::{autoreleasepool, Retained};
 /// use objc2::runtime::NSObject;
 ///
 /// autoreleasepool(|pool| {
 ///     // Create `obj` and autorelease it to the pool
-///     let obj = Retained::autorelease(NSObject::new(), pool);
+///     // SAFETY: The pool is the current innermost pool
+///     let obj = unsafe { Retained::autorelease(NSObject::new(), pool) };
 ///     // We now have a reference that we can freely use
 ///     println!("{obj:?}");
-///     // `obj` is deallocated when the pool ends
-/// });
+/// }); // `obj` is deallocated when the pool ends
 /// // And is no longer usable outside the closure
 /// ```
 ///
@@ -359,7 +390,7 @@ impl !AutoreleaseSafe for AutoreleasePool<'_> {}
 /// use objc2::rc::{autoreleasepool, Retained};
 /// use objc2::runtime::NSObject;
 ///
-/// let obj = autoreleasepool(|pool| {
+/// let obj = autoreleasepool(|pool| unsafe {
 ///     Retained::autorelease(NSObject::new(), pool)
 /// });
 /// ```
@@ -375,7 +406,8 @@ impl !AutoreleaseSafe for AutoreleasePool<'_> {}
 ///
 /// autoreleasepool(|outer_pool| {
 ///     let obj = autoreleasepool(|inner_pool| {
-///         Retained::autorelease(NSObject::new(), outer_pool)
+///         // SAFETY: NOT safe, the pool is _not_ the innermost pool!
+///         unsafe { Retained::autorelease(NSObject::new(), outer_pool) }
 ///     });
 ///     // `obj` could wrongly be used here because its lifetime was
 ///     // assigned to the outer pool, even though it was released by the
@@ -444,7 +476,8 @@ where
 ///
 /// autoreleasepool(|outer_pool| {
 ///     let obj = autoreleasepool_leaking(|inner_pool| {
-///         Retained::autorelease(NSObject::new(), outer_pool)
+///         // SAFETY: The given `outer_pool` is the actual innermost pool.
+///         unsafe { Retained::autorelease(NSObject::new(), outer_pool) }
 ///     });
 ///     // `obj` is still usable here, since the leaking pool doesn't actually
 ///     // do anything.
@@ -461,8 +494,8 @@ where
 /// use objc2::rc::{autoreleasepool_leaking, Retained};
 /// use objc2::runtime::NSObject;
 ///
-/// let obj = autoreleasepool_leaking(|pool| {
-///     Retained::autorelease(NSObject::new(), pool)
+/// let obj = autoreleasepool_leaking(|pool| unsafe {
+///     unsafe { Retained::autorelease(NSObject::new(), pool) }
 /// });
 /// ```
 ///
@@ -476,7 +509,8 @@ where
 ///
 /// autoreleasepool_leaking(|outer_pool| {
 ///     let obj = autoreleasepool(|inner_pool| {
-///         Retained::autorelease(NSObject::new(), outer_pool)
+///         // SAFETY: NOT safe, the pool is _not_ the innermost pool!
+///         unsafe { Retained::autorelease(NSObject::new(), outer_pool) }
 ///     });
 /// });
 /// #
@@ -564,7 +598,7 @@ mod tests {
                 let mut expected = expected;
 
                 autoreleasepool(|pool| {
-                    let _autoreleased = Retained::autorelease(obj.0, pool);
+                    let _autoreleased = unsafe { Retained::autorelease(obj.0, pool) };
                     expected.autorelease += 1;
                     expected.assert_current();
                     panic!("unwind");

crates/objc2/src/rc/id.rs

@@ -502,17 +502,20 @@ impl<T: Message> Retained<T> {
     /// Autoreleases the [`Retained`], returning a pointer.
     ///
     /// The object is not immediately released, but will be when the innermost
-    /// / current autorelease pool (given as a parameter) is drained.
+    /// / current autorelease pool is drained.
     ///
     /// This is useful when defining your own classes and you have some error
     /// parameter passed as `Option<&mut *mut NSError>`, and you want to
     /// create and autorelease an error before returning.
     ///
-    /// See [`Retained::autorelease`] for an alternative that yields safe
-    /// references.
-    ///
     /// This is an associated method, and must be called as
     /// `Retained::autorelease_ptr(obj)`.
+    ///
+    /// # Safety
+    ///
+    /// This method is safe to call, but the returned pointer is only
+    /// guaranteed to be valid until the innermost autorelease pool is
+    /// drained.
     #[doc(alias = "objc_autorelease")]
     #[must_use = "if you don't intend to use the object any more, drop it as usual"]
     #[inline]
@@ -535,11 +538,17 @@ impl<T: Message> Retained<T> {
     ///
     /// This is an associated method, and must be called as
     /// `Retained::autorelease(obj, pool)`.
+    ///
+    /// # Safety
+    ///
+    /// The given pool must represent the innermost pool, to ensure that the
+    /// reference is not moved outside the autorelease pool into which it has
+    /// been put in.
     #[doc(alias = "objc_autorelease")]
     #[must_use = "if you don't intend to use the object any more, drop it as usual"]
     #[inline]
     #[allow(clippy::needless_lifetimes)]
-    pub fn autorelease<'p>(this: Self, pool: AutoreleasePool<'p>) -> &'p T {
+    pub unsafe fn autorelease<'p>(this: Self, pool: AutoreleasePool<'p>) -> &'p T {
         let ptr = Self::autorelease_ptr(this);
         // SAFETY: The pointer is valid as a reference
         unsafe { pool.ptr_as_ref(ptr) }
@@ -569,14 +578,10 @@ impl<T: Message> Retained<T> {
     /// will often find yourself in need of returning autoreleased objects to
     /// properly follow [Cocoa's Memory Management Policy][mmRules].
     ///
-    /// To that end, you could use [`Retained::autorelease`], but that would require
-    /// you to have an [`AutoreleasePool`] object at hand, which you often
-    /// won't have in such cases. This function doesn't require a `pool`
-    /// object (but as a downside returns a pointer instead of a reference).
-    ///
-    /// This is also more efficient than a normal `autorelease`, since it
-    /// makes a best effort attempt to hand off ownership of the retain count
-    /// to a subsequent call to `objc_retainAutoreleasedReturnValue` /
+    /// To that end, you could also use [`Retained::autorelease_ptr`], but
+    /// this is more efficient than a normal `autorelease`, since it makes a
+    /// best effort attempt to hand off ownership of the retain count to a
+    /// subsequent call to `objc_retainAutoreleasedReturnValue` /
     /// [`Retained::retain_autoreleased`] in the enclosing call frame.
     ///
     /// This optimization relies heavily on this function being tail called,
@@ -803,7 +808,7 @@ mod tests {
         let mut expected = ThreadTestData::current();
 
         autoreleasepool(|pool| {
-            let _ref = Retained::autorelease(obj, pool);
+            let _ref = unsafe { Retained::autorelease(obj, pool) };
             expected.autorelease += 1;
             expected.assert_current();
             assert_eq!(cloned.retainCount(), 2);
@@ -813,7 +818,7 @@ mod tests {
         assert_eq!(cloned.retainCount(), 1);
 
         autoreleasepool(|pool| {
-            let _ref = Retained::autorelease(cloned, pool);
+            let _ref = unsafe { Retained::autorelease(cloned, pool) };
             expected.autorelease += 1;
             expected.assert_current();
         });

crates/objc2/src/rc/mod.rs

@@ -39,7 +39,8 @@
 //! autoreleasepool(|pool| {
 //!     // Autorelease consumes the Retained, but won't actually
 //!     // release it until the end of the autoreleasepool
-//!     let obj_ref: &NSObject = Retained::autorelease(cloned, pool);
+//!     // SAFETY: The given is the innermost pool.
+//!     let obj_ref: &NSObject = unsafe { Retained::autorelease(cloned, pool) };
 //! });
 //!
 //! // Weak references won't retain the object

crates/objc2/src/topics/about_generated/CHANGELOG.md

@@ -48,6 +48,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 * **BREAKING**: Removed `NSSet` methods `contains`, `is_subset`, `is_superset`
   and `is_disjoint` that were simple wrappers over the original methods.
 * **BREAKING**: Renamed `from_id_slice` to `from_retained_slice`.
+* **BREAKING**: Renamed `NSString::as_str` to `to_str`, and made it `unsafe`,
+  since we cannot ensure that the given pool is actually the innermost pool.
 
 ### Deprecated
 * Moved `MainThreadMarker` from `objc2-foundation` to `objc2`.

crates/test-fuzz/fuzz_targets/nsstring.rs

@@ -3,9 +3,9 @@ use objc2::rc::autoreleasepool;
 use objc2_foundation::NSString;
 
 fn run(s: &str) {
-    autoreleasepool(|pool| {
+    autoreleasepool(|pool| unsafe {
         let obj = NSString::from_str(s);
-        assert_eq!(obj.as_str(pool), s);
+        assert_eq!(obj.to_str(pool), s);
     });
 }
 

crates/test-ui/ui/nsstring_as_str_use_after_release.rs

@@ -3,9 +3,9 @@ use objc2::rc::autoreleasepool;
 use objc2_foundation::NSString;
 
 fn main() {
-    autoreleasepool(|pool| {
+    autoreleasepool(|pool| unsafe {
         let ns_string = NSString::new();
-        let s = ns_string.as_str(pool);
+        let s = ns_string.to_str(pool);
         drop(ns_string);
         println!("{}", s);
     });

crates/test-ui/ui/nsstring_as_str_use_after_release.stderr

@@ -4,5 +4,5 @@ use objc2_foundation::NSString;
 
 fn main() {
     let ns_string = NSString::new();
-    let _s = autoreleasepool(|pool| ns_string.as_str(pool));
+    let _s = autoreleasepool(|pool| unsafe { ns_string.to_str(pool) });
 }