Manage early-exit of ByteBufferBIO in tests. (#286)

Motivation:

Nightly Swift's improvement to the copy propagation pass has caused
ByteBufferBIOTests to start misbehaving, as they now deallocate
some ByteBufferBIOs very early in the test case. This is because the
backing BIO pointer does not have a strong reference to the
ByteBufferBIO.

Modifications:

- Give the backing BIO a strong ref that can be broken by the
    ByteBufferBIO calling close().
- Update the tests to call close()

Result:

No leaking BIOs.

Author: Lukasa

Sources/NIOSSL/ByteBufferBIO.swift

@@ -217,21 +217,36 @@ final class ByteBufferBIO {
             preconditionFailure("Unable to initialize custom BIO")
         }
 
-        // We now need to complete the BIO initialization. The BIO does not have an owned pointer
-        // to us, as that would create an annoying-to-break reference cycle.
+        // We now need to complete the BIO initialization. The BIO takes an owned reference to self here,
+        // which is broken on close().
         self.bioPtr = bio
-        CNIOBoringSSL_BIO_set_data(self.bioPtr, Unmanaged.passUnretained(self).toOpaque())
+        CNIOBoringSSL_BIO_set_data(self.bioPtr, Unmanaged.passRetained(self).toOpaque())
         CNIOBoringSSL_BIO_set_init(self.bioPtr, 1)
         CNIOBoringSSL_BIO_set_shutdown(self.bioPtr, 1)
     }
 
     deinit {
-        // On deinit we need to drop our reference to the BIO, and also ensure that it doesn't hold any
-        // pointers to this object anymore.
-        CNIOBoringSSL_BIO_set_data(self.bioPtr, nil)
+        // In debug mode we assert that we've been closed.
+        assert(CNIOBoringSSL_BIO_get_data(self.bioPtr) == nil, "must call close() on ByteBufferBIO before deinit")
+
+        // On deinit we need to drop our reference to the BIO.
         CNIOBoringSSL_BIO_free(self.bioPtr)
     }
 
+    /// Shuts down the BIO, rendering it unable to be used.
+    ///
+    /// This method is idempotent: it is safe to call more than once.
+    internal func close() {
+        guard let selfRef = CNIOBoringSSL_BIO_get_data(self.bioPtr) else {
+            // Shutdown is safe to call more than once.
+            return
+        }
+
+        // We consume the original retain of self, and then nil out the ref in the BIO so that this can't happen again.
+        Unmanaged<ByteBufferBIO>.fromOpaque(selfRef).release()
+        CNIOBoringSSL_BIO_set_data(self.bioPtr, nil)
+    }
+
     /// Obtain an owned pointer to the backing BoringSSL BIO object.
     ///
     /// This pointer is safe to use elsewhere, as it has increased the reference to the backing

Sources/NIOSSL/SSLConnection.swift

@@ -403,6 +403,9 @@ internal final class SSLConnection {
 
         // Also drop the reference to the parent channel handler, which is a trivial reference cycle.
         self.parentHandler = nil
+
+        // And finally drop the data stored by the bytebuffer BIO
+        self.bio?.close()
     }
 
     /// Retrieves any inbound data that has not been processed by BoringSSL.

Tests/NIOSSLTests/ByteBufferBIOTest.swift

@@ -29,8 +29,10 @@ final class ByteBufferBIOTest: XCTestCase {
         }
     }
 
+    /// This leaks on purpose!
     private func retainedBIO() -> UnsafeMutablePointer<BIO> {
         let swiftBIO = ByteBufferBIO(allocator: ByteBufferAllocator())
+        swiftBIO.close()
         return swiftBIO.retainedBIO()
     }
 
