Skip to content

[Concurrency runtime] Don't read from the actor after transitioning state #66016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 22, 2023
Merged

[Concurrency runtime] Don't read from the actor after transitioning state #66016

merged 1 commit into from
May 22, 2023

Conversation

DougGregor
Copy link
Member

@DougGregor DougGregor commented May 19, 2023 •
edited
Loading

  • Explanation: In the concurrency runtime, calls to the tracing hooks for tracking actor state transitions assumed that they could read from the actor's memory. While unlocking an actor, there is an extremely narrow window in which the actor has transitioned into an idle state and then been deallocated by another thread before the tracing hook gets invoked, causing a use-after-free in in the concurrency runtime. Predetermine the information that was being read from the actor at pointers where we know that the actor is still alive, and pass it through to these tracing functions, to eliminate the use-after-free.
  • Scope: Narrow; moves a load of actor state from one central place (that might be invalid) to a few places where it is guaranteed to be valid.
  • Risk: Low; there's no semantic change here, just moving loads around to safer places in the concurrency runtime.
  • Reviewers: @ktoso, @rjmccall
  • Issue: rdar://108497870
  • Original pull request: [Concurrency runtime] Don't read from the actor after transitioning state #66008

@DougGregor
[Concurrency runtime] Don't read from the actor after transitioning s…
a57f316 
…tate

Once we have transitioned the actor into a new state, we report the
state change as a trace event so it can be noted by tools (e.g.,
Instruments). However, the act of transitioning to a new state can mean
that there is an opportunity for another thread to deallocate the
actor. This means that the tracing call cannot depend on dereferencing
the actor pointer.

A refactoring a few months ago to move the bit that indicates when a
distributed actor is remote from inside the atomic actor state out to a
separate field (because it's constant for a given actor instance),
which introduced a dereference of the actor instance in forming the
tracing call. This introduced a narrow window in which a race
condition could occur: the actor transitions to an idle state, and is
then deallocate before the trace event for the actor transition occurs,
leading to a use-after-free.

Fetch this bit of information earlier in the process, before any state
changes and when we know the actor is still allocated, and pass it
through to the tracing code.

Fixes rdar://108497870.

(cherry picked from commit b14e47f)
@DougGregor DougGregor requested a review from a team as a code owner May 19, 2023 04:25
@DougGregor
Copy link
Member Author

@swift-ci please test

@ktoso
Copy link
Contributor

ktoso commented May 19, 2023

Unrelated failure

Failed Tests (1):
  Swift(macosx-x86_64) :: Macros/macro_plugin_server.swift

Lemme restart macos again

@ktoso
Copy link
Contributor

ktoso commented May 19, 2023

@swift-ci please test macOS

@ktoso
Copy link
Contributor

ktoso commented May 19, 2023

@swift-ci please test macOS platform

@DougGregor
Copy link
Member Author

@swift-ci please test macOS

@DougGregor DougGregor merged commit ea5520d into swiftlang:release/5.9-20230510 May 22, 2023
@DougGregor DougGregor deleted the actor-state-tracing-race-5.9-20230510 branch May 22, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktoso ktoso ktoso approved these changes

@hborla hborla hborla approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DougGregor @ktoso @hborla