Panic while formatting panic message + always_abort leads to stack overflow

[RalfJung]

Example and initial analysis by @correabuscar:

#![feature(panic_always_abort)]

use std::fmt::{Display, self};

struct MyStruct;

impl Display for MyStruct {
    fn fmt(&self, _: &mut fmt::Formatter<'_>) -> fmt::Result {
        panic!("this is bad: {}", MyStruct);
    }
}

fn main() {
    std::panic::always_abort();
    println!("{}", MyStruct);
}

The reason for this is that if both "panic while processing panic" and "panic after always_abort" apply, then we use the AlwaysAbort code path which does format the panic message.

This could be fixed by moving this check further down below the in_panic_hook check:

rust/library/std/src/panicking.rs

Lines 393 to 395 in 9f25a04

	if global_count & ALWAYS_ABORT_FLAG != 0 {
	return Some(MustAbort::AlwaysAbort);
	}

But I don't know if this has other adverse side-effects. Can we even access thread-local variables after fork (i.e., when always-abort is set)?
Cc @Amanieu

[correabuscar]

Can we even access thread-local variables after fork?

we can't due to // Accessing LOCAL_PANIC_COUNT in a child created bylibc::fork would lead to a memory allocation. from here: https://github.com/rust-lang/rust/blob/9f25a04498fbe30dcf6a9c764953704c333a0137/library/std/src/panicking.rs#L372C4-L373C19

(Presumably thread locals cannot be done without allocating in rust, but can in C ?)

Unless we do thread local in a .c library which is NOT guaranteed to not allocate on all platforms (but on linux and windows shouldn't allocate)
I've an example of this being done here, reproduced below (but I doubt this will even be considered even for just the few platforms that is guaranteed to not alloc):

Cargo.toml

[package]
name = "c_thread_local_in_rust"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]

[build-dependencies]
#cc = { version="1", path = "/tmp/cc-rs"  } #used this because it doesn't do this warn: warning: c_thread_local_in_rust@0.1.0: Compiler version doesn't include clang or GCC: "cc" "--version"
cc = "1.0.90" #otherwise, using this is preferred

build.rs

fn main() {
    // Compile the C code into a shared library using cc crate
    cc::Build::new()
        .file("tl.c")
        .shared_flag(true)
        .compile("tl");
}

tl.c

// example.c
//#include <stdio.h>

// Define a thread-local variable
__thread int thread_local_var;

// Function to access the thread-local variable
int *get_thread_local_var() {
    return &thread_local_var;
}

// Function to set the value of the thread-local variable
void set_thread_local_var(int value) {
    thread_local_var = value;
}

/*
   In C, thread-local storage (TLS) can be implemented in different ways, and whether it involves heap allocation or new allocations depends on the specific implementation and the platform.

However, the most common and efficient way to implement thread-local storage in C is by using platform-specific mechanisms such as compiler extensions (`__thread` specifier) or standard library functions (`pthread_key_create`, `TlsAlloc`, etc.), which typically allocate thread-local storage directly from the system or using thread-specific data structures managed by the runtime environment.

For example, in the case of the `__thread` specifier in GCC/Clang, thread-local variables are often allocated directly within the thread's control block or using thread-specific registers, avoiding heap allocation entirely.

Similarly, with `pthread_key_create` or Windows-specific functions like `TlsAlloc`, the thread-local storage is managed by the operating system or the runtime environment, typically without involving heap allocation.

That said, it's crucial to understand that the exact implementation details can vary across platforms, compilers, and runtime environments. Therefore, while thread-local storage in C is often implemented efficiently without heap allocation, it's essential to consult the documentation and consider the specific context and requirements of your application to ensure optimal performance and behavior.

*/

main.rs

//mod ffi;
use std::thread;
use std::time::Duration;

//so, unlike thread_local!() in rust which supposedly somehow does memory allocation(s) on heap
//upon first access of the var, this in .c file thread local does not, but cannot guarantee it
//doesn't on all other platforms

