Store the Windows process HANDLE in Execution to avoid pid reuse races

Unlike Unix, Windows doesn't have a "zombie" state where the numeric pid remains valid after the subprocess has exited. Instead, the numeric process ID becomes invalid immediately once the process has exited. Per the Windows documentation of GetProcessId and related APIs, "Until a process terminates, its process identifier uniquely identifies it on the system."

On Windows, instead of closing/discarding the process HANDLE immediately after CreateProcess returns, retain the HANDLE in Execution and only close it once monitorProcessTermination returns. This resolve a race condition where the process could have exited between the period where CloseProcess was called and monitorProcessTermination is called. Now we simply use the original process handle to wait for the process to exit and retrieve its exit code, rather than "reverse engineering" its HANDLE from its numeric pid using OpenProcess.

Closes #92

Author: jakepetroules

Sources/Subprocess/Configuration.swift

@@ -103,7 +103,7 @@ public struct Configuration: Sendable {
         // even if `body` throws, and we are not leaving zombie processes in the
         // process table which will cause the process termination monitoring thread
         // to effectively hang due to the pid never being awaited
-        let terminationStatus = try await Subprocess.monitorProcessTermination(forProcessWithIdentifier: processIdentifier)
+        let terminationStatus = try await Subprocess.monitorProcessTermination(forExecution: _spawnResult.execution)
 
         return try ExecutionResult(terminationStatus: terminationStatus, value: result.get())
     }

Sources/Subprocess/Execution.swift

