Scoped threads violate 'dereferenceable for function call' requirement of references #101983
Labels
Comments
RalfJung
added
T-lang
rustbot
added
the
I-prioritize
4 tasks
|
I believe #98517 included a fix for this. |
|
Doesn't look like it -- the user-provided closure seems to be handled exactly the same as currently? |
|
Ah, I read too quickly and misunderstood, apologies. It solves a different potential unsoundness (one more similar to the Arc drop issue). |
|
Yeah, it looks like an alternative fix for the |
apiraino
removed
the
I-prioritize
|
While the RFC for a proper fix is cooking, I am proposing a preliminary fix in #102589. |
thomcc
pushed a commit
to tcdi/postgrestd
that referenced
this issue
Feb 10, 2023
scoped threads: pass closure through MaybeUninit to avoid invalid dangling references
The `main` function defined here looks roughly like this, if it were written as a more explicit stand-alone function:
```rust
// Not showing all the `'lifetime` tracking, the point is that
// this closure might live shorter than `thread`.
fn thread(control: ..., closure: impl FnOnce() + 'lifetime) {
closure();
control.signal_done();
// A lot of time can pass here.
}
```
Note that `thread` continues to run even after `signal_done`! Now consider what happens if the `closure` captures a reference of lifetime `'lifetime`:
- The type of `closure` is a struct (the implicit unnameable closure type) with a `&'lifetime mut T` field. References passed to a function are marked with `dereferenceable`, which is LLVM speak for *this reference will remain live for the entire duration of this function*.
- The closure runs, `signal_done` runs. Then -- potentially -- this thread gets scheduled away and the main thread runs, seeing the signal and returning to the user. Now `'lifetime` ends and the memory the reference points to might be deallocated.
- Now we have UB! The reference that as passed to `thread` with the promise of remaining live for the entire duration of the function, actually got deallocated while the function still runs. Oops.
Long-term I think we should be able to use `ManuallyDrop` to fix this without `unsafe`, or maybe a new `MaybeDangling` type. I am working on an RFC for that. But in the mean time it'd be nice to fix this so that Miri with `-Zmiri-retag-fields` (which is needed for "full enforcement" of all the LLVM flags we generate) stops erroring on scoped threads.
Fixes rust-lang/rust#101983
r? `@m-ou-se`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Scopes threads as currently implemented are potentially unsound due to passing a reference to a function where the memory the reference points to is potentially deallocated while the function still runs.
Specifically, this main function here takes as implicit argument its closure environment, which contains
f. That environment might just be a reference to some memory that is allocated in the caller stack frame. However themainfunction can keep running aftertheir_packetgot dropped (implicitly at the end ofmain). Then the scope might end and the memory might be deallocated all whilemainis still running. If the environment is just a reference, it ends up being a newtype and we will (AFAIK) add thedereferenceableattribute, meaning deallocation while the function runs is actually LLVM UB.Miri found the error in this doc test, where the
a.push(4)invalidates the&areference that was passed to the forked-off threads. (adoesn't actually get deallocated, it just has a unique reference created to it, but deallocation is also a possibility).To fix this properly, I think we need a language extension: we want a way to store references in a type without having alias guarantees for that reference. In terms of Stacked Borrows this means Stacked Borrows will stop recursing at that type during retags. In terms of the LLVM IR we generate it means we must not add
noaliasordereferenceableattributes for references inside that type. We don't have such a type currently but I think it makes sense to makeManuallyDropthat type (or even if we add a new type for this,ManuallyDropshould use it). Also see this Zulip discussion.The text was updated successfully, but these errors were encountered: