Release file handles back to caller on failure to take ownership (#2715)

Motivation:

When taking ownership of input and output file descriptors in a bootstrap
the ownership of the file handles remains with the caller in the function
taking ownership fails.  This is currently where _takingOwnershipOfDescriptors
uses NIOFileHandle to take ownership of descriptors but then experiences a later
failure.

Modifications:

If an exception is throw in _takingOwnershipOfDescriptors release file descriptors
from NIOFileHandle.

Result:

No crash on failure to close file handles before end of scoped usage.

Author: PeterAdams-A

Sources/NIOPosix/Bootstrap.swift

@@ -1926,6 +1926,7 @@ public final class NIOPipeBootstrap {
     private var channelInitializer: Optional<ChannelInitializerCallback>
     @usableFromInline
     internal var _channelOptions: ChannelOptions.Storage
+    private let hooks: any NIOPipeBootstrapHooks
 
     /// Create a `NIOPipeBootstrap` on the `EventLoopGroup` `group`.
     ///
@@ -1956,6 +1957,19 @@ public final class NIOPipeBootstrap {
         self._channelOptions = ChannelOptions.Storage()
         self.group = group
         self.channelInitializer = nil
+        self.hooks = DefaultNIOPipeBootstrapHooks()
+    }
+
+    /// Initialiser for hooked testing
+    init?(validatingGroup group: EventLoopGroup, hooks: any NIOPipeBootstrapHooks) {
+        guard NIOOnSocketsBootstraps.isCompatible(group: group) else {
+            return nil
+        }
+
+        self._channelOptions = ChannelOptions.Storage()
+        self.group = group
+        self.channelInitializer = nil
+        self.hooks = hooks
     }
 
     /// Initialize the connected `PipeChannel` with `initializer`. The most common task in initializer is to add
@@ -2281,11 +2295,18 @@ extension NIOPipeBootstrap {
 
             inputFileHandle = input.flatMap { NIOFileHandle(descriptor: $0) }
             outputFileHandle = output.flatMap { NIOFileHandle(descriptor: $0) }
-            channel = try PipeChannel(
-                eventLoop: eventLoop as! SelectableEventLoop,
-                inputPipe: inputFileHandle,
-                outputPipe: outputFileHandle
-            )
+            do {
+                channel = try self.hooks.makePipeChannel(
+                    eventLoop: eventLoop as! SelectableEventLoop,
+                    inputPipe: inputFileHandle,
+                    outputPipe: outputFileHandle
+                )
+            } catch {
+                // Release file handles back to the caller in case of failure.
+                _ = try? inputFileHandle?.takeDescriptorOwnership()
+                _ = try? outputFileHandle?.takeDescriptorOwnership()
+                throw error
+            }
         } catch {
             return eventLoop.makeFailedFuture(error)
         }
@@ -2327,3 +2348,17 @@ extension NIOPipeBootstrap {
 
 @available(*, unavailable)
 extension NIOPipeBootstrap: Sendable {}
+
+protocol NIOPipeBootstrapHooks {
+    func makePipeChannel(eventLoop: SelectableEventLoop,
+                         inputPipe: NIOFileHandle?,
+                         outputPipe: NIOFileHandle?) throws -> PipeChannel
+}
+
+fileprivate struct DefaultNIOPipeBootstrapHooks: NIOPipeBootstrapHooks {
+    func makePipeChannel(eventLoop: SelectableEventLoop,
+                         inputPipe: NIOFileHandle?,
+                         outputPipe: NIOFileHandle?) throws -> PipeChannel {
+        return try PipeChannel(eventLoop: eventLoop, inputPipe: inputPipe, outputPipe: outputPipe)
+    }
+}

Tests/NIOPosixTests/BootstrapTest.swift

@@ -680,6 +680,30 @@ class BootstrapTest: XCTestCase {
                                 .wait())
         }
     }
+
+    // There was a bug where file handle ownership was not released when creating pipe channels failed.
+    func testReleaseFileHandleOnOwningFailure() {
+        struct NIOPipeBootstrapHooksChannelFail: NIOPipeBootstrapHooks {
+            func makePipeChannel(eventLoop: NIOPosix.SelectableEventLoop, inputPipe: NIOCore.NIOFileHandle?, outputPipe: NIOCore.NIOFileHandle?) throws -> NIOPosix.PipeChannel {
+                throw IOError(errnoCode: EBADF, reason: "testing")
+            }
+        }
+
+        let sock = socket(NIOBSDSocket.ProtocolFamily.local.rawValue, NIOBSDSocket.SocketType.stream.rawValue, 0)
+        defer {
+            close(sock)
+        }
+        let elg = MultiThreadedEventLoopGroup(numberOfThreads: 1)
+        defer {
+            try! elg.syncShutdownGracefully()
+        }
+
+        let bootstrap = NIOPipeBootstrap(validatingGroup: elg, hooks: NIOPipeBootstrapHooksChannelFail())
+        XCTAssertNotNil(bootstrap)
+
+        let channelFuture = bootstrap?.takingOwnershipOfDescriptor(inputOutput: sock)
+        XCTAssertThrowsError(try channelFuture?.wait())
+    }
 }
 
 private final class WriteStringOnChannelActive: ChannelInboundHandler {