Skip to content

Commit 0ddc201

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 0ddc201

File tree

6 files changed

+82
-115
lines changed

6 files changed

+82
-115
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: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ public struct Configuration: Sendable {
103103
// even if `body` throws, and we are not leaving zombie processes in the
104104
// process table which will cause the process termination monitoring thread
105105
// to effectively hang due to the pid never being awaited
106-
let terminationStatus = try await Subprocess.monitorProcessTermination(forProcessWithIdentifier: processIdentifier)
106+
let terminationStatus = try await Subprocess.monitorProcessTermination(forExecution: _spawnResult.execution)
107107

108108
return try ExecutionResult(terminationStatus: terminationStatus, value: result.get())
109109
}

‎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+Windows.swift

Lines changed: 43 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -116,54 +116,33 @@ extension Configuration {
116116
}
117117
}
118118
}
119-
// We don't need the handle objects, so close it right away
120-
guard CloseHandle(processInfo.hThread) else {
121-
let windowsError = GetLastError()
122-
try self.safelyCloseMultiple(
123-
inputRead: inputReadFileDescriptor,
124-
inputWrite: inputWriteFileDescriptor,
125-
outputRead: outputReadFileDescriptor,
126-
outputWrite: outputWriteFileDescriptor,
127-
errorRead: errorReadFileDescriptor,
128-
errorWrite: errorWriteFileDescriptor
129-
)
130-
throw SubprocessError(
131-
code: .init(.spawnFailed),
132-
underlyingError: .init(rawValue: windowsError)
133-
)
134-
}
135-
guard CloseHandle(processInfo.hProcess) else {
136-
let windowsError = GetLastError()
137-
try self.safelyCloseMultiple(
138-
inputRead: inputReadFileDescriptor,
139-
inputWrite: inputWriteFileDescriptor,
140-
outputRead: outputReadFileDescriptor,
141-
outputWrite: outputWriteFileDescriptor,
142-
errorRead: errorReadFileDescriptor,
143-
errorWrite: errorWriteFileDescriptor
144-
)
145-
throw SubprocessError(
146-
code: .init(.spawnFailed),
147-
underlyingError: .init(rawValue: windowsError)
148-
)
149-
}
150-
151-
try self.safelyCloseMultiple(
152-
inputRead: inputReadFileDescriptor,
153-
inputWrite: nil,
154-
outputRead: nil,
155-
outputWrite: outputWriteFileDescriptor,
156-
errorRead: nil,
157-
errorWrite: errorWriteFileDescriptor
158-
)
159119

160120
let pid = ProcessIdentifier(
161121
value: processInfo.dwProcessId
162122
)
163123
let execution = Execution(
164124
processIdentifier: pid,
125+
processInformation: processInfo,
165126
consoleBehavior: self.platformOptions.consoleBehavior
166127
)
128+
129+
do {
130+
// After spawn finishes, close all child side fds
131+
try self.safelyCloseMultiple(
132+
inputRead: inputReadFileDescriptor,
133+
inputWrite: nil,
134+
outputRead: nil,
135+
outputWrite: outputWriteFileDescriptor,
136+
errorRead: nil,
137+
errorWrite: errorWriteFileDescriptor
138+
)
139+
} catch {
140+
// If spawn() throws, monitorProcessTermination or runDetached
141+
// won't have an opportunity to call release, so do it here to avoid leaking the handles.
142+
execution.release()
143+
throw error
144+
}
145+
167146
return SpawnResult(
168147
execution: execution,
169148
inputWriteEnd: inputWriteFileDescriptor?.createPlatformDiskIO(),
@@ -259,55 +238,33 @@ extension Configuration {
259238
}
260239
}
261240
}
262-
// We don't need the handle objects, so close it right away
263-
guard CloseHandle(processInfo.hThread) else {
264-
let windowsError = GetLastError()
265-
try self.safelyCloseMultiple(
266-
inputRead: inputReadFileDescriptor,
267-
inputWrite: inputWriteFileDescriptor,
268-
outputRead: outputReadFileDescriptor,
269-
outputWrite: outputWriteFileDescriptor,
270-
errorRead: errorReadFileDescriptor,
271-
errorWrite: errorWriteFileDescriptor
272-
)
273-
throw SubprocessError(
274-
code: .init(.spawnFailed),
275-
underlyingError: .init(rawValue: windowsError)
276-
)
277-
}
278-
guard CloseHandle(processInfo.hProcess) else {
279-
let windowsError = GetLastError()
280-
try self.safelyCloseMultiple(
281-
inputRead: inputReadFileDescriptor,
282-
inputWrite: inputWriteFileDescriptor,
283-
outputRead: outputReadFileDescriptor,
284-
outputWrite: outputWriteFileDescriptor,
285-
errorRead: errorReadFileDescriptor,
286-
errorWrite: errorWriteFileDescriptor
287-
)
288-
throw SubprocessError(
289-
code: .init(.spawnFailed),
290-
underlyingError: .init(rawValue: windowsError)
291-
)
292-
}
293-
294-
// After spawn finishes, close all child side fds
295-
try self.safelyCloseMultiple(
296-
inputRead: inputReadFileDescriptor,
297-
inputWrite: nil,
298-
outputRead: nil,
299-
outputWrite: outputWriteFileDescriptor,
300-
errorRead: nil,
301-
errorWrite: errorWriteFileDescriptor
302-
)
303241