@@ -39,6 +41,7 @@ final class ByteBufferBIOTest: XCTestCase {
         let cBIO = swiftBIO.retainedBIO()
         defer {
             CNIOBoringSSL_BIO_free(cBIO)
+            swiftBIO.close()
         }
 
         XCTAssertNil(swiftBIO.outboundCiphertext())
@@ -60,6 +63,7 @@ final class ByteBufferBIOTest: XCTestCase {
         let cBIO = swiftBIO.retainedBIO()
         defer {
             CNIOBoringSSL_BIO_free(cBIO)
+            swiftBIO.close()
         }
 
         XCTAssertNil(swiftBIO.outboundCiphertext())
@@ -85,6 +89,7 @@ final class ByteBufferBIOTest: XCTestCase {
         let cBIO = swiftBIO.retainedBIO()
         defer {
             CNIOBoringSSL_BIO_free(cBIO)
+            swiftBIO.close()
         }
 
         var targetBuffer = [UInt8](repeating: 0, count: 512)
@@ -100,6 +105,7 @@ final class ByteBufferBIOTest: XCTestCase {
         let cBIO = swiftBIO.retainedBIO()
         defer {
             CNIOBoringSSL_BIO_free(cBIO)
+            swiftBIO.close()
         }
 
         var inboundBytes = ByteBufferAllocator().buffer(capacity: 1024)
@@ -129,6 +135,7 @@ final class ByteBufferBIOTest: XCTestCase {
         let cBIO = swiftBIO.retainedBIO()
         defer {
             CNIOBoringSSL_BIO_free(cBIO)
+            swiftBIO.close()
         }
 
         var inboundBytes = ByteBufferAllocator().buffer(capacity: 1024)
@@ -181,6 +188,7 @@ final class ByteBufferBIOTest: XCTestCase {
         let cBIO = swiftBIO.retainedBIO()
         defer {
             CNIOBoringSSL_BIO_free(cBIO)
+            swiftBIO.close()
         }
 
         var targetBuffer = [UInt8](repeating: 0, count: 512)
@@ -194,6 +202,7 @@ final class ByteBufferBIOTest: XCTestCase {
         let cBIO = swiftBIO.retainedBIO()
         defer {
             CNIOBoringSSL_BIO_free(cBIO)
+            swiftBIO.close()
         }
 
         var bytesToWrite: [UInt8] = [1, 2, 3, 4, 5]
@@ -227,6 +236,7 @@ final class ByteBufferBIOTest: XCTestCase {
         let cBIO = swiftBIO.retainedBIO()
         defer {
             CNIOBoringSSL_BIO_free(cBIO)
+            swiftBIO.close()
         }
 
         let firstAddress = writeAddress(swiftBIO: swiftBIO, cBIO: cBIO)
@@ -244,6 +254,7 @@ final class ByteBufferBIOTest: XCTestCase {
         let cBIO = swiftBIO.retainedBIO()
         defer {
             CNIOBoringSSL_BIO_free(cBIO)
+            swiftBIO.close()
         }
 
         var bytesToWrite: [UInt8] = [1, 2, 3, 4, 5]
@@ -266,6 +277,7 @@ final class ByteBufferBIOTest: XCTestCase {
         let cBIO = swiftBIO.retainedBIO()
         defer {
             CNIOBoringSSL_BIO_free(cBIO)
+            swiftBIO.close()
         }
 
         XCTAssertNil(swiftBIO.outboundCiphertext())
@@ -286,6 +298,7 @@ final class ByteBufferBIOTest: XCTestCase {
         let cBIO = swiftBIO.retainedBIO()
         defer {
             CNIOBoringSSL_BIO_free(cBIO)
+            swiftBIO.close()
         }
 
         var buffer = ByteBufferAllocator().buffer(capacity: 1024)
@@ -306,6 +319,7 @@ final class ByteBufferBIOTest: XCTestCase {
         let cBIO = swiftBIO.retainedBIO()
         defer {
             CNIOBoringSSL_BIO_free(cBIO)
+            swiftBIO.close()
         }
 
         let originalShutdown = CNIOBoringSSL_BIO_ctrl(cBIO, BIO_CTRL_GET_CLOSE, 0, nil)