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.

[bjorn3]

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.

It is not UB. The reason we have this handler at all AFAIK is to avoid people thinking they managed to violate memory safety even though a stack overflow is guaranteed to result in an abort through SIGSEGV on every platform where we or the OS sets up a guard page and stack probing is used. (currently only x86/x86_64, but ideally every arch) The guard page doesn't get removed when dropping TLS data.

[ridiculousfish]

To clarify there are potentially two guard pages at play here:

1. One that the OS creates, which is not unmapped when dropping TLS
2. One that the Rust runtime creates (edit: I previously made a mistake here, I linked to the munmap for the sigaltstack. I am unclear where Rust's mmap gets cleaned up).

Whether this second page is created is platform dependent.

[bjorn3]

Didn't think about the unmapping of the rust created guard page. Looks like it is dropped right between returning from the thread main function and dropping TLS:

rust/library/std/src/sys/unix/thread.rs

Lines 104 to 109 in ad23942

	// Next, set up our stack overflow handler which may get triggered if we run
	// out of stack.
	let _handler = stack_overflow::Handler::new();
	// Finally, let's run some code.
	Box::from_raw(main as *mut Box<dyn FnOnce()>)();
	}

[RalfJung]

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).

AFAIK this is just a soundness bug for some targets and environments (like when main is not Rust). Stack overflow can mean we start colliding with the heap and that's clearly unsound. It's a hard-to-fix soundness bug though. Not sure if it is explicitly tracked anywhere.

Didn't think about the unmapping of the rust created guard page. Looks like it is dropped right between returning from the thread main function and dropping TLS:

So that does sound like a soundness issue then, if the guard page no longer exists when TLS dtors run?

[joboet]

So that does sound like a soundness issue then, if the guard page no longer exists when TLS dtors run?

It's not, because the guard page is provided by the system and available during TLS destruction. What's not available is our signal stack, which we unregister when the thread main function returns. Therefore, the signal handler will be run on the overflowing stack (more specifically: on the guard page), resulting in an immediate, system-caused second SIGSEGV.

[RalfJung]

It's not, because the guard page is provided by the system

There seems to be contradicting information here as above @bjorn3 said that the guard page is unmapped. That was the Rust guard page, which I presume is not the same thing as the system guard page, but if there is guaranteed to be a system guard page then why do we even have a Rust guard page?

[joboet]

There is no Rust guard page, except for some platforms where we protect the main thread (but that one we never reset).

We do need a stack for the signal handler though, and that stack we free at thread exit, otherwise we'd leak quite some memory.