Stack overflow not caught in Drop for TLS data

[ridiculousfish]

Rust attempts to catch stack overflow by installing a guard page at the end of the valid stack range. This guard page is stored in TLS and is torn down when a thread exits (including the main thread).

However other thread local data may run drop after this guard page is torn down. Stack overflows occurring in these drop implementations are not detected by Rust. (It may be backstopped by the OS, but this is system dependent.)

To reproduce:

use std::thread_local;
struct First;
struct Second;

impl Drop for First {
    fn drop(&mut self) {
        // First is materialized in TLS in main().
        // Second is materialized in First's dtor, thereby registering a
        // dtor for Second.
        // This dtor is guaranteed to run after the Thread data (including
        // its guard page) has been unmapped.
        SECOND.with(|_s| {} );
    }
}

impl Drop for Second {
    fn drop(&mut self) {
        // Trigger a stack overflow.
        recurse(0);
    }
}

thread_local! {
    static FIRST: First = First;
}

thread_local! {
    static SECOND: Second = Second;
}

// Triggers a stack overflow.
fn recurse(count: usize) {
    if count < usize::MAX {
        recurse(count + 1);
    }
    println!("{} bottles of beer on the wall", count);
}

fn main() {
    FIRST.with(|_f| {});
}

This causes a SIGILL on macOS and a SIGSEGV on Linux. In both cases I confirmed that Rust's stack overflow signal handler is not run.

Reproduced on rust stable and master branch.

[workingjubilee]

cc @m-ou-se may be of interest to you as you clean up the thread local impl.

[bstrie]

@ridiculousfish Did you believe that there is potential for UB from safe code here? If you can explain the circumstances where UB can arise here, I'll tag this with I-unsound (which is the tag used for the ability to invoke UB from safe code).

[ridiculousfish]

@bstrie There is no unsafe code in the repro case. I don't know if a stack overflow which is not caught by Rust's handler is considered UB, but imagine it should be, else there would be no reason for the handler. So (conservatively) yes.

[workingjubilee]

I don't think it's ever actually been definitively answered as to whether the stack protector is considered a soundness constraint or a quality of implementation detail.

[thomcc]

In general it's target-specific if we implement it (and even depends if Rust is in control of main) so I think it's considered quality of implementation (if for no other reason than pragmatism).

That said, I think this is worth fixing. On most targets (I don't think all, but it's been a while) we have enough control over the order dtors are run in already (since we run them manually most of the time) that we should be able to ensure the guard teardown happens last.

[thomcc]

I can look at this this weekend.

[joboet]

Duplicate of #109785. In short, the problem is that

1. the guard page range is stored in the same TLS variable as the current Thread, so it is destroyed before the other TLS destructors are run
2. the signal handler stack is deallocated just before the thread exits, so when the signal handler runs, it accesses the NULL pointer, resulting in the crash.

The first problem is quite easily handled by putting the guard page location in a second TLS variable, but the second problem is more complicated. I tried to resolve these by eagerly running the destructors in the thread itself (see #109858) but that appears to interact badly with the platform libc.

[workingjubilee]

Despite being a duplicate, I am going to close the other issue instead, because this one has more useful data.