Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 30, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 19 additions & 16 deletions Sources/Subprocess/API.swift
Original file line number Diff line number Diff line change
Expand Up @@ -644,39 +644,40 @@ 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()
let processError = FileDescriptorOutput(
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(
Expand All @@ -687,23 +688,23 @@ 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,
closeAfterSpawningProcess: false
)
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
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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
}

10 changes: 4 additions & 6 deletions Sources/Subprocess/Configuration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<Result, Error>
do {
Expand All @@ -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 {
Expand All @@ -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())
}
Expand Down
14 changes: 14 additions & 0 deletions Sources/Subprocess/Execution.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
6 changes: 3 additions & 3 deletions Sources/Subprocess/Platforms/Subprocess+Darwin.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions Sources/Subprocess/Platforms/Subprocess+Linux.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
12 changes: 0 additions & 12 deletions Sources/Subprocess/Platforms/Subprocess+Unix.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading
Loading