Which subsystem is responsible for preventing this concurrency bug?

Development Compiler
1

This data race safety hole was recently (re-)reported, and it's unclear to me where the responsibility for fixing it lies. Here's an illustration of the problem:

@MainActor
final class C {
    var state = 0
}

func send(_ fn: sending @escaping () -> Void) {
    // Can now call the closure (synchronously) from anywhere
    // even though the one we passed was main-actor-isolated
    Task.detached { fn() }
}

@MainActor
func bug(_ c: C) {
    // The function type conversion here is allowed even though it loses
    // isolation information and sends the closure value.
    send { @MainActor in
      c.state += 1
    }

    c.state += 1
}

Sema explicitly permits the conversion which drops the global actor – the original logic to support that was added here. There are a number of factors which control this, and the conversion is rejected in some cases, e.g. when the enclosing context doesn't match the isolation of the closure:

124 | func bug(_ c: C) {
125 |     send { @MainActor in
    |          `- error: converting function value of type '@MainActor @Sendable () -> Void' to '() -> Void' loses global actor 'MainActor'
126 |       c.state += 1
127 |     }

The Sema logic also disallows the conversion if the resulting function type is Sendable. Converting to a non-Sendable function type and attempting to pass that along is tolerated in Sema, which delegates responsibility to region analysis to enforce concurrency safety. e.g. in the original example, this gets through Sema, but is caught during the SIL mandatory passes:

@MainActor
func bug(_ c: C) {
    // Conversion is allowed in Sema
    let fn: () -> Void = { @MainActor in
        c.state += 1
    }
    send(fn) // 🛑
//       |- error: sending 'fn' risks causing data races
//       `- note: main actor-isolated 'fn' is passed as a 'sending' parameter; Uses in callee may race with later main actor-isolated uses
}

As the original example demonstrates, however, region analysis has its own blindspots, since the function value is allowed to be sent when the closure's isolation matches that of its enclosing context (which I think is the same as or related to the issues outlined here).

Which approach seems like the appropriate one to address this?

  1. Update Sema to prevent isolation-dropping conversions when converting values passed to sending parameters (and results?). This is an approach explored here.
  2. Change region analysis so it will actually catch this sort of pattern.
  3. Both?
  4. Something else?
2 Likes
[Pitch] Create official comprehensive concurrency documentation and adopt related enhanced processes
2

The basic suggestion is to enforce a rule that a conversion may erase isolation from a type only if the value's region is tagged with that isolation. (The converted closure's region has to be born MainActor-isolated rather than disconnected.)

But I think it's a lack of a coherent approach that's the responsible subsystem. You've inspired me to finish this document and base its motivation on this situation :D GitHub - gistya/swift-concurrency-reference: Reference spec for Swift Concurrency · GitHub

1 Like
3

I'm not sure if I'm the right person to get involved here, given that I have essentially zero understanding of the internals. But two things stand out to me.

The first is that converting a synchronous, isolated closure to a synchronous, nonisolated sending parameter seems suspect. I hesitate to say "should never be allowed", but I'm struggling to come up with a meaningfully-useful example where this would make sense. So, that seems at least potentially like sema territory?

But the second thing seems more significant. Your last example, which uses the closure local for conversion and then makes then sending call seals the deal for me. That should be semantically equivalent from RBI's perspective? And I think that means this has to be an RBI problem, independent of sema making the situation impossible.

1 Like
4

I agree, and I also hesitate to say that but... I sort of would like to say it because it seems like this conversion may generally be a soundness hole. That, and banning it in Sema seems like it might be a bit easier to reason about, and would catch it earlier in the pipeline.

Yeah, this does seem like it should produce the same outcome regardless of whether there is a local variable introduced. However, region analysis seems rather sensitive to exactly how things are represented in SIL, which unfortunately appears to result in issues like this sometimes. As I mentioned in the linked GH issue, region analysis actually does internally identify the issue in the original example as an problem, but it then suppresses the error for some reason. However, the bug still slips through in the local-variable case if you introduce another closure with the same isolation as the enclosing function:

@MainActor
func bug(_ c: C) {
    let cl: () -> Void = { @MainActor in c.state += 1 }
    let cl2: @MainActor () -> Void = { cl() }
    send(cl2) // ⁉️ allowed, but should not be

    c.state += 1
}

There are clearly some problems in region analysis that should be addressed, but I guess I'm still wondering if a Sema change would be justifiable and sufficient to catch this class of issue regardless of outstanding bugs in RBI.

5

100%. To repeat from that doc I linked, the moment sending shipped, "does this converted value cross a boundary?" became a flow-sensitive question, which puts it in RBI's jurisdiction by the paradigm's own division of labor. Implementing the invariant region tagging check, a conversion may erase isolation from a type only if the value's region is tagged with that isolation is also assertable in the SIL verifier, so future disagreements between Sema and RBI about what got erased fail at compile time as they should.

Now to be clear, we're really talking about a new evolution pitch, not just a single bugfix (this will fix three bugs if I'm right).

6

This seems like too strong a restriction. Perhaps this is true if the function type is synchronous and/or not @isolated(any)?

7

I do not see how it could be possible for a synchronous closure to be both isolated and disconnected. These two things seem inherently incompatible.

Typically, when I'm wrestling with these kinds of things, I try to cook up a valid counterexample. I haven't tried particularly hard, but the only scenario I can think of is if you applied sending but didn't actually end up moving the value out of the current isolation. This is not a good justification, but a better one might exist.

However, I'm feeling more confident now that this conversion is invalid and if it can be detected in sema it should be rejected there.

8

Yeah I think it is important to note that asynchronous functions are safe since they can dynamically hop back to their isolation domain.

9

I checked with the concurrency folks and this is indeed an RBI issue, converting a synchronous @MainActor function to nonisolated is sound as long as it stays within a @MainActor isolated context (which is enforced by virtue of the fact that we don't allow the function to be @Sendable), RBI shouldn't be allowing it to be passed to a sending parameter.

4 Likes
10

I am taking a quick look at this

1 Like