Process: correct a subtle runLoopSource lifetime issue
#3125
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
|
@swift-ci please test |
|
Converting to draft - I think that I hit the failure even with the check? |
|
Okay, caught the thing under a debugger with the change - it reduces the frequency, but is more indicative of a data race - we now hit it as a UaF in We can see in Clearly |
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.
compnerd
changed the title
runLoopSource lifetime issue
|
@swift-ci please test |
|
The failures seem unrelated - docc timeouts |
|
@swift-ci please test |
|
Alternative, which should fix both code paths: |
|
I think that the re-ordering is actually correct and desirable (though retaining is also something we should do in addition to the re-ordering). The re-ordering ensures that we do not touch the monitor runloop source after we indicate the process termination. We can simply hoist that above the flagging. Refactoring to share the path seems reasonable as well, though, I'd love to see a more invasive refactoring to share most of the path for the monitor. |
|
@swift-ci please test |
|
@swift-ci please test |
pcbeard
approved these changes
Jan 17, 2022
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 |
1 similar comment
|
@swift-ci please test |
|
@swift-ci please test Linux platform |
millenomi
approved these changes
Jan 18, 2022
|
LGTM. Merging if green. Please create a PR against release/5.6 as well? |
|
#3128 for 5.6 |
|
Please test with following PRs: @swift-ci please test Linux platform |
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.
When a process is run with
run, a monitoring thread is setup by meansof a socketpair. This socket is used to create a runloop source, which
will monitor the process for termination.
waitUntilExitwill monitorthe
isRunningivar which is set by the source to indicate the processtermination 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 asthe refcount is now 0. This would result in the
runloopSourcepassedto
CFRunLoopSourceInvalidatebeingniland breaking the contract ofthe call or a UaF if the value is
nil'ed during the execution. Thesestates have both been observed in practice. Reordering the
isRunningivar assignment to occur after the invalidation ensures that the value
remains usable until we no longer reference it.