From c03685bd37da2e947cf8d6c2bacfa5aa1807fc0b Mon Sep 17 00:00:00 2001
From: Charles Hu <charles@hu.codes>
Date: Wed, 11 Jun 2025 21:47:14 -0700
Subject: [PATCH] Fatal error if TrackedFileDescriptor and TrackedDispatchIO
 have not been closed in deinit.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We cannot depend on calling .close() in deinit since it’s non-failable. As a result, Subprocess needs to manually call .close() at the conclusion of the read/write operation. fatalError in deinit to ensures the FileDescriptor/DispatchIO will not leak.
---
 Sources/Subprocess/Configuration.swift | 23 ++---------------------
 Sources/Subprocess/IO/Output.swift     |  8 ++++++--
 2 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/Sources/Subprocess/Configuration.swift b/Sources/Subprocess/Configuration.swift
index af76c46b..aaeca182 100644
--- a/Sources/Subprocess/Configuration.swift
+++ b/Sources/Subprocess/Configuration.swift
@@ -632,26 +632,7 @@ internal struct TrackedFileDescriptor: ~Copyable {
             return
         }
 
-        do {
-            try fileDescriptor.close()
-        } catch {
-            guard let errno: Errno = error as? Errno else {
-                return
-            }
-            // Getting `.badFileDescriptor` suggests that the file descriptor
-            // might have been closed unexpectedly. This can pose security risks
-            // if another part of the code inadvertently reuses the same file descriptor
-            // number. This problem is especially concerning on Unix systems due to POSIX’s
-            // guarantee of using the lowest available file descriptor number, making reuse
-            // more probable. We use `fatalError` upon receiving `.badFileDescriptor`
-            // to prevent accidentally closing a different file descriptor.
-            guard errno != .badFileDescriptor else {
-                fatalError(
-                    "FileDescriptor \(fileDescriptor.rawValue) is already closed"
-                )
-            }
-            // Otherwise ignore the error
-        }
+        fatalError("FileDescriptor \(self.fileDescriptor.rawValue) was not closed")
     }
 
     internal func platformDescriptor() -> PlatformFileDescriptor {
@@ -695,7 +676,7 @@ internal struct TrackedDispatchIO: ~Copyable {
             return
         }
 
-        dispatchIO.close()
+        fatalError("DispatchIO \(self.dispatchIO) was not closed")
     }
 }
 #endif
diff --git a/Sources/Subprocess/IO/Output.swift b/Sources/Subprocess/IO/Output.swift
index 66f2dbb7..454eca4f 100644
--- a/Sources/Subprocess/IO/Output.swift
+++ b/Sources/Subprocess/IO/Output.swift
@@ -143,9 +143,12 @@ public struct BytesOutput: OutputProtocol {
         from diskIO: consuming TrackedPlatformDiskIO?
     ) async throws -> [UInt8] {
         #if os(Windows)
-        return try await diskIO?.fileDescriptor.read(upToLength: self.maxSize) ?? []
+        let result = try await diskIO?.fileDescriptor.read(upToLength: self.maxSize) ?? []
+        try diskIO?.safelyClose()
+        return result
         #else
         let result = try await diskIO!.dispatchIO.read(upToLength: self.maxSize)
+        try diskIO?.safelyClose()
         return result?.array() ?? []
         #endif
     }
@@ -261,12 +264,13 @@ extension OutputProtocol {
         if OutputType.self == Void.self {
             return () as! OutputType
         }
-
         #if os(Windows)
         let result = try await diskIO?.fileDescriptor.read(upToLength: self.maxSize)
+        try diskIO?.safelyClose()
         return try self.output(from: result ?? [])
         #else
         let result = try await diskIO!.dispatchIO.read(upToLength: self.maxSize)
+        try diskIO?.safelyClose()
         return try self.output(from: result ?? .empty)
         #endif
     }