scoped threads: pass closure through MaybeUninit to avoid invalid dangling references

[RalfJung]

The main function defined here looks roughly like this, if it were written as a more explicit stand-alone function:

// 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 #101983
r? @m-ou-se

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[RalfJung]

Hm, this has the side effect that if the closure is dropped before being run, we leak f. That is probably not what we want... bit right now that is hard to avoid. We'd need a small wrapper type around MaybeUninit that drops its contents.

[danielhenrymantilla]

Does the stdlib use scopeguard? Writing such ad-hoc drop glues becomes the breeze it should have always been:

let mb_dangling_f = ::scopeguard::guard(MaybeUninit::new(f), /* drop: */ |f| unsafe {
    drop(MaybeUninit::assume_init(f));
});
let closure = move/*(mb_dangling_f, ..)*/ |…| {
    let f = ScopeGuard::into_inner(mb_dangling_f);
    let f = unsafe {
        MaybeUninit::assume_init(f)
    };

[RalfJung]

No I don't think it does.

[m-ou-se]

@bors r+

[bors]

📌 Commit 78b577c has been approved by m-ou-se

It is now in the queue for this repository.

[coolreader18]

Just looking at this cause it was in twir and I'm confused - doesn't closure.call_once() take ownership of the closure type and drop it? How would closure still be alive/dereferenceable if it was consumed by the call operator?

[RalfJung]

The bug is described in a bit more detail at #101983, does that help?