fn main() {
    println!("Hello, world!");
    let value = access_thread_local_var();
    println!("Thread-local value: {}", value);
    set_thread_local_var2(20);
    let value = access_thread_local_var();
    println!("Thread-local value: {}", value);

    // Spawn a new thread
    let handle = thread::spawn(|| {
        // This closure represents the code that will run in the new thread
        for i in 1..=5 {
            let value = access_thread_local_var();
            println!("Hello from the spawned thread! Message {} ltvar={}", i,value);
            thread::sleep(Duration::from_millis(500)); // Sleep for 500 milliseconds
            set_thread_local_var2(10*i);
        }
    });

    // Main thread continues executing concurrently with the spawned thread
    for i in 1..=3 {
        let value = access_thread_local_var();
        println!("Hello from the main thread! Message {} ltvar={}", i,value);
        thread::sleep(Duration::from_millis(1000)); // Sleep for 1 second
        set_thread_local_var2(i);
    }

    // Wait for the spawned thread to finish
    handle.join().unwrap();

    println!("Main thread exiting.");
}
//
// Link against the C shared library
//#[link(name = "tl")] //looks like this isn't needed!
//extern {}

// Rust code (lib.rs)
extern {
    // Define an external function to access the thread-local variable
    fn get_thread_local_var() -> *mut i32;
    fn set_thread_local_var(value: i32);
}

// Function to access the thread-local variable from Rust
pub fn access_thread_local_var() -> i32 {
    unsafe {
        // Call the external function to get a pointer to the thread-local variable
        let ptr = get_thread_local_var();
        // Dereference the pointer to access the value of the thread-local variable
        *ptr
    }
}

// Function to set the value of the thread-local variable from Rust
pub fn set_thread_local_var2(value: i32) {
    unsafe {
        // Call the external function to set the value of the thread-local variable
        set_thread_local_var(value);
    }
}

output:

     Running `target/debug/c_thread_local_in_rust`
Hello, world!
Thread-local value: 0
Thread-local value: 20
Hello from the main thread! Message 1 ltvar=20
Hello from the spawned thread! Message 1 ltvar=0
Hello from the spawned thread! Message 2 ltvar=10
Hello from the main thread! Message 2 ltvar=1
Hello from the spawned thread! Message 3 ltvar=20
Hello from the spawned thread! Message 4 ltvar=30
Hello from the main thread! Message 3 ltvar=2
Hello from the spawned thread! Message 5 ltvar=40
Main thread exiting.

As an aside, if I do figure out how to get thread local storage on rust without allocating, then I might be able to fix a bug in libtest whereby tests ran in-process as threads (except for the case of --test-threads=1 which somehow works) will cause harness to exit because the test is using exit() or test infinitely panics and other variants, which would all be catch_unwind-able after the fix.

[RalfJung]

(Presumably thread locals cannot be done without allocating in rust, but can in C ?)

This certainly used to be true, but nowadays LOCAL_PANIC_COUNT is a const TLS variable with no destructor. So that comment may be outdated. But I am not sure whether const TLS variable with no destructor are alloc-free on all platforms. @m-ou-se or @joboet might know.

EDIT: Looking at the code, fast_local does not need allocations but os_local does. So it depends on cfg(target_thread_local). At least on one of the Windows GNU MSVC targets we don't have target_thread_local so we need allocations.

Not sure how else we could detect reentrancy in the panic handler though. Maybe we can make it depend on cfg(target_thread_local) so that at least on those platforms where we don't need to allocate, we can avoid the stack overflow?

[RalfJung]

As an aside, if I do figure out how to get thread local storage on rust without allocating, then I might be able to fix a bug in libtest whereby tests ran in-process as threads (except for the case of --test-threads=1 which somehow works) will cause harness to exit because the test is using exit() or test infinitely panics and other variants, which would all be catch_unwind-able after the fix.

I don't know what you mean, but if there is a bug here, please report it as a new issue. It's not related to this issue, so please let's avoid getting off-topic.

[correabuscar]

As an aside, if I do figure out how to get thread local storage on rust without allocating, then I might be able to fix a bug in libtest whereby tests ran in-process as threads (except for the case of --test-threads=1 which somehow works) will cause harness to exit because the test is using exit() or test infinitely panics and other variants, which would all be catch_unwind-able after the fix.

I don't know what you mean, but if there is a bug here, please report it as a new issue. It's not related to this issue, so please let's avoid getting off-topic.

