From 642235059cc79a5fd09798cacf2ba2cd3ddb6a47 Mon Sep 17 00:00:00 2001 From: Jake Petroules Date: Sat, 28 Jun 2025 16:38:47 -0700 Subject: [PATCH] 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 --- Sources/Subprocess/API.swift | 35 +-- Sources/Subprocess/Configuration.swift | 10 +- Sources/Subprocess/Execution.swift | 14 ++ .../Platforms/Subprocess+Darwin.swift | 6 +- .../Platforms/Subprocess+Linux.swift | 4 +- .../Platforms/Subprocess+Unix.swift | 12 -- .../Platforms/Subprocess+Windows.swift | 199 ++++-------------- Sources/Subprocess/Teardown.swift | 38 +--- 8 files changed, 94 insertions(+), 224 deletions(-) diff --git a/Sources/Subprocess/API.swift b/Sources/Subprocess/API.swift index d40c747..b7788d6 100644 --- a/Sources/Subprocess/API.swift +++ b/Sources/Subprocess/API.swift @@ -644,16 +644,17 @@ public func runDetached( output: FileDescriptor? = nil, error: FileDescriptor? = nil ) throws -> ProcessIdentifier { + let execution: Execution switch (input, output, error) { case (.none, .none, .none): let processInput = NoInput() let processOutput = DiscardedOutput() let processError = DiscardedOutput() - return try configuration.spawn( + execution = try configuration.spawn( withInput: try processInput.createPipe(), outputPipe: try processOutput.createPipe(), errorPipe: try processError.createPipe() - ).execution.processIdentifier + ).execution case (.none, .none, .some(let errorFd)): let processInput = NoInput() let processOutput = DiscardedOutput() @@ -661,22 +662,22 @@ public func runDetached( fileDescriptor: errorFd, closeAfterSpawningProcess: false ) - return try configuration.spawn( + execution = try configuration.spawn( withInput: try processInput.createPipe(), outputPipe: try processOutput.createPipe(), errorPipe: try processError.createPipe() - ).execution.processIdentifier + ).execution case (.none, .some(let outputFd), .none): let processInput = NoInput() let processOutput = FileDescriptorOutput( fileDescriptor: outputFd, closeAfterSpawningProcess: false ) let processError = DiscardedOutput() - return try configuration.spawn( + execution = try configuration.spawn( withInput: try processInput.createPipe(), outputPipe: try processOutput.createPipe(), errorPipe: try processError.createPipe() - ).execution.processIdentifier + ).execution case (.none, .some(let outputFd), .some(let errorFd)): let processInput = NoInput() let processOutput = FileDescriptorOutput( @@ -687,11 +688,11 @@ public func runDetached( fileDescriptor: errorFd, closeAfterSpawningProcess: false ) - return try configuration.spawn( + execution = try configuration.spawn( withInput: try processInput.createPipe(), outputPipe: try processOutput.createPipe(), errorPipe: try processError.createPipe() - ).execution.processIdentifier + ).execution case (.some(let inputFd), .none, .none): let processInput = FileDescriptorInput( fileDescriptor: inputFd, @@ -699,11 +700,11 @@ public func runDetached( ) let processOutput = DiscardedOutput() let processError = DiscardedOutput() - return try configuration.spawn( + execution = try configuration.spawn( withInput: try processInput.createPipe(), outputPipe: try processOutput.createPipe(), errorPipe: try processError.createPipe() - ).execution.processIdentifier + ).execution case (.some(let inputFd), .none, .some(let errorFd)): let processInput = FileDescriptorInput( fileDescriptor: inputFd, closeAfterSpawningProcess: false @@ -713,11 +714,11 @@ public func runDetached( fileDescriptor: errorFd, closeAfterSpawningProcess: false ) - return try configuration.spawn( + execution = try configuration.spawn( withInput: try processInput.createPipe(), outputPipe: try processOutput.createPipe(), errorPipe: try processError.createPipe() - ).execution.processIdentifier + ).execution case (.some(let inputFd), .some(let outputFd), .none): let processInput = FileDescriptorInput( fileDescriptor: inputFd, @@ -728,11 +729,11 @@ public func runDetached( closeAfterSpawningProcess: false ) let processError = DiscardedOutput() - return try configuration.spawn( + execution = try configuration.spawn( withInput: try processInput.createPipe(), outputPipe: try processOutput.createPipe(), errorPipe: try processError.createPipe() - ).execution.processIdentifier + ).execution case (.some(let inputFd), .some(let outputFd), .some(let errorFd)): let processInput = FileDescriptorInput( fileDescriptor: inputFd, @@ -746,11 +747,13 @@ public func runDetached( fileDescriptor: errorFd, closeAfterSpawningProcess: false ) - return try configuration.spawn( + execution = try configuration.spawn( withInput: try processInput.createPipe(), outputPipe: try processOutput.createPipe(), errorPipe: try processError.createPipe() - ).execution.processIdentifier + ).execution } + execution.release() + return execution.processIdentifier } diff --git a/Sources/Subprocess/Configuration.swift b/Sources/Subprocess/Configuration.swift index 2b30ce0..5396506 100644 --- a/Sources/Subprocess/Configuration.swift +++ b/Sources/Subprocess/Configuration.swift @@ -71,12 +71,11 @@ public struct Configuration: Sendable { outputPipe: output, errorPipe: error ) - let pid = spawnResults.execution.processIdentifier var spawnResultBox: SpawnResult?? = consume spawnResults var _spawnResult = spawnResultBox!.take()! - let processIdentifier = _spawnResult.execution.processIdentifier + let execution = _spawnResult.execution let result: Swift.Result do { @@ -89,9 +88,8 @@ public struct Configuration: Sendable { return try await body(_spawnResult.execution, inputIO, outputIO, errorIO) } onCleanup: { // Attempt to terminate the child process - await Execution.runTeardownSequence( - self.platformOptions.teardownSequence, - on: pid + await execution.runTeardownSequence( + self.platformOptions.teardownSequence ) }) } catch { @@ -103,7 +101,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()) } diff --git a/Sources/Subprocess/Execution.swift b/Sources/Subprocess/Execution.swift index 0aa06a7..a21a170 100644 --- a/Sources/Subprocess/Execution.swift +++ b/Sources/Subprocess/Execution.swift @@ -35,13 +35,16 @@ public struct Execution: Sendable { public let processIdentifier: ProcessIdentifier #if os(Windows) + internal nonisolated(unsafe) let processInformation: PROCESS_INFORMATION internal let consoleBehavior: PlatformOptions.ConsoleBehavior init( processIdentifier: ProcessIdentifier, + processInformation: PROCESS_INFORMATION, consoleBehavior: PlatformOptions.ConsoleBehavior ) { self.processIdentifier = processIdentifier + self.processInformation = processInformation self.consoleBehavior = consoleBehavior } #else @@ -51,6 +54,17 @@ public struct Execution: Sendable { self.processIdentifier = processIdentifier } #endif // os(Windows) + + internal func release() { + #if os(Windows) + guard CloseHandle(processInformation.hThread) else { + fatalError("Failed to close thread HANDLE: \(SubprocessError.UnderlyingError(rawValue: GetLastError()))") + } + guard CloseHandle(processInformation.hProcess) else { + fatalError("Failed to close process HANDLE: \(SubprocessError.UnderlyingError(rawValue: GetLastError()))") + } + #endif + } } // MARK: - Output Capture diff --git a/Sources/Subprocess/Platforms/Subprocess+Darwin.swift b/Sources/Subprocess/Platforms/Subprocess+Darwin.swift index a45e3e1..7cd32c7 100644 --- a/Sources/Subprocess/Platforms/Subprocess+Darwin.swift +++ b/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( diff --git a/Sources/Subprocess/Platforms/Subprocess+Linux.swift b/Sources/Subprocess/Platforms/Subprocess+Linux.swift index 7d4880e..125ba46 100644 --- a/Sources/Subprocess/Platforms/Subprocess+Linux.swift +++ b/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. diff --git a/Sources/Subprocess/Platforms/Subprocess+Unix.swift b/Sources/Subprocess/Platforms/Subprocess+Unix.swift index 5159e0a..268d32e 100644 --- a/Sources/Subprocess/Platforms/Subprocess+Unix.swift +++ b/Sources/Subprocess/Platforms/Subprocess+Unix.swift @@ -117,18 +117,6 @@ extension Execution { public func send( signal: Signal, toProcessGroup shouldSendToProcessGroup: Bool = false - ) throws { - try Self.send( - signal: signal, - to: self.processIdentifier, - toProcessGroup: shouldSendToProcessGroup - ) - } - - internal static func send( - signal: Signal, - to processIdentifier: ProcessIdentifier, - toProcessGroup shouldSendToProcessGroup: Bool ) throws { let pid = shouldSendToProcessGroup ? -(processIdentifier.value) : processIdentifier.value guard kill(pid, signal.rawValue) == 0 else { diff --git a/Sources/Subprocess/Platforms/Subprocess+Windows.swift b/Sources/Subprocess/Platforms/Subprocess+Windows.swift index d65b2ea..70afa33 100644 --- a/Sources/Subprocess/Platforms/Subprocess+Windows.swift +++ b/Sources/Subprocess/Platforms/Subprocess+Windows.swift @@ -116,54 +116,33 @@ extension Configuration { } } } - // We don't need the handle objects, so close it right away - guard CloseHandle(processInfo.hThread) 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) - ) - } - 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, - inputWrite: nil, - outputRead: nil, - outputWrite: outputWriteFileDescriptor, - errorRead: nil, - errorWrite: errorWriteFileDescriptor - ) let pid = ProcessIdentifier( value: processInfo.dwProcessId ) let execution = Execution( processIdentifier: pid, + processInformation: processInfo, consoleBehavior: self.platformOptions.consoleBehavior ) + + do { + // After spawn finishes, close all child side fds + try self.safelyCloseMultiple( + inputRead: inputReadFileDescriptor, + inputWrite: nil, + outputRead: nil, + outputWrite: outputWriteFileDescriptor, + errorRead: nil, + errorWrite: errorWriteFileDescriptor + ) + } catch { + // If spawn() throws, monitorProcessTermination or runDetached + // won't have an opportunity to call release, so do it here to avoid leaking the handles. + execution.release() + throw error + } + return SpawnResult( execution: execution, inputWriteEnd: inputWriteFileDescriptor?.createPlatformDiskIO(), @@ -259,55 +238,33 @@ extension Configuration { } } } - // We don't need the handle objects, so close it right away - guard CloseHandle(processInfo.hThread) 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) - ) - } - 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( - inputRead: inputReadFileDescriptor, - inputWrite: nil, - outputRead: nil, - outputWrite: outputWriteFileDescriptor, - errorRead: nil, - errorWrite: errorWriteFileDescriptor - ) let pid = ProcessIdentifier( value: processInfo.dwProcessId ) let execution = Execution( processIdentifier: pid, + processInformation: processInfo, consoleBehavior: self.platformOptions.consoleBehavior ) + + do { + // After spawn finishes, close all child side fds + try self.safelyCloseMultiple( + inputRead: inputReadFileDescriptor, + inputWrite: nil, + outputRead: nil, + outputWrite: outputWriteFileDescriptor, + errorRead: nil, + errorWrite: errorWriteFileDescriptor + ) + } catch { + // If spawn() throws, monitorProcessTermination or runDetached + // won't have an opportunity to call release, so do it here to avoid leaking the handles. + execution.release() + throw error + } + return SpawnResult( execution: execution, inputWriteEnd: inputWriteFileDescriptor?.createPlatformDiskIO(), @@ -464,7 +421,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 +430,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) + + execution.release() } try? await withCheckedThrowingContinuation { (continuation: CheckedContinuation) in @@ -500,7 +450,7 @@ internal func monitorProcessTermination( guard RegisterWaitForSingleObject( &waitHandle, - processHandle, + execution.processInformation.hProcess, callback, context, INFINITE, @@ -518,7 +468,7 @@ internal func monitorProcessTermination( } var status: DWORD = 0 - guard GetExitCodeProcess(processHandle, &status) else { + guard GetExitCodeProcess(execution.processInformation.hProcess, &status) else { // The child process terminated but we couldn't get its status back. // Assume generic failure. return .exited(1) @@ -536,30 +486,7 @@ extension Execution { /// Terminate the current subprocess with the given exit code /// - Parameter exitCode: The exit code to use for the subprocess. public func terminate(withExitCode exitCode: DWORD) throws { - try Self.terminate(self.processIdentifier, withExitCode: exitCode) - } - - internal static func terminate( - _ processIdentifier: ProcessIdentifier, - withExitCode exitCode: DWORD - ) throws { - guard - let processHandle = OpenProcess( - // PROCESS_ALL_ACCESS - DWORD(STANDARD_RIGHTS_REQUIRED | SYNCHRONIZE | 0xFFFF), - false, - processIdentifier.value - ) - else { - throw SubprocessError( - code: .init(.failedToTerminate), - underlyingError: .init(rawValue: GetLastError()) - ) - } - defer { - CloseHandle(processHandle) - } - guard TerminateProcess(processHandle, exitCode) else { + guard TerminateProcess(processInformation.hProcess, exitCode) else { throw SubprocessError( code: .init(.failedToTerminate), underlyingError: .init(rawValue: GetLastError()) @@ -569,23 +496,6 @@ extension Execution { /// Suspend the current subprocess public func suspend() throws { - guard - let processHandle = OpenProcess( - // PROCESS_ALL_ACCESS - DWORD(STANDARD_RIGHTS_REQUIRED | SYNCHRONIZE | 0xFFFF), - false, - self.processIdentifier.value - ) - else { - throw SubprocessError( - code: .init(.failedToSuspend), - underlyingError: .init(rawValue: GetLastError()) - ) - } - defer { - CloseHandle(processHandle) - } - let NTSuspendProcess: (@convention(c) (HANDLE) -> LONG)? = unsafeBitCast( GetProcAddress( @@ -600,7 +510,7 @@ extension Execution { underlyingError: .init(rawValue: GetLastError()) ) } - guard NTSuspendProcess(processHandle) >= 0 else { + guard NTSuspendProcess(processInformation.hProcess) >= 0 else { throw SubprocessError( code: .init(.failedToSuspend), underlyingError: .init(rawValue: GetLastError()) @@ -610,23 +520,6 @@ extension Execution { /// Resume the current subprocess after suspension public func resume() throws { - guard - let processHandle = OpenProcess( - // PROCESS_ALL_ACCESS - DWORD(STANDARD_RIGHTS_REQUIRED | SYNCHRONIZE | 0xFFFF), - false, - self.processIdentifier.value - ) - else { - throw SubprocessError( - code: .init(.failedToResume), - underlyingError: .init(rawValue: GetLastError()) - ) - } - defer { - CloseHandle(processHandle) - } - let NTResumeProcess: (@convention(c) (HANDLE) -> LONG)? = unsafeBitCast( GetProcAddress( @@ -641,7 +534,7 @@ extension Execution { underlyingError: .init(rawValue: GetLastError()) ) } - guard NTResumeProcess(processHandle) >= 0 else { + guard NTResumeProcess(processInformation.hProcess) >= 0 else { throw SubprocessError( code: .init(.failedToResume), underlyingError: .init(rawValue: GetLastError()) diff --git a/Sources/Subprocess/Teardown.swift b/Sources/Subprocess/Teardown.swift index ec452d7..a4c0352 100644 --- a/Sources/Subprocess/Teardown.swift +++ b/Sources/Subprocess/Teardown.swift @@ -78,15 +78,8 @@ extension Execution { /// Teardown sequence always ends with a `.kill` signal /// - Parameter sequence: The steps to perform. public func teardown(using sequence: some Sequence & Sendable) async { - await Self.runTeardownSequence(sequence, on: self.processIdentifier) - } - - internal static func teardown( - using sequence: some Sequence & Sendable, - on processIdentifier: ProcessIdentifier - ) async { await withUncancelledTask { - await Self.runTeardownSequence(sequence, on: processIdentifier) + await runTeardownSequence(sequence) } } } @@ -98,25 +91,10 @@ internal enum TeardownStepCompletion { } extension Execution { - internal static func gracefulShutDown( - _ processIdentifier: ProcessIdentifier, + internal func gracefulShutDown( allowedDurationToNextStep duration: Duration ) async { #if os(Windows) - guard - let processHandle = OpenProcess( - DWORD(PROCESS_QUERY_INFORMATION | SYNCHRONIZE), - false, - processIdentifier.value - ) - else { - // Nothing more we can do - return - } - defer { - CloseHandle(processHandle) - } - // 1. Attempt to send WM_CLOSE to the main window if _subprocess_windows_send_vm_close( processIdentifier.value @@ -148,15 +126,13 @@ extension Execution { // Send SIGTERM try? self.send( signal: .terminate, - to: processIdentifier, toProcessGroup: false ) #endif } - internal static func runTeardownSequence( - _ sequence: some Sequence & Sendable, - on processIdentifier: ProcessIdentifier + internal func runTeardownSequence( + _ sequence: some Sequence & Sendable ) async { // First insert the `.kill` step let finalSequence = sequence + [TeardownStep(storage: .kill)] @@ -177,7 +153,6 @@ extension Execution { } } await self.gracefulShutDown( - processIdentifier, allowedDurationToNextStep: allowedDuration ) return await group.next()! @@ -195,18 +170,17 @@ extension Execution { return .processHasExited } } - try? self.send(signal: signal, to: processIdentifier, toProcessGroup: false) + try? self.send(signal: signal, toProcessGroup: false) return await group.next()! } #endif // !os(Windows) case .kill: #if os(Windows) try? self.terminate( - processIdentifier, withExitCode: 0 ) #else - try? self.send(signal: .kill, to: processIdentifier, toProcessGroup: false) + try? self.send(signal: .kill, toProcessGroup: false) #endif stepCompletion = .killedTheProcess }