Great stack overflow error messages

[gilescope]

As a coder
Given I have a fn main() { main() }
Then I expect the following output:

Exception in thread "main" java.lang.StackOverflowError
	at X.main(X.java:3)
	at X.main(X.java:3)
	at X.main(X.java:3)
	at X.main(X.java:3)

Obviously this is an example of how Java handles stack overflows, but you get the idea.

There didn't seem to be a tracking issue for this, so here is one.
(Some discussion here: https://users.rust-lang.org/t/how-to-diagnose-a-stack-overflow-issues-cause/17320/9 )

I've had a little look and I naively think we need to do something like this in sys_common/util.rs

    #[allow(dead_code)] // stack overflow detection not enabled on all platforms    
    pub unsafe fn report_overflow() {
        dumb_print(format_args!("\nthread '{}' has overflowed its stack\n",
                            thread::current().name().unwrap_or("<unknown>")));

        #[cfg(feature = "backtrace")]
        {
            let log_backtrace = backtrace::log_enabled();

            use sync::atomic::{AtomicBool, Ordering};

            static FIRST_PANIC: AtomicBool = AtomicBool::new(true);

            if let Some(format) = log_backtrace {
                if let Ok(mut stderr) = Stderr::new() {
                    let _ = backtrace::print(&mut stderr, format);
                }
            } else if FIRST_PANIC.compare_and_swap(true, false, Ordering::SeqCst) {
                dumb_print(format_args!("note: Run with `RUST_BACKTRACE=1` for a backtrace."));
            }
        }
    }

Quite possibly one can ditch the first panic checks. I'm sure there's lots of concerns here, e.g. have we got enough stack headroom to report without going pop.

[Techcable]

I often wish that this was the case, given that most managed languages have this feature and preventing segfaults is a major goal of rust.

Unfortunately, this could break panic safety and introduce undefined behavior.
It is very common for unsafe code to assume that certain functions can never panic based on both documented or undocumented guarantees. For example ptr::copy_nonovoerlapping ptr.offset and ptr::write are often assumed to never panic.

vec.reserve(1);
unsafe {
    let i = vec.len();
    let p = vec.as_mut_ptr();
    // Right now `vec[i]` is uninitialized and should never be observed by user code
    vec.set_len(vec.len() + 1);
    /*
     * Keeping `vec[i]` uninitlaized is safe as long as untrusted user code
     * can never read the uninitialized memory because it's never called.
     * Since we never directly invoke untrusted code,
     * our only risk is from panicking causing user destructors to run.  
     * Because `ptr::copy_nonoverlapping` and `ptr.offset` never panic we
     * should be safe and users will never see our uninitialized memory.
     */
    ptr::copy_nonoverlapping(existingPointer, p.offset(i), 1));
}

If stack overflows could trigger a panic then both ptr::copy_nonoverlapping and ptr.offset are turned into potentially panicking functions. The stack could unwind and user code could observe the uninitialized memory when running their destructors.
This could trigger a large amount of potential unsafety across the stdlib and entire ecosystem. Furthermore, it makes it nigh on impossible to write correct unsafe code in the future because crucial functions like ptr::copy_nonoverlapping and ptr::write can now panic.

First of all, we could unwind the stack without calling the destructors, which should avoid most cases of untrusted code seeing unsafe states.
The downside of this approach is that it leaks memory and may be unsound.

The other alternative is a system to mark functions as #[nopanic].
Then we could statically know which functions are 'allowed' to panic without impacting memory safety. If a #[nopanic] function is on the stack during stack overflow, it'd be forbidden to panic and we would trigger an abort to avoid undefined behavior. Otherwise, the function is allowed to panic and the runtime would prefer that when dealing with a stack overflow.
This would force unsafe code to statically mark all their assumptions about which functions can and can not panic in order to retain memory safety. However, this would still be a major barrier to adoption and would probably require an RFC before it could be relied on.

[gilescope]

Thank you for the pointers. I'm working on the assumption that any thread that stack overflows is going to trigger an abort in the process. I.e. we're not planning on having processes that can survive a stack overflow in one thread. Please disabuse me of this notion if not correct!

If an abort is the only outcome of a stack overflow, then leaking memory is probably ok as the OS is just about to tear down the process.

I suspect full 'unwinding' is unnecessary - can we walk the stack's return addresses given we're really just after a good stack trace to show how the overflow occurred. For the first cut a list of addresses would show how many frames were in the loop which is more info than none.
(Let - me rename the issue to be more focused on the outcome rather than the implementation)

[jonas-schievink]

So what you want is just a backtrace, not actual unwinding. That would probably be useful for debugging, yeah.