304242
let pid = ProcessIdentifier(
305243
value: processInfo.dwProcessId
306244
)
307245
let execution = Execution(
308246
processIdentifier: pid,
247+
processInformation: processInfo,
309248
consoleBehavior: self.platformOptions.consoleBehavior
310249
)
250+
251+
do {
252+
// After spawn finishes, close all child side fds
253+
try self.safelyCloseMultiple(
254+
inputRead: inputReadFileDescriptor,
255+
inputWrite: nil,
256+
outputRead: nil,
257+
outputWrite: outputWriteFileDescriptor,
258+
errorRead: nil,
259+
errorWrite: errorWriteFileDescriptor
260+
)
261+
} catch {
262+
// If spawn() throws, monitorProcessTermination or runDetached
263+
// won't have an opportunity to call release, so do it here to avoid leaking the handles.
264+
execution.release()
265+
throw error
266+
}
267+
311268
return SpawnResult(
312269
execution: execution,
313270
inputWriteEnd: inputWriteFileDescriptor?.createPlatformDiskIO(),
@@ -464,7 +421,7 @@ extension PlatformOptions: CustomStringConvertible, CustomDebugStringConvertible
464421
// MARK: - Process Monitoring
465422
@Sendable
466423
internal func monitorProcessTermination(
467-
forProcessWithIdentifier pid: ProcessIdentifier
424+
forExecution execution: Execution
468425
) async throws -> TerminationStatus {
469426
// Once the continuation resumes, it will need to unregister the wait, so
470427
// yield the wait handle back to the calling scope.
@@ -473,15 +430,8 @@ internal func monitorProcessTermination(
473430
if let waitHandle {
474431
_ = UnregisterWait(waitHandle)
475432
}
476-
}
477-
guard
478-
let processHandle = OpenProcess(
479-
DWORD(PROCESS_QUERY_INFORMATION | SYNCHRONIZE),
480-
false,
481-
pid.value
482-
)
483-
else {
484-
return .exited(1)
433+
434+
execution.release()
485435
}
486436

487437
try? await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Void, any Error>) in
@@ -500,7 +450,7 @@ internal func monitorProcessTermination(
500450
guard
501451
RegisterWaitForSingleObject(
502452
&waitHandle,
503-
processHandle,
453+
execution.processHandle,
504454
callback,
505455
context,
506456
INFINITE,
@@ -518,7 +468,7 @@ internal func monitorProcessTermination(
518468
}
519469

520470
var status: DWORD = 0
521-
guard GetExitCodeProcess(processHandle, &status) else {
471+
guard GetExitCodeProcess(execution.processHandle, &status) else {
522472
// The child process terminated but we couldn't get its status back.
523473
// Assume generic failure.
524474
return .exited(1)

0 commit comments

Comments
 (0)