You're right. (though I didn't yet scope the whole thing about that issue, I'm only aware of 3 ways that test would break out of the test harness that aren't fixed yet, so I'm postponing reporting it until I do understand it and can formulate it properly - also, perfectionism sux)

But what I should've said, that is relevant to this issue(or so I think), is the following:
that if we have thread locals that do not alloc, in rust (or particularly at that point in rust_panic_with_hook function), then,
we could keep track of and back off of each path on each recursive panic call.
Say for example we initially try to show the 'message' (mark this path as being in progress, eg. 1 bit in some U64 maybe) then if that panics, we get back to the same place, see that the path was taken(ie. we're already in it, but panic-ed inside it, thus recursed back to it) then we take a lesser path next which is to not show 'message', so show location only, and if even that path recurses (ie. panics) then an even lesser path of not showing location but just a normal hardcoded message and even this path fails somehow and so on ...

parens

(that being said, 'message' can still allocate since it's user-controlled and they may choose to do so, it breaks the 'fork' case of needing to avoid any allocations which is why we've used panic::always_abort in the first place - but this is another issue)

however this is just the panic::always_abort() case branch of paths, this could possibly be done for the whole tree of paths inside the rust_panic_with_hook function. If we had thread locals that don't alloc on first access(maybe we do, I just don't know it yet - you mentioned fast_local which I know nothing of, atm).
Either way I plan to do it(or something like it) locally(local patch) for my own rustc usage(as I assume upstream won't be interested if it's hacky - eg. uses __thread int a_thread_local_var; from a .c lib), as soon as I find a way to get a thread local without allocating (to ensure the fork case is covered).

[RalfJung]

that if we have thread locals that do not alloc, in rust (or particularly at that point in rust_panic_with_hook function), then,
we could keep track of and back off of each path on each recursive panic call.

I'm not sure it's worth the effort to distinguish whether it's the panic message formatting or the panic hook that caused the recursive panic. But either way that possible improvement is orthogonal to this issue as well.

[correabuscar]

that if we have thread locals that do not alloc, in rust (or particularly at that point in rust_panic_with_hook function), then,
we could keep track of and back off of each path on each recursive panic call.

I'm not sure it's worth the effort to distinguish whether it's the panic message formatting or the panic hook that caused the recursive panic. But either way that possible improvement is orthogonal to this issue as well.

Well no, it would prevent any infinite recursion because eventually at 3rd or whatever panic recursion it would either print a harcoded message, or if even that one panic-ed, then it wouldn't print anything just reach the abort call.

That was the point, progressively back off of paths that we've been on, until the only path left is one that doesn't panic or the last one which is abort.

