Skip to content

Commit 2b8c265

Browse files
jakepetroulesjakepetroules
committed
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
1 parent 5ab5f7b commit 2b8c265

File tree

8 files changed

+94
-224
lines changed

8 files changed

+94
-224
lines changed

‎Sources/Subprocess/API.swift

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -644,39 +644,40 @@ public func runDetached(
644644
output: FileDescriptor? = nil,
645645
error: FileDescriptor? = nil
646646
) throws -> ProcessIdentifier {
647+
let execution: Execution
647648
switch (input, output, error) {
648649
case (.none, .none, .none):
649650
let processInput = NoInput()
650651
let processOutput = DiscardedOutput()
651652
let processError = DiscardedOutput()
652-
return try configuration.spawn(
653+
execution = try configuration.spawn(
653654
withInput: try processInput.createPipe(),
654655
outputPipe: try processOutput.createPipe(),
655656
errorPipe: try processError.createPipe()
656-
).execution.processIdentifier
657+
).execution
657658
case (.none, .none, .some(let errorFd)):
658659
let processInput = NoInput()
659660
let processOutput = DiscardedOutput()
660661
let processError = FileDescriptorOutput(
661662
fileDescriptor: errorFd,
662663
closeAfterSpawningProcess: false
663664
)
664-
return try configuration.spawn(
665+
execution = try configuration.spawn(
665666
withInput: try processInput.createPipe(),
666667
outputPipe: try processOutput.createPipe(),
667668
errorPipe: try processError.createPipe()
668-
).execution.processIdentifier
669+
).execution
669670
case (.none, .some(let outputFd), .none):
670671
let processInput = NoInput()
671672
let processOutput = FileDescriptorOutput(
672673
fileDescriptor: outputFd, closeAfterSpawningProcess: false
673674
)
674675
let processError = DiscardedOutput()
675-
return try configuration.spawn(
676+
execution = try configuration.spawn(
676677
withInput: try processInput.createPipe(),
677678
outputPipe: try processOutput.createPipe(),
678679
errorPipe: try processError.createPipe()
679-
).execution.processIdentifier
680+
).execution
680681
case (.none, .some(let outputFd), .some(let errorFd)):
681682
let processInput = NoInput()
682683
let processOutput = FileDescriptorOutput(
@@ -687,23 +688,23 @@ public func runDetached(
687688
fileDescriptor: errorFd,
688689
closeAfterSpawningProcess: false
689690
)
690-
return try configuration.spawn(
691+
execution = try configuration.spawn(
691692
withInput: try processInput.createPipe(),
692693
outputPipe: try processOutput.createPipe(),
693694
errorPipe: try processError.createPipe()
694-
).execution.processIdentifier
695+
).execution
695696
case (.some(let inputFd), .none, .none):
696697
let processInput = FileDescriptorInput(
697698
fileDescriptor: inputFd,
698699
closeAfterSpawningProcess: false
699700
)
700701
let processOutput = DiscardedOutput()
701702
let processError = DiscardedOutput()
702-
return try configuration.spawn(
703+
execution = try configuration.spawn(
703704
withInput: try processInput.createPipe(),
704705
outputPipe: try processOutput.createPipe(),
705706
errorPipe: try processError.createPipe()
706-
).execution.processIdentifier
707+
).execution
707708
case (.some(let inputFd), .none, .some(let errorFd)):
708709
let processInput = FileDescriptorInput(
709710
fileDescriptor: inputFd, closeAfterSpawningProcess: false
@@ -713,11 +714,11 @@ public func runDetached(
713714
fileDescriptor: errorFd,
714715
closeAfterSpawningProcess: false
715716
)
716-
return try configuration.spawn(
717+
execution = try configuration.spawn(
717718
withInput: try processInput.createPipe(),
718719
outputPipe: try processOutput.createPipe(),
719720
errorPipe: try processError.createPipe()
720-
).execution.processIdentifier
721+
).execution
721722
case (.some(let inputFd), .some(let outputFd), .none):
722723
let processInput = FileDescriptorInput(
723724
fileDescriptor: inputFd,
@@ -728,11 +729,11 @@ public func runDetached(
728729
closeAfterSpawningProcess: false
729730
)
730731
let processError = DiscardedOutput()
731-
return try configuration.spawn(
732+
execution = try configuration.spawn(
732733
withInput: try processInput.createPipe(),
733734
outputPipe: try processOutput.createPipe(),
734735
errorPipe: try processError.createPipe()
735-
).execution.processIdentifier
736+
).execution
736737
case (.some(let inputFd), .some(let outputFd), .some(let errorFd)):
737738
let processInput = FileDescriptorInput(
738739
fileDescriptor: inputFd,
@@ -746,11 +747,13 @@ public func runDetached(
746747
fileDescriptor: errorFd,
747748
closeAfterSpawningProcess: false
748749
)
749-
return try configuration.spawn(
750+
execution = try configuration.spawn(
750751
withInput: try processInput.createPipe(),
751752
outputPipe: try processOutput.createPipe(),
752753
errorPipe: try processError.createPipe()
753-
).execution.processIdentifier
754+
).execution
754755
}
756+
execution.release()
757+
return execution.processIdentifier
755758
}
756759

‎Sources/Subprocess/Configuration.swift

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -71,12 +71,11 @@ public struct Configuration: Sendable {
7171
outputPipe: output,
7272
errorPipe: error
7373
)
74-
let pid = spawnResults.execution.processIdentifier
7574

7675
var spawnResultBox: SpawnResult?? = consume spawnResults
7776
var _spawnResult = spawnResultBox!.take()!
7877

79-
let processIdentifier = _spawnResult.execution.processIdentifier
78+
let execution = _spawnResult.execution
8079

8180
let result: Swift.Result<Result, Error>
8281
do {
@@ -89,9 +88,8 @@ public struct Configuration: Sendable {
8988
return try await body(_spawnResult.execution, inputIO, outputIO, errorIO)
9089
} onCleanup: {
9190
// Attempt to terminate the child process
92-
await Execution.runTeardownSequence(
93-
self.platformOptions.teardownSequence,
94-
on: pid
91+
await execution.runTeardownSequence(
92+
self.platformOptions.teardownSequence
9593
)
9694
})
9795
} catch {
@@ -103,7 +101,7 @@ public struct Configuration: Sendable {
103101
// even if `body` throws, and we are not leaving zombie processes in the
104102
// process table which will cause the process termination monitoring thread
105103
// to effectively hang due to the pid never being awaited
106-
let terminationStatus = try await Subprocess.monitorProcessTermination(forProcessWithIdentifier: processIdentifier)
104+
let terminationStatus = try await Subprocess.monitorProcessTermination(forExecution: _spawnResult.execution)
107105

108106
return try ExecutionResult(terminationStatus: terminationStatus, value: result.get())
109107
}

‎Sources/Subprocess/Execution.swift

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,16 @@ public struct Execution: Sendable {
3535
public let processIdentifier: ProcessIdentifier
3636

3737
#if os(Windows)
38+
internal nonisolated(unsafe) let processInformation: PROCESS_INFORMATION
3839
internal let consoleBehavior: PlatformOptions.ConsoleBehavior
3940

4041
init(
4142
processIdentifier: ProcessIdentifier,
43+
processInformation: PROCESS_INFORMATION,
4244
consoleBehavior: PlatformOptions.ConsoleBehavior
4345
) {
4446
self.processIdentifier = processIdentifier
47+
self.processInformation = processInformation
4548
self.consoleBehavior = consoleBehavior
4649
}
4750
#else
@@ -51,6 +54,17 @@ public struct Execution: Sendable {
5154
self.processIdentifier = processIdentifier
5255
}
5356
#endif // os(Windows)
57+
58+
internal func release() {
59+
#if os(Windows)
60+
guard CloseHandle(processInformation.hThread) else {
61+
fatalError("Failed to close thread HANDLE: \(SubprocessError.UnderlyingError(rawValue: GetLastError()))")
62+
}
63+
guard CloseHandle(processInformation.hProcess) else {
64+
fatalError("Failed to close process HANDLE: \(SubprocessError.UnderlyingError(rawValue: GetLastError()))")
65+
}
66+
#endif
67+
}
5468
}
5569

5670
// MARK: - Output Capture

‎Sources/Subprocess/Platforms/Subprocess+Darwin.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -485,18 +485,18 @@ extension String {
485485
// MARK: - Process Monitoring
486486
@Sendable
487487
internal func monitorProcessTermination(
488-
forProcessWithIdentifier pid: ProcessIdentifier
488+
forExecution execution: Execution
489489
) async throws -> TerminationStatus {
490490
return try await withCheckedThrowingContinuation { continuation in
491491
let source = DispatchSource.makeProcessSource(
492-
identifier: pid.value,
492+
identifier: execution.processIdentifier.value,
493493
eventMask: [.exit],
494494
queue: .global()
495495
)
496496
source.setEventHandler {
497497
source.cancel()
498498
var siginfo = siginfo_t()
499-
let rc = waitid(P_PID, id_t(pid.value), &siginfo, WEXITED)
499+
let rc = waitid(P_PID, id_t(execution.processIdentifier.value), &siginfo, WEXITED)
500500
guard rc == 0 else {
501501
continuation.resume(
502502
throwing: SubprocessError(

‎Sources/Subprocess/Platforms/Subprocess+Linux.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,7 @@ extension String {
265265
// MARK: - Process Monitoring
266266
@Sendable
267267
internal func monitorProcessTermination(
268-
forProcessWithIdentifier pid: ProcessIdentifier
268+
forExecution execution: Execution
269269
) async throws -> TerminationStatus {
270270
try await withCheckedThrowingContinuation { continuation in
271271
_childProcessContinuations.withLock { continuations in
@@ -274,7 +274,7 @@ internal func monitorProcessTermination(
274274
// the child process has terminated and manages to acquire the lock before
275275
// we add this continuation to the dictionary, then it will simply loop
276276
// and report the status again.
277-
let oldContinuation = continuations.updateValue(continuation, forKey: pid.value)
277+
let oldContinuation = continuations.updateValue(continuation, forKey: execution.processIdentifier.value)
278278
precondition(oldContinuation == nil)
279279

280280
// Wake up the waiter thread if it is waiting for more child processes.

‎Sources/Subprocess/Platforms/Subprocess+Unix.swift

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -117,18 +117,6 @@ extension Execution {
117117
public func send(
118118
signal: Signal,
119119
toProcessGroup shouldSendToProcessGroup: Bool = false
120-
) throws {
121-
try Self.send(
122-
signal: signal,
123-
to: self.processIdentifier,
124-
toProcessGroup: shouldSendToProcessGroup
125-
)
126-
}
127-
128-
internal static func send(
129-
signal: Signal,
130-
to processIdentifier: ProcessIdentifier,
131-
toProcessGroup shouldSendToProcessGroup: Bool
132120
) throws {
133121
let pid = shouldSendToProcessGroup ? -(processIdentifier.value) : processIdentifier.value
134122
guard kill(pid, signal.rawValue) == 0 else {

0 commit comments

Comments
 (0)