Merge pull request #70945 from Azoy/fix-unmanaged-opaque

[stdlib] Avoid materializing strong references in potential dangling unmanaged opaque functions

Author: Azoy

stdlib/public/core/Unmanaged.swift

@@ -34,7 +34,15 @@ public struct Unmanaged<Instance: AnyObject> {
   public static func fromOpaque(
     @_nonEphemeral _ value: UnsafeRawPointer
   ) -> Unmanaged {
-    return Unmanaged(_private: unsafeBitCast(value, to: Instance.self))
+    // NOTE: `value` is allowed to represent a dangling reference, so 
+    // this function must not ever try to dereference it. For
+    // example, this function must NOT use the init(_private:) initializer
+    // because doing so requires materializing a strong reference to 'Instance'.
+    // This materialization would be enough to convince the compiler to add
+    // retain/releases which must be avoided for the opaque pointer functions.
+    // 'Unmanaged<Instance>' is layout compatible with 'UnsafeRawPointer' and
+    // casting to that will not attempt to retain the reference held at 'value'.
+    unsafeBitCast(value, to: Unmanaged<Instance>.self)
   }
 
   /// Unsafely converts an unmanaged class reference to a pointer.
@@ -48,7 +56,13 @@ public struct Unmanaged<Instance: AnyObject> {
   /// - Returns: An opaque pointer to the value of this unmanaged reference.
   @_transparent
   public func toOpaque() -> UnsafeMutableRawPointer {
-    return unsafeBitCast(_value, to: UnsafeMutableRawPointer.self)
+    // NOTE: `self` is allowed to be a dangling reference.
+    // Therefore, this function must not unsafeBitCast '_value' because
+    // that will get a strong reference temporary value that the compiler will
+    // try to retain/release. Use 'self' to avoid this. 'Unmanaged<Instance>' is
+    // layout compatible with 'UnsafeRawPointer' and casting from that will not
+    // attempt to retain the reference held at '_value'.
+    unsafeBitCast(self, to: UnsafeMutableRawPointer.self)
   }
 
   /// Creates an unmanaged reference with an unbalanced retain.

test/stdlib/Unmanaged.swift

@@ -71,4 +71,26 @@ UnmanagedTests.test("Opaque") {
   _fixLifetime(ref)
 }
 
+UnmanagedTests.test("Opaque avoid retain/release") {
+  // The purpose of this test is to ensure that the fromOpaque/toOpaque calls do
+  // NOT attempt to retain/release the class instance at the passed pointer.
+  // Here we're simulating a dangling pointer referencing a class who has
+  // already been released (the allocated pointer points at nothing) and
+  // attempting to create an Unmanaged instance from it and get back the
+  // pointer. This test's expectEqual is kind of bogus, we're just checking that
+  // it doesn't crash.
+
+  // Create a dangling pointer, usually to unmapped memory.
+  let ref = UnsafeRawPointer(bitPattern: 1)!
+  // Turn it into a dangling unmanaged reference.
+  // We expect this not to crash, as this operation isn't 
+  // supposed to dereference the pointer in any way.
+  let unmanaged = Unmanaged<Foobar>.fromOpaque(ref)
+  // Similarly, converting the unmanaged reference back to a 
+  // pointer should not ever try to dereference it either.
+  let ref2 = unmanaged.toOpaque()
+  // ...and we must get back the same pointer.
+  expectEqual(ref, ref2)
+}
+
 runAllTests()