make closures act like MaybeDangling - #160745
Conversation
|
cc @rust-lang/miri Some changes occurred to the CTFE machinery Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
|
r? @clarfonthey rustbot has assigned @clarfonthey. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@rfcbot merge opsem |
|
@RalfJung has proposed to merge this. The next step is review by the rest of the tagged team members: 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. |
This comment has been minimized.
This comment has been minimized.
75383ed to
932a86d
Compare
This comment has been minimized.
This comment has been minimized.
932a86d to
a13be5a
Compare
This comment has been minimized.
This comment has been minimized.
e3c49f3 to
d919fe5
Compare
|
r? compiler This isn't really a libs change. |
|
@rustbot reviewed
But in the face of std getting it wrong1, and of unpin generators hitting another very subtle case of not being logically pinned but still needing to be (logically) wrapped in Furthermore, I strongly suspect that giving closures Footnotes
|
|
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 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 |
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.
The example in the OP actually does get |
|
I am not a good reviewer for this. Gonna guess a better one, please reassign if this is a bad choice: r? @saethlin |
|
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. |
|
@rustbot ready |
|
@WaffleLapkin friendly review ping. :) |
| } | ||
| } | ||
|
|
||
| // Patch result if we are a MaybeDangling-like type. |
There was a problem hiding this comment.
I don't really like this patching, but I don't see an easy way to make this better, so it's fine for now.
There was a problem hiding this comment.
FWIW this is pre-existing, I am just being more explicit in the comments.
903b6d3 to
b74e09e
Compare
|
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. |
b74e09e to
60b5f88
Compare
|
@bors r=WaffleLapkin |
|
📋 This PR cannot be approved because it currently has the following label: |
|
@bors r=WaffleLapkin |
…=WaffleLapkin make closures act like MaybeDangling 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 `Box`es 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 rust-lang#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](rust-lang#159443). 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: ```rust 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 rust-lang#159443
Rollup of 25 pull requests Successful merges: - #159074 ([PAC] FnAbi, llvm.ptrauth.resign and Session API change (2/8)) - #159792 (A more readable debug map for IndexMaps) - #160745 (make closures act like MaybeDangling) - #161895 (std::sys::pal::sgx: fix mismatched alloc/free alignment) - #161940 (Promote `wasm32-wasip3` to a tier 2 target) - #162072 (Add new Tier-3 target: `powerpc64-sony-ps3`) - #162179 (type system const items via direct rhs) - #162277 (Introduce `rustc_middle::middel::resolve`) - #162285 (box: fixup map/try_map deallocate calls) - #162286 (string: don't unwind prematurely) - #162289 (alloc: a bunch of safety comments) - #162292 (Update `askama` version to `0.16.1`) - #160509 (Remove `RegionExt`; move methods to `Region` in `rustc_type_ir`) - #160906 (Suggest usize instead of placeholder type for array length constants) - #160936 (traits: Represent live alias arguments as bitsets) - #161400 (Improve diagnostics for references to closures) - #161656 (Suggest mutable references for FnMut closure arguments) - #161711 (Add more splat fn type tests) - #161786 (Make `tcx.def_id_partial_cmp` public) - #161953 (sanitizers: Implicitly disable mutually exclusive sanitizers) - #162155 (add suggestion for `rustc_allowed_through_unstable_modules` attribute) - #162212 (Implement `Rng` for `Box`) - #162246 (Fix incorrect meta span) - #162266 (std: fix typo) - #162291 (Add regression test from 1.98.1)
…=WaffleLapkin make closures act like MaybeDangling 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 `Box`es 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 rust-lang#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](rust-lang#159443). 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: ```rust 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 rust-lang#159443
…=WaffleLapkin make closures act like MaybeDangling 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 `Box`es 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 rust-lang#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](rust-lang#159443). 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: ```rust 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 rust-lang#159443
Rollup of 27 pull requests Successful merges: - #159074 ([PAC] FnAbi, llvm.ptrauth.resign and Session API change (2/8)) - #159792 (A more readable debug map for IndexMaps) - #160745 (make closures act like MaybeDangling) - #161940 (Promote `wasm32-wasip3` to a tier 2 target) - #162030 (Prevent `--test` to be used in `rustdoc-html` testsuite) - #162072 (Add new Tier-3 target: `powerpc64-sony-ps3`) - #162179 (type system const items via direct rhs) - #162262 (Avoid manually instantiating some binders in error reporting with `-Znext-solver`) - #162277 (Introduce `rustc_middle::middel::resolve`) - #162285 (box: fixup map/try_map deallocate calls) - #162286 (string: don't unwind prematurely) - #162289 (alloc: a bunch of safety comments) - #162290 (abby test DSL: AliasTyOutlivesViaEnv) - #162292 (Update `askama` version to `0.16.1`) - #160509 (Remove `RegionExt`; move methods to `Region` in `rustc_type_ir`) - #160906 (Suggest usize instead of placeholder type for array length constants) - #160936 (traits: Represent live alias arguments as bitsets) - #161400 (Improve diagnostics for references to closures) - #161656 (Suggest mutable references for FnMut closure arguments) - #161711 (Add more splat fn type tests) - #161786 (Make `tcx.def_id_partial_cmp` public) - #161953 (sanitizers: Implicitly disable mutually exclusive sanitizers) - #162155 (add suggestion for `rustc_allowed_through_unstable_modules` attribute) - #162212 (Implement `Rng` for `Box`) - #162246 (Fix incorrect meta span) - #162266 (std: fix typo) - #162291 (Add regression test from 1.98.1)
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:Here's an example of code that no longer has UB under this PR:
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 simplifyManuallyDrop, 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 @WaffleLapkinFixes #159443