Fix soundness issue in scoped threads.

[m-ou-se]

This was discovered in #94559 (comment)

The scope() function returns when all threads are finished, but I accidentally considered a thread 'finished' before dropping their panic payload or ignored return value.

So if a thread returned (or panics with) something that in its Drop implementation still uses borrowed stuff, it goes wrong.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=2a1f19ac4676cdabe43e24e536ff9358

[rust-highfive]

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

[nagisa]

Reading just this comment in isolation, I feel like it is dangerous to assume that all unwinding implementations would abort in absence of landing pads on the stack. I suspect there won't be any documented mandate for this sort of behavior anywhere.

[m-ou-se]

Maybe, but then that's a larger problem that Rust's regular threads (through std::thread::spawn) also have (and had, since like forever).

[m-ou-se]

It's easy to fix it for both cases by adding a catch_unwind and abort here though. I'll do that in a second commit.

[m-ou-se]

Added a second commit that fixes that issue for both scoped and regular threads. It now explicitly calls rtabort!() when dropping the thread result panics.

[m-ou-se]

(cc rust-lang/lang-team#97, which would make this a non-issue.)

[nagisa]

I wonder if there's any way to write a regression test here. I think comparing drop order between data captured by the scope and the data that refers to this captured data shouldn't he too difficult. Something like this perhaps?

struct MustDropFirst;
struct MustDropSecond<'scope>(&'scope MustDropFirst);

impl Drop for MustDropFirst { /* checks drop order between the two types */ }
impl Drop for MustDropSecond { /* checks drop order between the two types */ }

fn main() {
    let data = MustDropFirst;
    let r = &data;
    scope(|s| {
        s.spawn(move || {
            MustDropSecond(r);
        });
    });
}

The implementation LGTM either way, though.

[m-ou-se]

I wonder if there's any way to write a regression test here.

It's a bit tricky, because the bug doesn't mean it gets ordered in the opposite direction, but that there's no guarantee how it gets ordered. In practice, without sleeping or a significant amount of work, the thread most likely finishes dropping the result before the main thread wakes up, making the bug invisible.

It's somewhat easy to test this with sleeping, but I'm not sure it's a great idea to have a unit test that sleeps.

[danielhenrymantilla]

Good catch!

[m-ou-se]

@nagisa I've added a test. (And verified it fails without the changes in this PR.)

[joshtriplett]

@bors r+

[bors]

📌 Commit b97d875 has been approved by joshtriplett

[bors]

⌛ Testing commit b97d875 with merge a301059c353dee85b3a3e52923c8094a9d7a02d1...

[bors]

💔 Test failed - checks-actions

[nagisa]

@bors retry #94779

[rust-log-analyzer]

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

.......... (60/61)
.          (61/61)


/checkout/src/test/rustdoc-gui/mobile.goml mobile... FAILED
[ERROR] (line 27) Error: Evaluation failed: assert didn't fail: for command `compare-elements-position-near-false: ("#preferred-light-theme .setting-name", "#preferred-light-theme .choice", {"y": 16})`
Build completed unsuccessfully in 0:00:16

[m-ou-se]

@bors retry