[Sema]: Fix data race safety hole with mutable statics within actors #78652

jamieQ · 2025-01-15T04:51:38Z

Issue

the existing logic in checkGlobalIsolation ignored global variables nested within actor types when diagnosing global isolation during typechecking. this left open a data race safety hole, as described here.

Description

removes the actor bypass logic from checkGlobalIsolation
updates the existing tests
reformat & add commentary to the checkGlobalIsolation implementation (separate commit)

i'd recommend using the 'hide whitespace' option if reviewing the full diff via GitHub as it makes it somewhat easier to parse.

resolves #78435

jamieQ · 2025-01-15T04:54:22Z

lib/Sema/TypeCheckConcurrency.cpp

-    auto *classDecl = var->getDeclContext()->getSelfClassDecl();
-    const bool isActorType = classDecl && classDecl->isAnyActor();
-    if (var->isGlobalStorage() && !isActorType) {


the primary logical change is the removal of this isActorType condition. i'm curious if this seems like the appropriate solution to this issue to those more familiar with this code.

I agree that there is no reason to check whether the enclosing type is an actor type in this code. If a variable is static, it doesn't matter whether or not it's inside an actor - as you've noted, actor instance isolation can't apply here.

jamieQ · 2025-01-15T05:02:27Z

lib/Sema/TypeCheckConcurrency.cpp

-      if (auto *originalVar = var->getOriginalWrappedProperty()) {
-        diagVar = originalVar;
-      }
+  if (isolation.isGlobalActor() || isolation.isNonisolatedUnsafe())


minor change here to use the isNonisolatedUnsafe() accessor vs what was previously being done.

tangent: if someone cares to enlighten me as to whether these two formulations are actually the same i'd be curious to learn more about exactly why/how that is. the first case seems like a class is being compared to a 'Kind' which i don't entirely understand.

I believe the reason the comparison between ActorIsolation and ActorIsolation::Kind works is because there's an operator () on ActorIsolation that returns its kind:

operator Kind() const { return getKind(); }

This effectively enables implicit conversions from ActorIsolation to ActorIsoaltion::Kind; I believe operator () was implicitly called on ActorIsolation in the call to == / !=, so it was really just a comparison between two ActorIsolation::Kinds.

jamieQ · 2025-01-15T12:39:19Z

test/Concurrency/global_variables.swift

@@ -8,7 +8,7 @@

 @globalActor
 actor TestGlobalActor {
-  static var shared = TestGlobalActor()


this code is now diagnosed as invalid since shared is global, mutable, non-isolated state. presumably this means this change could be a source-break – what is the stance on that when it comes to addressing bugs?

It's okay to fix this as an error in Swift 6 mode - we can adjust if we find this causes wide spread source incompatibility amongst Swift 6 mode adopters through testing. These diagnostics are already warnings in strict concurrency checking / using the GlobalConcurrency upcoming feature prior to migrating to the Swift 6 language mode.

hborla

Thank you!

hborla · 2025-02-14T18:38:56Z

@swift-ci please smoke test

jamieQ · 2025-02-15T16:29:12Z

hmm... seems my refactoring of some of the early exit conditions to occur before the getActorIsolation() call may have broken one of the other Concurrency tests (sorry, i thought i ran this stuff locally against that test dir and things passed, but maybe i missed something). i will try to address by preserving the prior execution order.

Update the concurrency typechecking logic to remove a check that allowed mutable static variable declarations nested within an Actor type to be ignored when diagnosing mutable non-Sendable state.

jamieQ · 2025-02-15T17:00:57Z

@hborla i pushed a fix (and rebased) for the failed CI test. would you mind kicking off another build when you have a moment please?

hborla · 2025-02-15T20:02:22Z

@swift-ci please smoke test

hborla · 2025-02-15T20:03:43Z

hmm... seems my refactoring of some of the early exit conditions to occur before the getActorIsolation() call may have broken one of the other Concurrency tests

Huh, I'd love to investigate this to figure out why (separately from your fix here). I thought your refactoring to add the early exits was much easier to follow!

jamieQ · 2025-02-15T21:31:36Z

Huh, I'd love to investigate this to figure out why (separately from your fix here). I thought your refactoring to add the early exits was much easier to follow!

i'll leave some notes based on the brief time i spent looking into it today. the refactoring commit that adjusted the early exit conditions caused the global_actor_inference.swift test to fail with this error:

/swift-project/swift/test/Concurrency/global_actor_inference.swift:502:42: error: unexpected note produced: property declared here
  @SimplePropertyWrapper nonisolated var value 
                                         ^
/swift-project/swift/test/Concurrency/global_actor_inference.swift:507:9: error: unexpected error produced: main actor-isolated property 'value' can not be referenced from a nonisolated context
    _ = value
        ^

this seemed to be due to my original change which moved the call to compute actor isolation to only conditionally occur if there was a valid source loc & the global concurrency feature was enabled (it originally occurred unconditionally like this). it seems there may be something load-bearing about the downstream ActorIsolationRequest that occurs from the getActorIsolation() here?

jamieQ · 2025-02-16T16:38:27Z

CI failures seemed unrelated to this change and might be resolved now that this merged? #79415

hborla · 2025-02-16T22:54:40Z

@swift-ci please smoke test

To fix a concurrency error that was surfaced by swiftlang/swift#78652.

hborla · 2025-02-18T17:48:04Z

swiftlang/sourcekit-lsp#1985

@swift-ci please smoke test

hborla · 2025-02-18T17:48:14Z

@swift-ci please test source compatibility

hborla · 2025-02-20T04:19:52Z

@swift-ci please smoke test Linux

hborla · 2025-02-20T15:11:32Z

@swift-ci please smoke test Linux

hborla · 2025-02-20T20:05:15Z

@jamieQ Thank you for the contribution! Please feel free to ping me if you decide to pick up more in the concurrency area and have questions or need guidance 😄

jamieQ · 2025-02-20T22:08:55Z

@hborla thank you for helping get this through CI – much appreciated!

jamieQ commented Jan 15, 2025

View reviewed changes

jamieQ marked this pull request as ready for review January 15, 2025 05:03

jamieQ requested review from ktoso, hborla, slavapestov and xedin as code owners January 15, 2025 05:03

jamieQ commented Jan 15, 2025

View reviewed changes

hborla approved these changes Feb 14, 2025

View reviewed changes

jamieQ added 2 commits February 15, 2025 10:29

[Sema]: Fix data race safety hole with mutable statics within actors

96ee7a0

Update the concurrency typechecking logic to remove a check that allowed mutable static variable declarations nested within an Actor type to be ignored when diagnosing mutable non-Sendable state.

[gardening]: refactor checkGlobalIsolation function & add comments

f1a1998

jamieQ force-pushed the static-actor-safety-hole branch from fa36959 to f1a1998 Compare February 15, 2025 16:39

ahoppen added a commit to ahoppen/sourcekit-lsp that referenced this pull request Feb 18, 2025

Make LogHandlerActor.shared a let

94d7a22

To fix a concurrency error that was surfaced by swiftlang/swift#78652.

ahoppen mentioned this pull request Feb 18, 2025

Make LogHandlerActor.shared a let swiftlang/sourcekit-lsp#1985

Merged

hborla merged commit be035a6 into swiftlang:main Feb 20, 2025
5 checks passed

jamieQ deleted the static-actor-safety-hole branch February 20, 2025 22:07

hborla mentioned this pull request Jun 20, 2025

Inconsistent compiler behavior with mutable static members in classes and actors #76886

Closed

[Sema]: Fix data race safety hole with mutable statics within actors #78652

[Sema]: Fix data race safety hole with mutable statics within actors #78652

Uh oh!

Conversation

jamieQ commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description

Uh oh!

jamieQ Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

hborla Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

jamieQ Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

hborla Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamieQ Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

hborla Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

hborla left a comment

Choose a reason for hiding this comment

Uh oh!

hborla commented Feb 14, 2025

Uh oh!

jamieQ commented Feb 15, 2025

Uh oh!

jamieQ commented Feb 15, 2025

Uh oh!

hborla commented Feb 15, 2025

Uh oh!

hborla commented Feb 15, 2025

Uh oh!

jamieQ commented Feb 15, 2025

Uh oh!

jamieQ commented Feb 16, 2025

Uh oh!

hborla commented Feb 16, 2025

Uh oh!

hborla commented Feb 18, 2025

Uh oh!

hborla commented Feb 18, 2025

Uh oh!

hborla commented Feb 20, 2025

Uh oh!

hborla commented Feb 20, 2025

Uh oh!

Uh oh!

hborla commented Feb 20, 2025

Uh oh!

jamieQ commented Feb 20, 2025

Uh oh!

Uh oh!

jamieQ commented Jan 15, 2025 •

edited

Loading

hborla Feb 14, 2025 •

edited

Loading