Store the Windows process HANDLE in Execution to avoid pid reuse races #93
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakepetroules
added a commit
that referenced
this pull request
Jun 24, 2025
…ier and make the HANDLE public Currently runDetached returns a ProcessIdentifier, which on Unix is a pid and on Windows is a numeric process identifier. Similar to #92, there is a Windows-specific race condition here. On Unix the pid will be valid until someone waitid's it. But on Windows the ProcessIdentifier may already be invalid by the time runDetached returns. This patch builds on #93 (where the HANDLE is stored in the Execution instead of being closed immediately), and now provides that HANDLE as part of the public Execution API. This allows callers to properly free the handle using CloseHandle, as well as not having to reverse engineer the handle from the numeric pid. #93 by itself would solve the race condition but introduce a resource leak. The API now also better matches the Unix behavior since it is the caller's responsibility to waitid the pid to clean up the resources, which is semantically similar to calling CloseHandle on Windows. Closes #94
jakepetroules
added a commit
that referenced
this pull request
Jun 24, 2025
…ier and make the HANDLE public Currently runDetached returns a ProcessIdentifier, which on Unix is a pid and on Windows is a numeric process identifier. Similar to #92, there is a Windows-specific race condition here. On Unix the pid will be valid until someone waitid's it. But on Windows the ProcessIdentifier may already be invalid by the time runDetached returns. This patch builds on #93 (where the HANDLE is stored in the Execution instead of being closed immediately), and now provides that HANDLE as part of the public Execution API. This allows callers to properly free the handle using CloseHandle, as well as not having to reverse engineer the handle from the numeric pid. #93 by itself would solve the race condition but introduce a resource leak. The API now also better matches the Unix behavior since it is the caller's responsibility to waitid the pid to clean up the resources, which is semantically similar to calling CloseHandle on Windows. Closes #94
jakepetroules
added a commit
that referenced
this pull request
Jun 24, 2025
…ier and make the HANDLE public Currently runDetached returns a ProcessIdentifier, which on Unix is a pid and on Windows is a numeric process identifier. Similar to #92, there is a Windows-specific race condition here. On Unix the pid will be valid until someone waitid's it. But on Windows the ProcessIdentifier may already be invalid by the time runDetached returns. This patch builds on #93 (where the HANDLE is stored in the Execution instead of being closed immediately), and now provides that HANDLE as part of the public Execution API. This allows callers to properly free the handle using CloseHandle, as well as not having to reverse engineer the handle from the numeric pid. #93 by itself would solve the race condition but introduce a resource leak. The API now also better matches the Unix behavior since it is the caller's responsibility to waitid the pid to clean up the resources, which is semantically similar to calling CloseHandle on Windows. Closes #94
jakepetroules
force-pushed
the
eng/PR-always-monitor
branch
from
b66271c to
59caa66
Compare
jakepetroules
added a commit
that referenced
this pull request
Jun 25, 2025
…ier and make the HANDLE public Currently runDetached returns a ProcessIdentifier, which on Unix is a pid and on Windows is a numeric process identifier. Similar to #92, there is a Windows-specific race condition here. On Unix the pid will be valid until someone waitid's it. But on Windows the ProcessIdentifier may already be invalid by the time runDetached returns. This patch builds on #93 (where the HANDLE is stored in the Execution instead of being closed immediately), and now provides that HANDLE as part of the public Execution API. This allows callers to properly free the handle using CloseHandle, as well as not having to reverse engineer the handle from the numeric pid. #93 by itself would solve the race condition but introduce a resource leak. The API now also better matches the Unix behavior since it is the caller's responsibility to waitid the pid to clean up the resources, which is semantically similar to calling CloseHandle on Windows. Closes #94
|
@compnerd Any thoughts on this or the associated GitHub issue? |
compnerd
reviewed
Jun 27, 2025
Yep, that's what it's doing. |
jakepetroules
added a commit
that referenced
this pull request
Jun 27, 2025
…ier and make the HANDLE public Currently runDetached returns a ProcessIdentifier, which on Unix is a pid and on Windows is a numeric process identifier. Similar to #92, there is a Windows-specific race condition here. On Unix the pid will be valid until someone waitid's it. But on Windows the ProcessIdentifier may already be invalid by the time runDetached returns. This patch builds on #93 (where the HANDLE is stored in the Execution instead of being closed immediately), and now provides that HANDLE as part of the public Execution API. This allows callers to properly free the handle using CloseHandle, as well as not having to reverse engineer the handle from the numeric pid. #93 by itself would solve the race condition but introduce a resource leak. The API now also better matches the Unix behavior since it is the caller's responsibility to waitid the pid to clean up the resources, which is semantically similar to calling CloseHandle on Windows. Closes #94
jakepetroules
added a commit
that referenced
this pull request
Jun 27, 2025
…ier and make the HANDLE public Currently runDetached returns a ProcessIdentifier, which on Unix is a pid and on Windows is a numeric process identifier. Similar to #92, there is a Windows-specific race condition here. On Unix the pid will be valid until someone waitid's it. But on Windows the ProcessIdentifier may already be invalid by the time runDetached returns. This patch builds on #93 (where the HANDLE is stored in the Execution instead of being closed immediately), and now provides that HANDLE as part of the public Execution API. This allows callers to properly free the handle using CloseHandle, as well as not having to reverse engineer the handle from the numeric pid. #93 by itself would solve the race condition but introduce a resource leak. The API now also better matches the Unix behavior since it is the caller's responsibility to waitid the pid to clean up the resources, which is semantically similar to calling CloseHandle on Windows. Closes #94
jakepetroules
force-pushed
the
eng/PR-windows-pid-reuse-eng/PR-always-monitor
branch
from
7f1597d to
6b7b547
Compare
jakepetroules
added a commit
that referenced
this pull request
Jun 27, 2025
…ier and make the HANDLE public Currently runDetached returns a ProcessIdentifier, which on Unix is a pid and on Windows is a numeric process identifier. Similar to #92, there is a Windows-specific race condition here. On Unix the pid will be valid until someone waitid's it. But on Windows the ProcessIdentifier may already be invalid by the time runDetached returns. This patch builds on #93 (where the HANDLE is stored in the Execution instead of being closed immediately), and now provides that HANDLE as part of the public Execution API. This allows callers to properly free the handle using CloseHandle, as well as not having to reverse engineer the handle from the numeric pid. #93 by itself would solve the race condition but introduce a resource leak. The API now also better matches the Unix behavior since it is the caller's responsibility to waitid the pid to clean up the resources, which is semantically similar to calling CloseHandle on Windows. Closes #94
iCharlesHu
reviewed
Jun 27, 2025
Sources/Subprocess/Execution.swift
Outdated
| @@ -35,13 +35,16 @@ public struct Execution: Sendable { | |||
| public let processIdentifier: ProcessIdentifier | |||
|
|
|||
| #if os(Windows) | |||
| internal nonisolated(unsafe) let processHandle: HANDLE | |||
There was a problem hiding this comment.
I agree we should save the process HANDLE but I don't think we should put it on Execution. Why not leave it in ProcessIdentifier since it serves that purpose? Therefore you don't have to make other changes.
There was a problem hiding this comment.
I looked at that, but ProcessIdentifier is Codable and Hashable, so HANDLE shouldn't go in there because it's a pointer (which we should not be serializing to disk)
There was a problem hiding this comment.
Also, since the HANDLE is only valid for the lifetime of the process and becomes invalid once CloseHandle is called, it seemed a better for to be on Execution.
Sources/Subprocess/Execution.swift
Outdated
| @@ -35,13 +35,16 @@ public struct Execution: Sendable { | |||
| public let processIdentifier: ProcessIdentifier | |||
|
|
|||
| #if os(Windows) | |||
| internal nonisolated(unsafe) let processHandle: HANDLE | |||
There was a problem hiding this comment.
I understand you used nonisolated(unsafe) here because HANDLE isn't Sendable, and we want Execution and ProcessIdentifier to be. @compnerd do you know what the best approach is? Should we use nonisolated(unsafe) or mark the struct @unchecked Sendable (or other approaches)?
There was a problem hiding this comment.
HANDLE is an opaque pointer, so it's effectively Sendable in practice in the logical context of how it's used.
There was a problem hiding this comment.
Also, you meant @compnerd not @ compend
jakepetroules
added a commit
that referenced
this pull request
Jun 27, 2025
…ier and make the HANDLE public Currently runDetached returns a ProcessIdentifier, which on Unix is a pid and on Windows is a numeric process identifier. Similar to #92, there is a Windows-specific race condition here. On Unix the pid will be valid until someone waitid's it. But on Windows the ProcessIdentifier may already be invalid by the time runDetached returns. This patch builds on #93 (where the HANDLE is stored in the Execution instead of being closed immediately), and now provides that HANDLE as part of the public Execution API. This allows callers to properly free the handle using CloseHandle, as well as not having to reverse engineer the handle from the numeric pid. #93 by itself would solve the race condition but introduce a resource leak. The API now also better matches the Unix behavior since it is the caller's responsibility to waitid the pid to clean up the resources, which is semantically similar to calling CloseHandle on Windows. Closes #94
owenv
reviewed
Jun 28, 2025
jakepetroules
force-pushed
the
eng/PR-windows-pid-reuse-eng/PR-always-monitor
branch
2 times, most recently
from
0ddc201 to
2b8c265
Compare
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
jakepetroules
force-pushed
the
eng/PR-windows-pid-reuse-eng/PR-always-monitor
branch
from
2b8c265 to
6422350
Compare
owenv
approved these changes
Jun 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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