EDIT: that being said, maybe I'm misunderstanding something and that's why the above makes sense to me but is in fact wrong (i'm attempting to figure that out and update this after)
EDIT2: confirmed works, here's an equivalent(but wrongly implemented way) sample:

output:

    Running `target/debug/panic_in_display`
must_abort=Some(AlwaysAbort)
in panic_count::MustAbort::AlwaysAbort
panicked at /home/user/sandbox/rust/05_sandbox/tests/panic_in_display/src/main.rs:21:9:
oh2 no, 'must_abort=Some(AlwaysAbort)
in panic_count::MustAbort::AlwaysAbort
panicked after panic::always_abort(), aborting. (and recursion prevented) loc='Location { file: "/home/user/sandbox/rust/05_sandbox/tests/panic_in_display/src/main.rs", line: 14, col: 9 }'
about to abort_internal()
./gmy: line 5: 143389 Aborted                 (core dumped) cargo run
exit code: 134

for this code:

#![feature(panic_always_abort)]
//
//src: https://github.com/rust-lang/rust/issues/97181
//this double panic no longer shows stacktrace due to https://github.com/rust-lang/rust/pull/110975

use std::fmt::{Display, self};

struct MyStruct;
struct MyStruct2;
impl Display for MyStruct2 {
    fn fmt(&self, _: &mut fmt::Formatter<'_>) -> fmt::Result {
        //todo!()
        let instance = MyStruct;
        panic!("oh1 no, '{}' was unexpected", instance);
    }
}

impl Display for MyStruct {
    fn fmt(&self, _: &mut fmt::Formatter<'_>) -> fmt::Result {
        let instance2 = MyStruct2;
        panic!("oh2 no, '{}' was unexpected", instance2);
        //todo!()
    }
}

fn main() {
    let instance = MyStruct;

    std::panic::always_abort();//issue: https://github.com/rust-lang/rust/issues/122940
    println!("{}", instance);
    //assert!(false, "oh no, '{}' was unexpected", instance);
}

With these rustc changes
(ignore the irrelevant parts in the patch which I've included so the output above makes sense)
The only relevant in this patch is the block of panic_count::MustAbort::AlwaysAbort => { which emulates a path (this is the wrong var(atomicbool) to track this, but is ok for the current example)

Index: /var/tmp/portage/dev-lang/rust-1.75.0-r1/work/rustc-1.75.0-src/library/std/src/panicking.rs
===================================================================
--- .orig/var/tmp/portage/dev-lang/rust-1.75.0-r1/work/rustc-1.75.0-src/library/std/src/panicking.rs
+++ /var/tmp/portage/dev-lang/rust-1.75.0-r1/work/rustc-1.75.0-src/library/std/src/panicking.rs
@@ -739,6 +739,7 @@ fn rust_panic_with_hook(
     force_no_backtrace: bool,
 ) -> ! {
     let must_abort = panic_count::increase(true);
+    rtprintpanic!("must_abort={:?}\n", must_abort);
 
     // Check if we need to abort immediately.
     if let Some(must_abort) = must_abort {
@@ -746,11 +747,27 @@ fn rust_panic_with_hook(
             panic_count::MustAbort::PanicInHook => {
                 // Don't try to print the message in this case
                 // - perhaps that is causing the recursive panics.
-                rtprintpanic!("thread panicked while processing panic. aborting.\n");
-            }
-            panic_count::MustAbort::AlwaysAbort => {
+                //rtprintpanic!("thread panicked while processing panic. aborting. msg='{:?}' loc='{:?}'\n",message, location);
+                //FIXME: I need thread local(that doesn't do allocations when accessing it) and
+                //which will let me progressively remove things from the output which might panic
+                //like the 'message' at first, then 'location', then the backtrace dump?
+                rtprintpanic!("thread panicked while processing panic. aborting. msg='not shown' loc='{:?}'\n", location);
+                //let panicinfo = PanicInfo::internal_constructor(
+                //    message,//this would infinite panic
+                //    location,
+                //    can_unwind,
+                //    force_no_backtrace,
+                //);
+                //rtprintpanic!("{panicinfo}\nhere's a stacktrace attempt:\n");
+                rtprintpanic!("{}\n", crate::backtrace::Backtrace::force_capture());
+             }
+             panic_count::MustAbort::AlwaysAbort => {
+                rtprintpanic!("in panic_count::MustAbort::AlwaysAbort\n");
                 // Unfortunately, this does not print a backtrace, because creating
                 // a `Backtrace` will allocate, which we must to avoid here.
+               static BEEN_HERE:AtomicBool=AtomicBool::new(false);
+               //FIXME: this is the wrong way; need a thread local var, but one that doesn't alloc (for the fork case)
+               if false==BEEN_HERE.swap(true, Ordering::SeqCst) {
                 let panicinfo = PanicInfo::internal_constructor(
                     message,
                     location,
@@ -758,13 +775,27 @@ fn rust_panic_with_hook(
                     force_no_backtrace,
                 );
                 rtprintpanic!("{panicinfo}\npanicked after panic::always_abort(), aborting.\n");
+                //XXX: we'd restore this even if thread local because this whole thing may end up in a
+                //landing pad and some future new panic(like if this is ran within libtest via
+                //'cargo test' with --test-threads 1, it could be catch_unwind-ed aka
+                //caught, then the next test that panics gets back here)
+                //might get itself here and have an unclean path state from this old panic we're doing now.
+                //even the abort below might get caught by atexit hook(iirc) and end up in landing
+                //pad (well, it would with my hacky patch that's about that, iirc)
+                BEEN_HERE.store(false, Ordering::SeqCst);
+               } else {//been here, prevent recursion
+                    rtprintpanic!("panicked after panic::always_abort(), aborting. (and recursion prevented) loc='{:?}'\n", location);
+               }
             }
         }
+        rtprintpanic!("about to abort_internal()\n");
         crate::sys::abort_internal();
     }
 
+    rtprintpanic!("about to mut info\n");
     let mut info =
         PanicInfo::internal_constructor(message, location, can_unwind, force_no_backtrace);
+    rtprintpanic!("about to hook read\n");
     let hook = HOOK.read().unwrap_or_else(PoisonError::into_inner);
     match *hook {
         // Some platforms (like wasm) know that printing to stderr won't ever actually
@@ -773,31 +804,48 @@ fn rust_panic_with_hook(
         // methods, this means we avoid formatting the string at all!
         // (The panic runtime might still call `payload.take_box()` though and trigger
         // formatting.)
-        Hook::Default if panic_output().is_none() => {}
+        Hook::Default if panic_output().is_none() => {
+            rtprintpanic!("1\n");
+        }
         Hook::Default => {
-            info.set_payload(payload.get());
+            rtprintpanic!("2\n");
+            let p=payload.get();
+            rtprintpanic!("22\n");
+            info.set_payload(p);
+            rtprintpanic!("222\n");
             default_hook(&info);
         }
         Hook::Custom(ref hook) => {
-            info.set_payload(payload.get());
+            rtprintpanic!("3\n");
+            let p=payload.get();
+            rtprintpanic!("33\n");
+            info.set_payload(p);
+            //info.set_payload(payload.get());
+            rtprintpanic!("333\n");
             hook(&info);
         }
     };
+    rtprintpanic!("4\n");
     drop(hook);
 
+    rtprintpanic!("5\n");
     // Indicate that we have finished executing the panic hook. After this point
     // it is fine if there is a panic while executing destructors, as long as it
     // it contained within a `catch_unwind`.
     panic_count::finished_panic_hook();
 
+    rtprintpanic!("6\n");
     if !can_unwind {
+        rtprintpanic!("7\n");
         // If a thread panics while running destructors or tries to unwind
         // through a nounwind function (e.g. extern "C") then we cannot continue
         // unwinding and have to abort immediately.
         rtprintpanic!("thread caused non-unwinding panic. aborting.\n");
+        rtprintpanic!("8ai\n");
         crate::sys::abort_internal();
     }
 
+    rtprintpanic!("9rp\n");
     rust_panic(payload)
 }
 

[correabuscar]

(Presumably thread locals cannot be done without allocating in rust, but can in C ?)

This certainly used to be true, but nowadays LOCAL_PANIC_COUNT is a const TLS variable with no destructor. So that comment may be outdated. But I am not sure whether const TLS variable with no destructor are alloc-free on all platforms. @m-ou-se or @joboet might know.

const was there since 23rd of April 2022,
then the comment

    // Accessing LOCAL_PANIC_COUNT in a child created by `libc::fork` would lead to a memory 
    // allocation.

was added at line 315 on October 6th 2022,
so that's 5 months later, but they might not have been aware that 'const' was there at line 300 when they made the comment ??

Would be nice if more people would please confirm whether or not that comment still applies even though it's const like thread_local! { static LOCAL_PANIC_COUNT: Cell<usize> = const { Cell::new(0) } } which suggests it doesn't ? or depends on platform (as per above find) ?

I'm not too sure about what the docs for thread_local! say; they appear to imply the comment is outdated(which is what RalfJung says and I too thought it to be so yesterday when I tried to ask about it on libera chat, but I guess it wasn't the right place to ask; and changed my mind later on, for some reason, and believed the comment still applies):

This macro supports a special const {} syntax that can be used when the initialization expression can be evaluated as a constant. This can enable a more efficient thread local implementation that can avoid lazy initialization. For types that do not need to be dropped, this can enable an even more efficient implementation that does not need to track any additional state.

use std::cell::Cell;
thread_local! {
    pub static FOO: Cell<u32> = const { Cell::new(1) };
}

assert_eq!(FOO.get(), 1);

(feel free to mark this comment as off-topic)

[Amanieu]

Rather than messing with thread locals, we could just add a second bit to global_count to abort immediate but without printing the panic payload. There is some potential for a race condition if 2 threads are panicking at the same time, but I think this is acceptable since always_abort is currently only used after fork which means there are no other threads in the new process.

[correabuscar]

EDIT2: actually I think I missed something when I said what I said below, if by without printing the panic payload. you meant without printing the message (i must've gotten confused due to "payload" and "message" being two different args in that function; however there's no "payload" in the always abort branch, so you could've only meant "message" there, so what I said below is only valid because I wrongly assumed that you still meant to print the "message", but this doesn't make sense given what you've said, so my bad) so, basically we both agreed that we shouldn't print the 'message' which does solve 2 problems at once. The "EDIT" at the end still applies though.
EDIT3: oh wait actually I remember, I thought you meant to abort only if this is the second time we're in the always abort branch, as if, because the first try was to print the 'message' and this itself panic-ed again so we're back here, we're not gonna try and print it again. But this means, we still tried to print 'message' in first try which leaves the user could've allocated in the Display impl, problem on the table.

Rather than messing with thread locals, we could just add a second bit to global_count to abort immediate but without printing the panic payload. There is some potential for a race condition if 2 threads are panicking at the same time, but I think this is acceptable since always_abort is currently only used after fork which means there are no other threads in the new process.

If indeed it is so, as even the comments re-iterate:

// panic::always_abort() is usually called to prevent memory allocations done by
// the panic handling in the child created by libc::fork.
// Memory allocations performed in a child created with libc::fork are undefined
// behavior in most operating systems.

then I posit that message(line 755 below) should never be shown because just like the panic! calls in the fmt of impl Display, the user may allocate there, so to be sure allocation(s) don't happen in a fork ... message cannot be used, therefore recursion is averted, thus 2 problems solved.

for reference:

rust/library/std/src/panicking.rs

Lines 751 to 760 in 9f25a04

	panic_count::MustAbort::AlwaysAbort => {
	// Unfortunately, this does not print a backtrace, because creating
	// a `Backtrace` will allocate, which we must to avoid here.
	let panicinfo = PanicInfo::internal_constructor(
	message,
	location,
	can_unwind,
	force_no_backtrace,
	);
	rtprintpanic!("{panicinfo}\npanicked after panic::always_abort(), aborting.\n");

EDIT: that usually though, if others are gonna use it(like after it's stabilized?) for other more benign purposes they won't be seeing a (proper) message ...

[RalfJung]

An alternative would be to set the message to message.and_then(fmt::Arguments::as_str).map(|s| &fmt::Arguments::new_const(&[s])) (well, not quite, the lifetimes don't work out, but you get the idea). That way panic messages that are simple strings will still work but no custom formatting code will be invoked.

In fact I could probably have done that in #122930 as well.

[joboet]

Would be nice if more people would please confirm whether or not that comment still applies even though it's const like thread_local! { static LOCAL_PANIC_COUNT: Cell<usize> = const { Cell::new(0) } } which suggests it doesn't ? or depends on platform (as per above find) ?

I'm not too sure about what the docs for thread_local! say; they appear to imply the comment is outdated(which is what RalfJung says and I too thought it to be so yesterday when I tried to ask about it on libera chat, but I guess it wasn't the right place to ask; and changed my mind later on, for some reason, and believed the comment still applies)

The comment still applies if you replace "would" by "could". On platforms like x86_64 Linux, const-initialized, !Drop variables in thread_local! do not allocate. But on platforms like GNU/Windows, we always use keys, which need to allocate.

We could get around this by using pthread_getspecific/pthread_setspecific directly on the relevant platforms, as long as the value fits into a pointer, we don't need to allocate. However, I'm not convinced this is correct (even on platforms with native TLS). fork requires all functions called after it to be async-signal safe, which both pthread_key_* and #[thread_local] are not documented to be, so all TLS accesses after fork risk running into deadlocks if the platform has not considered this extremely niche use-case. In practice, this works on GNU/Linux, but as for other platforms, who knows...

An alternative would be to set the message to message.and_then(fmt::Arguments::as_str).map(|s| &fmt::Arguments::new_const(&[s])) (well, not quite, the lifetimes don't work out, but you get the idea). That way panic messages that are simple strings will still work but no custom formatting code will be invoked.

In fact I could probably have done that in #122930 as well.

I'd much prefer that solution.