fix interleaved panic output

previously, we only held a lock for printing the backtrace itself. since all threads were printing to the same file descriptor, that meant random output in the default panic hook would be interleaved with the backtrace. now, we hold the lock for the full duration of the hook, and the output is ordered.
rust-lang · Jul 5, 2024 · 5ce6bb0 · 5ce6bb0
1 parent de14f1f
commit 5ce6bb0
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 19 deletions.
diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs
@@ -253,17 +253,21 @@ fn default_hook(info: &PanicHookInfo<'_>) {
     let name = thread.as_ref().and_then(|t| t.name()).unwrap_or("<unnamed>");
 
     let write = |err: &mut dyn crate::io::Write| {
+        // Use a lock to prevent mixed output in multithreading context.
+        // Some platforms also require it when printing a backtrace, like `SymFromAddr` on Windows.
+        let _lock = backtrace::lock();
         let _ = writeln!(err, "thread '{name}' panicked at {location}:\n{msg}");
 
         static FIRST_PANIC: AtomicBool = AtomicBool::new(true);
 
         match backtrace {
-            Some(BacktraceStyle::Short) => {
+            // SAFETY: we took out a lock just a second ago.
+            Some(BacktraceStyle::Short) => unsafe {
                 drop(backtrace::print(err, crate::backtrace_rs::PrintFmt::Short))
-            }
-            Some(BacktraceStyle::Full) => {
+            },
+            Some(BacktraceStyle::Full) => unsafe {
                 drop(backtrace::print(err, crate::backtrace_rs::PrintFmt::Full))
-            }
+            },
             Some(BacktraceStyle::Off) => {
                 if FIRST_PANIC.swap(false, Ordering::Relaxed) {
                     let _ = writeln!(

diff --git a/library/std/src/sys/backtrace.rs b/library/std/src/sys/backtrace.rs
@@ -12,13 +12,15 @@ use crate::sync::{Mutex, PoisonError};
 /// Max number of frames to print.
 const MAX_NB_FRAMES: usize = 100;
 
-pub fn lock() -> impl Drop {
+pub(crate) fn lock() -> impl Drop {
     static LOCK: Mutex<()> = Mutex::new(());
     LOCK.lock().unwrap_or_else(PoisonError::into_inner)
 }
 
 /// Prints the current backtrace.
-pub fn print(w: &mut dyn Write, format: PrintFmt) -> io::Result<()> {
+///
+/// SAFETY: this function is not Sync. The caller must hold a mutex lock, or there must be only one thread in the program.
+pub(crate) unsafe fn print(w: &mut dyn Write, format: PrintFmt) -> io::Result<()> {
     // There are issues currently linking libbacktrace into tests, and in
     // general during std's own unit tests we're not testing this path. In
     // test mode immediately return here to optimize away any references to the
@@ -27,15 +29,6 @@ pub fn print(w: &mut dyn Write, format: PrintFmt) -> io::Result<()> {
         return Ok(());
     }
 
-    // Use a lock to prevent mixed output in multithreading context.
-    // Some platforms also requires it, like `SymFromAddr` on Windows.
-    unsafe {
-        let _lock = lock();
-        _print(w, format)
-    }
-}
-
-unsafe fn _print(w: &mut dyn Write, format: PrintFmt) -> io::Result<()> {
     struct DisplayBacktrace {
         format: PrintFmt,
     }

diff --git a/tests/ui/backtrace/synchronized-panic-handler.run.stderr b/tests/ui/backtrace/synchronized-panic-handler.run.stderr
@@ -1,5 +1,5 @@
-thread '<unnamed>' panicked at $DIR/synchronized-panic-handler.rs:thread '8<unnamed>:5' panicked at :
-oops oh no woe is me$DIR/synchronized-panic-handler.rs
-:note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
-8:5:
+thread '<unnamed>' panicked at $DIR/synchronized-panic-handler.rs:8:5:
+oops oh no woe is me
+note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
+thread '<unnamed>' panicked at $DIR/synchronized-panic-handler.rs:8:5:
 oops oh no woe is me