[gilescope]

Ok so instead of using libunwind if we use glibc's backtrace in this case we should be able to have both safety and debug info on platforms where glibc backtrace is supported. I get that we might not get such a good stacktrace in release mode versus debug mode due to optimisations, but anything's better than nothing.

I couldn't see unwinding in glibc's backtrace source:
https://github.com/lattera/glibc/blob/master/debug/backtrace.c

[sfackler]

Our current backtrace implementation doesn't unwind either.

The main complexity in doing this is making sure we have enough stack space to create the backtrace even though we've already run out of stack space.

[jonas-schievink]

On Unix, this is running in signal context, which uses the alternative signal stack, which is usually a few KB in size (smallest size on supported Linux platforms seems to be 8K). It's possible to reconfigure this size using sigaltstack.

EDIT:

On Windows, Rust uses Vectored Exception Handling to register a handler that detects and reports a stack overflow. The stack size guarantee can be set with SetThreadStackGuarantee, and this is in fact already done here (it's set to 20 KB to make space for printing and formatting).

Now whether it's even possible to use libbacktrace/libunwind or StackWalk64 from the handler while the stack is in this overflowed state yet remains to be seen, but capturing a stacktrace on segfaults seems possible at least.

[sfackler]

If we'd be running the backtrace in the signal handler, we'd need to switch back to libbacktrace's mmap allocator rather than malloc/free which I seem to remember having some pretty severe perf issues. Not sure if the allocator can be configured at runtime.

[jonas-schievink]

Relevant: #51408 (removes libbacktrace)

[gilescope]

Thanks @jonas-schievink, that's great instead of relying on backtrace_symbols or backtrace_symbols_fd. We'd still need to walk the stack frames for a pure rust solution - maybe I'm missing something but I couldn't see anything currently built / being built that tries to walk the stack in pure rust.

I tested the above code on OSX and it worked a treat. But it depends on libunwind and looks to me like libunwind does indeed unwind when creating the backtrace so as @Techcable mentioned it won't be panic safe. I'm going to try and see if I can get glibc's backtrace working (no luck yet).

(Just thinking outside the box, one could spawn a separate thread to output the backtrace and pass it a reference to the overflowed thread's stack and join on the thread. It sounds like the major OSes give you enough stack during a stack overflow that we don't need to go there.)

[sfackler]

But it depends on libunwind and looks to me like libunwind does indeed unwind when creating the backtrace so as @Techcable mentioned it won't be panic safe.

Using libunwind is not the same thing as unwinding the stack. Taking a backtrace with libbacktrace or glibc backtrace or backtrace_symbols are all the same in that they do not change the state of the stack.

[retep998]

It's more accurate to say that libunwind is merely being used to walk the stack to build up a stacktrace (hence why the windows equivalent is called StackWalk)

[Techcable]

The java virtual machine (and others) typically reserve a separate 'yellow zone' and 'red zone' in the stack space which is used for the JVM as reserved space for handling stack overflows [1].
When normal code runs out of room it will try and read/write in the yellow zone which will trigger a SEGFAULT that the JVM will catch.
In order to give the user a nice pretty StackOverflowError the JVM will remove the protection of the yellow zone to give enough stack space to handle the error and unwind the stack. In case that's not enough room, the functions will run into the red zone and the JVM will issue an internal error and give a crash dump.

While this approach is often taken by fast managed languages like V8, C#, and the JVM, reserving an extra zone of stack space is not necessarily an appropriate choice for Rust. While it has exactly zero overhead in the common case, this approach requires a few KB of reserved stack space for each thread and extra system calls on thread creation. This is kind of uncharted territory since rust is both a hardcore system programming language like C/C++ but also has much of the safety and convenience of a managed language.
If we can't figure out how to efficiently print backtraces in a signal handling context, I believe a similar approach may be worthwhile (at the minimum for debug builds) in order to provide nice pretty error messages to make it easier to track down where the heck we had a stack overflow.

[Techcable]

@sfackler libbacktrace's allocator can't be configured at runtime.However, the mmap allocator is enabled by default when mmap is present so it should already be enabled on all the major unix systems. The presense of the mmap allocator and signal safety can be tested at compile time by the C define flag BACKTRACE_USES_MALLOC which is 0 on Debian (signal safe). Since we already use the system libbacktrace library, we're already stuck with their defaults so this shouldn't make backtrace performance any worse 👿

Also, we may not even need to use libbacktrace at all, since libunwind provides a signal-safe unw_get_proc_name and unw_get_proc_info although we'd need to do some trickery to get line number info.