make closures act like MaybeDangling

[RalfJung]

View all comments

This makes closures (and types like them: coroutines and coroutine closures) act like MaybeDangling. This means that the aliasing model will entirely ignore references and Boxes passed around as closure captures, removing a pretty subtle footgun that has already caused multiple soundness issues:

• The standard library thread spawning logic was unsound because it passed around arbitrary user data in a closure capture. See Scoped threads violate 'dereferenceable for function call' requirement of references #101983 for details. I doubt that this is the only such unsoundness in the ecosystem, this is just very hard to find -- you need to not only run your code in Miri but also pass very specific types through your API to trigger the UB.
• Movable (unpinned) generators can contain mutable references that are reborrowed from other references stored in the same generator. This is currently unsound. The only way this is sound is if the reborrowed-from references are inside MaybeDangling; without this, moving the generator (which retags its contents) invalidates the reborrowed reference. So at least for generators, we have to do this change anyway one way or another.

Here's an example of code that no longer has UB under this PR:

fn invoke(f: impl FnOnce()) {
    f()
}

fn main() {
    let p = Box::leak(Box::new(0i32));
    invoke(move || {
        drop(unsafe { Box::from_raw(p) });
    });
}

Basically, what we are establishing here is that immediately invoking a closure should be (almost) equivalent to just inlining its body. (There is still a caveat here in that if you capture things that violate their validity invariant, the inlined body might not care but immediately invoking the closure will. But at least for all the subtle questions around aliasing, the two will be equivalent under this PR.)

Overall I think the fact that moving a closure / generator will alter its contents (by retagging) is just a bit too subtle. It's already subtle for "normal" types but there at least one can see the type with its fields. For closures, that's all entirely implicit. At the same time, the benefit we get from this at the moment is tiny -- we can only actually tell LLVM about these references if the closure/generator has scalar / scalar-pair representation, which can only happen when it captures at most 2 scalar values.

This PR just implements the semantics without updating any docs. I am not sure where we'd document this, given our general lack of documentation around the aliasing model. Still we should t-opsem FCP this PR to ensure we have team consensus for not retagging or requiring reference dereferenceability inside closoures and closure-like types (and then we can involve lang if/when we start making official promises about this).

On the implementation side, I realized this by introducing the notion of "maybe-dangling-like" types, so that the semantics is not hard-coded specifically to MaybeDangling. This also lets us simplify ManuallyDrop, reducing its field nesting a bit, which should help with some of the query limit issues people encountered when we added the extra field nesting. It also means generators get the desired semantics without increasing their field nesting. Cc @WaffleLapkin

Fixes #159443

[rustbot]

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

[rustbot]

r? @clarfonthey

rustbot has assigned @clarfonthey.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, nia-e

[RalfJung]

@rfcbot merge opsem

[rust-rfcbot]

@RalfJung has proposed to merge this. The next step is review by the rest of the tagged team members:

• @CAD97
• @RalfJung
• @digama0
• @joshlf
• @saethlin

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[clarfonthey]

r? compiler

This isn't really a libs change.

[CAD97]

@rustbot reviewed

The unpin generators case could theoretically be solved by making generators specifically !UnsafeUnpin. Even if fix the opsem by giving all closures MaybeDangling semantics, this could arguably still be a correct (removal of) application of the autotrait.

FnOnce closures can't be used after they're consumed, so the "morally correct" fix is that usage sites that could logically move from impl FnOnce should be wrapping it in ManuallyDrop so the compiler can know that this is happening.

But in the face of std getting it wrong¹, and of unpin generators hitting another very subtle case of not being logically pinned but still needing to be (logically) wrapped in UnsafePinned, I agree that giving all closures (fixed) ManuallyDrop (i.e. MaybeDangling) semantics

Furthermore, I strongly suspect that giving closures MaybeDangling semantics won't have an observable impact on performance. (Though this assumption should be tested.) Namely, because uses of closures will either be monomorphic and able to re-infer the stronger properties that hold; or polymorphic, need to be conservative, and the opportunity cost largely dominated by the indirect call anyway.

Footnotes

1. And notably, IIRC, the version that got it wrong and was fixed with MaybeUninit didn't use ManuallyDrop despite it being stable, which is intended to have semantics that would also fix the soundness pitfall. (Although it didn't yet at the time.) ↩

[CAD97]

I almost forgot the strongest rationale: there's the really subtle case where you might think you're capturing a pointer but actually capture a (potentially mutable) reference instead because of how place capture rules work.

fn invoke(f: impl FnOnce()) {
    f()
}

fn main() {
    let p = Box::leak(Box::new(0i32));
    invoke(|| {
        drop(unsafe { Box::from_raw(&raw mut *p) });
    });
}

Note: I could've sworn that having ptr: *mut i32 and using it as &mut *ptr would capture *ptr by-mut instead of capturing ptr by-move. That footgun was extremely dangerous, so I'm glad we fixed it already and I just forgot that we did.

The remaining instance of this style of footgun is much less potent than the one I recall existing previously, but combined with the other evidence that retagging closures' captures when retagging the impl FnOnce is fraught with subtle footguns that the most experienced Rust programmers did miss originally, the practical decision is to merge this change.

[RalfJung]

The unpin generators case could theoretically be solved by making generators specifically !UnsafeUnpin.

That would not help. The problem is the retag that happens when we move the generator. At that point it is not behind any kind of reference so no amount of UnsafeCell/UnsafeUnpin makes a difference.

Furthermore, I strongly suspect that giving closures MaybeDangling semantics won't have an observable impact on performance.

The example in the OP actually does get noalias currently which we'd lose.
But I doubt that's a common enough pattern to matter.

[nnethercote]

I am not a good reviewer for this. Gonna guess a better one, please reassign if this is a bad choice:

r? @saethlin

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[WaffleLapkin]

r? me

[ais523]

#161026 (which I discovered this morning) is another aliasing violation of the same general nature. I'm assuming that this pull request will fix that too?

(EDIT: probably not, I discovered a different way to exploit it that involves uninitialized memory rather than just aliasing / dangling references, and MaybeDangling doesn't help with that.)

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[RalfJung]

#161026 (which I discovered this morning) is another aliasing violation of the same general nature. I'm assuming that this pull request will fix that too?

At least the first, original example in that issue is indeed fixed by this PR. I added a test for that.

[ais523]

In the original MCVE, this comment attaches to the c = b; line, although in this version it's (misleadingly) above the else block. I suspect this was an autoformatter accident.

View changes since the review

[RalfJung]

Ah yeah rustfmt likes to re-associate comments. Has been reported years ago but they are so understaffed not even critical (comment-semantics-altering) bugs get fixed unfortunately :/

Thanks for catching this.

[RalfJung]

@WaffleLapkin you removed "waiting on review", but this did not get a review yet, so I think it is waiting for review as well as waiting for FCP?

[WaffleLapkin]

@RalfJung that is right. I removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. because usually we only have one S-* label and S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. "takes priority" in my opinion.

[RalfJung]

We often have PRs waiting on more than one thing. And when it seems overwhelmingly likely that FCP will pass I see no reason to sequentialize reviewing and the FCP wait. I often get reviews for such PRs, which means they can be r+ immediately when FCP finishes.

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[RalfJung]

@rustbot ready