@@ -35,13 +35,16 @@ public struct Execution: Sendable {
     public let processIdentifier: ProcessIdentifier
 
     #if os(Windows)
+    internal nonisolated(unsafe) let processHandle: HANDLE
     internal let consoleBehavior: PlatformOptions.ConsoleBehavior
 
     init(
         processIdentifier: ProcessIdentifier,
+        processHandle: HANDLE,
         consoleBehavior: PlatformOptions.ConsoleBehavior
     ) {
         self.processIdentifier = processIdentifier
+        self.processHandle = processHandle
         self.consoleBehavior = consoleBehavior
     }
     #else

Sources/Subprocess/Platforms/Subprocess+Darwin.swift

@@ -485,18 +485,18 @@ extension String {
 // MARK: - Process Monitoring
 @Sendable
 internal func monitorProcessTermination(
-    forProcessWithIdentifier pid: ProcessIdentifier
+    forExecution execution: Execution
 ) async throws -> TerminationStatus {
     return try await withCheckedThrowingContinuation { continuation in
         let source = DispatchSource.makeProcessSource(
-            identifier: pid.value,
+            identifier: execution.processIdentifier.value,
             eventMask: [.exit],
             queue: .global()
         )
         source.setEventHandler {
             source.cancel()
             var siginfo = siginfo_t()
-            let rc = waitid(P_PID, id_t(pid.value), &siginfo, WEXITED)
+            let rc = waitid(P_PID, id_t(execution.processIdentifier.value), &siginfo, WEXITED)
             guard rc == 0 else {
                 continuation.resume(
                     throwing: SubprocessError(

Sources/Subprocess/Platforms/Subprocess+Linux.swift

@@ -265,7 +265,7 @@ extension String {
 // MARK: - Process Monitoring
 @Sendable
 internal func monitorProcessTermination(
-    forProcessWithIdentifier pid: ProcessIdentifier
+    forExecution execution: Execution
 ) async throws -> TerminationStatus {
     try await withCheckedThrowingContinuation { continuation in
         _childProcessContinuations.withLock { continuations in
@@ -274,7 +274,7 @@ internal func monitorProcessTermination(
             // the child process has terminated and manages to acquire the lock before
             // we add this continuation to the dictionary, then it will simply loop
             // and report the status again.
-            let oldContinuation = continuations.updateValue(continuation, forKey: pid.value)
+            let oldContinuation = continuations.updateValue(continuation, forKey: execution.processIdentifier.value)
             precondition(oldContinuation == nil)
 
             // Wake up the waiter thread if it is waiting for more child processes.

Sources/Subprocess/Platforms/Subprocess+Windows.swift

@@ -132,21 +132,6 @@ extension Configuration {
                 underlyingError: .init(rawValue: windowsError)
             )
         }
-        guard CloseHandle(processInfo.hProcess) else {
-            let windowsError = GetLastError()
-            try self.safelyCloseMultiple(
-                inputRead: inputReadFileDescriptor,
-                inputWrite: inputWriteFileDescriptor,
-                outputRead: outputReadFileDescriptor,
-                outputWrite: outputWriteFileDescriptor,
-                errorRead: errorReadFileDescriptor,
-                errorWrite: errorWriteFileDescriptor
-            )
-            throw SubprocessError(
-                code: .init(.spawnFailed),
-                underlyingError: .init(rawValue: windowsError)
-            )
-        }
 
         try self.safelyCloseMultiple(
             inputRead: inputReadFileDescriptor,
@@ -162,6 +147,7 @@ extension Configuration {
         )
         let execution = Execution(
             processIdentifier: pid,
+            processHandle: processInfo.hProcess,
             consoleBehavior: self.platformOptions.consoleBehavior
         )
         return SpawnResult(
@@ -275,21 +261,6 @@ extension Configuration {
                 underlyingError: .init(rawValue: windowsError)
             )
         }
-        guard CloseHandle(processInfo.hProcess) else {
-            let windowsError = GetLastError()
-            try self.safelyCloseMultiple(
-                inputRead: inputReadFileDescriptor,
-                inputWrite: inputWriteFileDescriptor,
-                outputRead: outputReadFileDescriptor,
-                outputWrite: outputWriteFileDescriptor,
-                errorRead: errorReadFileDescriptor,
-                errorWrite: errorWriteFileDescriptor
-            )
-            throw SubprocessError(
-                code: .init(.spawnFailed),
-                underlyingError: .init(rawValue: windowsError)
-            )
-        }
 
         // After spawn finishes, close all child side fds
         try self.safelyCloseMultiple(
@@ -306,6 +277,7 @@ extension Configuration {
         )
         let execution = Execution(
             processIdentifier: pid,
+            processHandle: processInfo.hProcess,
             consoleBehavior: self.platformOptions.consoleBehavior
         )
         return SpawnResult(
@@ -464,7 +436,7 @@ extension PlatformOptions: CustomStringConvertible, CustomDebugStringConvertible
 // MARK: - Process Monitoring
 @Sendable
 internal func monitorProcessTermination(
-    forProcessWithIdentifier pid: ProcessIdentifier
+    forExecution execution: Execution
 ) async throws -> TerminationStatus {
     // Once the continuation resumes, it will need to unregister the wait, so
     // yield the wait handle back to the calling scope.
@@ -473,15 +445,8 @@ internal func monitorProcessTermination(
         if let waitHandle {
             _ = UnregisterWait(waitHandle)
         }
-    }
-    guard
-        let processHandle = OpenProcess(
-            DWORD(PROCESS_QUERY_INFORMATION | SYNCHRONIZE),
-            false,
-            pid.value
-        )
-    else {
-        return .exited(1)
+
+        _ = CloseHandle(execution.processHandle)
     }
 
     try? await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Void, any Error>) in
@@ -500,7 +465,7 @@ internal func monitorProcessTermination(
         guard
             RegisterWaitForSingleObject(
                 &waitHandle,
-                processHandle,
+                execution.processHandle,
                 callback,
                 context,
                 INFINITE,
@@ -518,7 +483,7 @@ internal func monitorProcessTermination(
     }
 
     var status: DWORD = 0
-    guard GetExitCodeProcess(processHandle, &status) else {
+    guard GetExitCodeProcess(execution.processHandle, &status) else {
         // The child process terminated but we couldn't get its status back.
         // Assume generic failure.
         return .exited(1)