[5.6] Process: correct a subtle runLoopSource lifetime issue #3128
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
When a process is run with `run`, a monitoring thread is setup by means of a socketpair. This socket is used to create a runloop source, which will monitor the process for termination. `waitUntilExit` will monitor the `isRunning` ivar which is set by the source to indicate the process termination having been observed. We however would previously mark the process as terminated before invalidating the runloop source to prevent any new callouts on the source. However, the operation is multithreaded the operation is not guaranteed to be serialized and the runloop source may be `nil`'ed prior or during the invalidation which also releases as the refcount is now 0. This would result in the `runloopSource` passed to `CFRunLoopSourceInvalidate` being `nil` and breaking the contract of the call or a UaF if the value is `nil`'ed during the execution. These states have both been observed in practice. Reordering the `isRunning` ivar assignment to occur after the invalidation ensures that the value remains usable until we no longer reference it.
This fixes a data race between setting `process.isRunning = false` and then calling `CFRunLoopSourceInvalidate(process.runLoopSource)`. Process.waitUntilExit() may wake up and clear .runLoopSource which results in a fairly common crash on Windows. Put this logic in a common method to reduce drift between the Darwin / Linux and Windows code paths. Co-authored-by: Saleem Abdulrasool <[email protected]>
|
@swift-ci please test |
|
Please test with following PRs: @swift-ci please test Linux platform |
|
cc @parkera OK for merging? This fixes a timing issue in detecting whether a Process-managed task has finished running. |
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.
No description provided.