windows_exception: Improve async signal safety (#14619)

It's not as bad as I feared to bypass libsys's stderr. (There's still a
lock in libsys's backtrace, which might also not be too bad to bypass.)

Cargo.lock

@@ -4637,6 +4637,7 @@ version = "0.7.21"
 dependencies = [
  "anstream",
  "anyhow",
+ "arrayvec",
  "assert_cmd",
  "assert_fs",
  "axoupdater",
@@ -4735,6 +4736,7 @@ dependencies = [
  "which",
  "whoami",
  "windows 0.59.0",
+ "windows-result 0.3.4",
  "wiremock",
  "zip",
 ]

Cargo.toml

@@ -75,6 +75,7 @@ uv-workspace = { path = "crates/uv-workspace" }
 anstream = { version = "0.6.15" }
 anyhow = { version = "1.0.89" }
 arcstr = { version = "1.2.0" }
+arrayvec = { version = "0.7.6" }
 astral-tokio-tar = { version = "0.5.1" }
 async-channel = { version = "2.3.1" }
 async-compression = { version = "0.4.12", features = ["bzip2", "gzip", "xz", "zstd"] }
@@ -184,7 +185,7 @@ url = { version = "2.5.2", features = ["serde"] }
 version-ranges = { git = "https://github.com/astral-sh/pubgrub", rev = "06ec5a5f59ffaeb6cf5079c6cb184467da06c9db" }
 walkdir = { version = "2.5.0" }
 which = { version = "8.0.0", features = ["regex"] }
-windows = { version = "0.59.0", features = ["Win32_System_Kernel", "Win32_System_Diagnostics_Debug", "Win32_Storage_FileSystem"] }
+windows = { version = "0.59.0", features = ["Win32_Globalization", "Win32_System_Console", "Win32_System_Kernel", "Win32_System_Diagnostics_Debug", "Win32_Storage_FileSystem"] }
 windows-core = { version = "0.59.0" }
 windows-registry = { version = "0.5.0" }
 windows-result = { version = "0.3.0" }

crates/uv/Cargo.toml

@@ -107,8 +107,10 @@ which = { workspace = true }
 zip = { workspace = true }
 
 [target.'cfg(target_os = "windows")'.dependencies]
+arrayvec = { workspace = true }
 self-replace = { workspace = true }
 windows = { workspace = true }
+windows-result = { workspace = true }
 
 [dev-dependencies]
 assert_cmd = { version = "2.0.16" }

crates/uv/src/windows_exception.rs

@@ -9,121 +9,304 @@
 //! implementation and also displays some minimal information from the exception itself.
 
 #![allow(unsafe_code)]
-#![allow(clippy::print_stderr)]
+// Usually we want fs_err over std::fs, but there's no advantage here, we don't
+// report errors encountered while reporting an exception.
+#![allow(clippy::disallowed_types)]
 
+use std::fmt::Write;
+use std::fs::File;
+use std::mem::ManuallyDrop;
+use std::os::windows::io::FromRawHandle;
+
+use arrayvec::ArrayVec;
 use windows::Win32::{
     Foundation,
+    Globalization::CP_UTF8,
+    System::Console::{
+        CONSOLE_MODE, GetConsoleMode, GetConsoleOutputCP, GetStdHandle, STD_ERROR_HANDLE,
+        WriteConsoleW,
+    },
     System::Diagnostics::Debug::{
         CONTEXT, EXCEPTION_CONTINUE_SEARCH, EXCEPTION_POINTERS, SetUnhandledExceptionFilter,
     },
 };
 
-fn display_exception_info(name: &str, info: &[usize; 15]) {
+/// A write target for standard error that can be safely used in an exception handler.
-    match info[0] {
+///
-        0 => eprintln!("{name} reading {:#x}", info[1]),
+/// The exception handler can be called at any point in the execution of machine code, perhaps
-        1 => eprintln!("{name} writing {:#x}", info[1]),
+/// halfway through a Rust operation. It needs to be robust to operating with unknown program
-        8 => eprintln!("{name} executing {:#x}", info[1]),
+/// state, a concept that the UNIX world calls "async signal safety." In particular, we can't
-        _ => eprintln!("{name} from operation {} at {:#x}", info[0], info[1]),
+/// write to `std::io::stderr()` because that takes a lock, and we could be called in the middle of
+/// code that is holding that lock.
+enum ExceptionSafeStderr {
+    // This is a simplified version of the logic in Rust std::sys::stdio::windows, on the
+    // assumption that we're only writing strs, not bytes (so we do not need to care about
+    // incomplete or invalid UTF-8) and we don't care about Windows 7 or every drop of
+    // performance.
+    // - If stderr is a non-UTF-8 console, we need to write UTF-16 with WriteConsoleW, and we
+    //   convert with encode_utf16().
+    // - If stderr is not a console, we cannot use WriteConsole and must use NtWriteFile, which
+    //   takes (UTF-8) bytes.
+    // - If stderr is a UTF-8 console, we can do either. std uses NtWriteFile.
+    // Note that we do not want to close stderr at any point, hence ManuallyDrop.
+    WriteConsole(Foundation::HANDLE),
+    NtWriteFile(ManuallyDrop<File>),
+}
+
+impl ExceptionSafeStderr {
+    fn new() -> Result<Self, windows_result::Error> {
+        // SAFETY: winapi call, no interesting parameters
+        let handle = unsafe { GetStdHandle(STD_ERROR_HANDLE) }?;
+        if handle.is_invalid() {
+            return Err(windows_result::Error::empty());
+        }
+        let mut mode = CONSOLE_MODE::default();
+        // SAFETY: winapi calls, no interesting parameters
+        if unsafe {
+            GetConsoleMode(handle, &raw mut mode).is_ok() && GetConsoleOutputCP() != CP_UTF8
+        } {
+            Ok(Self::WriteConsole(handle))
+        } else {
+            // SAFETY: winapi call, we just got this handle from the OS and checked it
+            let file = unsafe { File::from_raw_handle(handle.0) };
+            Ok(Self::NtWriteFile(ManuallyDrop::new(file)))
+        }
+    }
+
+    fn write_winerror(&mut self, s: &str) -> Result<(), windows_result::Error> {
+        match self {
+            Self::WriteConsole(handle) => {
+                // According to comments in the ReactOS source, NT's behavior is that writes of 80
+                // bytes or fewer are passed in-line in the message to the console server and
+                // longer writes allocate out of a shared heap with CSRSS. In an attempt to avoid
+                // allocations, write in 80-byte chunks.
+                let mut buf = ArrayVec::<u16, 40>::new();
+                for c in s.encode_utf16() {
+                    if buf.try_push(c).is_err() {
+                        // SAFETY: winapi call, arrayvec guarantees the slice is valid
+                        unsafe { WriteConsoleW(*handle, &buf, None, None) }?;
+                        buf.clear();
+                        buf.push(c);
+                    }
+                }
+                if !buf.is_empty() {
+                    // SAFETY: winapi call, arrayvec guarantees the slice is valid
+                    unsafe { WriteConsoleW(*handle, &buf, None, None) }?;
+                }
+            }
+            Self::NtWriteFile(file) => {
+                use std::io::Write;
+                file.write_all(s.as_bytes())?;
+            }
+        }
+        Ok(())
     }
 }
 
+impl Write for ExceptionSafeStderr {
+    fn write_str(&mut self, s: &str) -> std::fmt::Result {
+        self.write_winerror(s).map_err(|_| std::fmt::Error)
+    }
+}
+
+fn display_exception_info(
+    e: &mut ExceptionSafeStderr,
+    name: &str,
+    info: &[usize; 15],
+) -> std::fmt::Result {
+    match info[0] {
+        0 => writeln!(e, "{name} reading {:#x}", info[1])?,
+        1 => writeln!(e, "{name} writing {:#x}", info[1])?,
+        8 => writeln!(e, "{name} executing {:#x}", info[1])?,
+        _ => writeln!(e, "{name} from operation {} at {:#x}", info[0], info[1])?,
+    }
+    Ok(())
+}
+
 #[cfg(target_arch = "x86")]
-fn dump_regs(c: &CONTEXT) {
+fn dump_regs(e: &mut ExceptionSafeStderr, c: &CONTEXT) -> std::fmt::Result {
-    eprintln!(
+    let CONTEXT {
-        "eax={:08x} ebx={:08x} ecx={:08x} edx={:08x} esi={:08x} edi={:08x}",
+        Eax,
-        c.Eax, c.Ebx, c.Ecx, c.Edx, c.Esi, c.Edi
+        Ebx,
-    );
+        Ecx,
-    eprintln!(
+        Edx,
-        "eip={:08x} ebp={:08x} esp={:08x} eflags={:08x}",
+        Esi,
-        c.Eip, c.Ebp, c.Esp, c.EFlags
+        Edi,
-    );
+        Eip,
+        Ebp,
+        Esp,
+        EFlags,
+        ..
+    } = c;
+    writeln!(
+        e,
+        "eax={Eax:08x} ebx={Ebx:08x} ecx={Ecx:08x} edx={Edx:08x} esi={Esi:08x} edi={Edi:08x}"
+    )?;
+    writeln!(
+        e,
+        "eip={Eip:08x} ebp={Ebp:08x} esp={Esp:08x} eflags={EFlags:08x}"
+    )?;
+    Ok(())
 }
 
 #[cfg(target_arch = "x86_64")]
-fn dump_regs(c: &CONTEXT) {
+fn dump_regs(e: &mut ExceptionSafeStderr, c: &CONTEXT) -> std::fmt::Result {
-    eprintln!("rax={:016x} rbx={:016x} rcx={:016x}", c.Rax, c.Rbx, c.Rcx);
+    let CONTEXT {
-    eprintln!("rdx={:016x} rsx={:016x} rdi={:016x}", c.Rdx, c.Rsi, c.Rdi);
+        Rax,
-    eprintln!("rsp={:016x} rbp={:016x}  r8={:016x}", c.Rsp, c.Rbp, c.R8);
+        Rbx,
-    eprintln!(" r9={:016x} r10={:016x} r11={:016x}", c.R9, c.R10, c.R11);
+        Rcx,
-    eprintln!("r12={:016x} r13={:016x} r14={:016x}", c.R12, c.R13, c.R14);
+        Rdx,
-    eprintln!(
+        Rsi,
-        "r15={:016x} rip={:016x} eflags={:016x}",
+        Rdi,
-        c.R15, c.Rip, c.EFlags
+        Rsp,
-    );
+        Rbp,
+        R8,
+        R9,
+        R10,
+        R11,
+        R12,
+        R13,
+        R14,
+        R15,
+        Rip,
+        EFlags,
+        ..
+    } = c;
+    writeln!(e, "rax={Rax:016x} rbx={Rbx:016x} rcx={Rcx:016x}")?;
+    writeln!(e, "rdx={Rdx:016x} rsi={Rsi:016x} rdi={Rdi:016x}")?;
+    writeln!(e, "rsp={Rsp:016x} rbp={Rbp:016x}  r8={R8 :016x}")?;
+    writeln!(e, " r9={R9 :016x} r10={R10:016x} r11={R11:016x}")?;
+    writeln!(e, "r12={R12:016x} r13={R13:016x} r14={R14:016x}")?;
+    writeln!(e, "r15={R15:016x} rip={Rip:016x} eflags={EFlags:016x}")?;
+    Ok(())
 }
 
 #[cfg(target_arch = "aarch64")]
-fn dump_regs(c: &CONTEXT) {
+fn dump_regs(e: &mut ExceptionSafeStderr, c: &CONTEXT) -> std::fmt::Result {
+    let CONTEXT { Cpsr, Sp, Pc, .. } = c;
     // SAFETY: The two variants of this anonymous union are equivalent,
     // one's an array and one has named registers.
-    let r = unsafe { c.Anonymous.Anonymous };
+    let regs = unsafe { c.Anonymous.Anonymous };
-    eprintln!("cpsr={:016x}  sp={:016x}  pc={:016x}", c.Cpsr, c.Sp, c.Pc);
+    let Windows::Win32::System::Diagnostics::Debug::CONTEXT_0_0 {
-    eprintln!("  x0={:016x}  x1={:016x}  x2={:016x}", r.X0, r.X1, r.X2);
+        X0,
-    eprintln!("  x3={:016x}  x4={:016x}  x5={:016x}", r.X3, r.X4, r.X5);
+        X1,
-    eprintln!("  x6={:016x}  x7={:016x}  x8={:016x}", r.X6, r.X7, r.X8);
+        X2,
-    eprintln!("  x9={:016x} x10={:016x} x11={:016x}", r.X9, r.X10, r.X11);
+        X3,
-    eprintln!(" x12={:016x} x13={:016x} x14={:016x}", r.X12, r.X13, r.X14);
+        X4,
-    eprintln!(" x15={:016x} x16={:016x} x17={:016x}", r.X15, r.X16, r.X17);
+        X5,
-    eprintln!(" x18={:016x} x19={:016x} x20={:016x}", r.X18, r.X19, r.X20);
+        X6,
-    eprintln!(" x21={:016x} x22={:016x} x23={:016x}", r.X21, r.X22, r.X23);
+        X7,
-    eprintln!(" x24={:016x} x25={:016x} x26={:016x}", r.X24, r.X25, r.X26);
+        X8,
-    eprintln!(" x27={:016x} x28={:016x}", r.X27, r.X28);
+        X9,
-    eprintln!("  fp={:016x}  lr={:016x}", r.Fp, r.Lr);
+        X10,
+        X11,
+        X12,
+        X13,
+        X14,
+        X15,
+        X16,
+        X17,
+        X18,
+        X19,
+        X20,
+        X21,
+        X22,
+        X23,
+        X24,
+        X25,
+        X26,
+        X27,
+        X28,
+        Fp,
+        Lr,
+    } = regs;
+    writeln!(e, "cpsr={Cpsr:016x}  sp={Sp :016x}  pc={Pc :016x}")?;
+    writeln!(e, "  x0={X0  :016x}  x1={X1 :016x}  x2={X2 :016x}")?;
+    writeln!(e, "  x3={X3  :016x}  x4={X4 :016x}  x5={X5 :016x}")?;
+    writeln!(e, "  x6={X6  :016x}  x7={X7 :016x}  x8={X8 :016x}")?;
+    writeln!(e, "  x9={X9  :016x} x10={X10:016x} x11={X11:016x}")?;
+    writeln!(e, " x12={X12 :016x} x13={X13:016x} x14={X14:016x}")?;
+    writeln!(e, " x15={X15 :016x} x16={X16:016x} x17={X17:016x}")?;
+    writeln!(e, " x18={X18 :016x} x19={X19:016x} x20={X20:016x}")?;
+    writeln!(e, " x21={X21 :016x} x22={X22:016x} x23={X23:016x}")?;
+    writeln!(e, " x24={X24 :016x} x25={X25:016x} x26={X26:016x}")?;
+    writeln!(e, " x27={X27 :016x} x28={X28:016x}")?;
+    writeln!(e, "  fp={Fp  :016x}  lr={Lr :016x}")?;
+    Ok(())
 }
 
-unsafe extern "system" fn unhandled_exception_filter(
+fn dump_exception(exception_info: *const EXCEPTION_POINTERS) -> std::fmt::Result {
-    exception_info: *const EXCEPTION_POINTERS,
+    let mut e = ExceptionSafeStderr::new().map_err(|_| std::fmt::Error)?;
-) -> i32 {
+    writeln!(e, "error: unhandled exception in uv, please report a bug:")?;
-    // TODO: Really we should not be using eprintln here because Stderr is not async-signal-safe.
-    // Probably we should be calling the console APIs directly.
-    eprintln!("error: unhandled exception in uv, please report a bug:");
     let mut context = None;
     // SAFETY: Pointer comes from the OS
     if let Some(info) = unsafe { exception_info.as_ref() } {
         // SAFETY: Pointer comes from the OS
         if let Some(exc) = unsafe { info.ExceptionRecord.as_ref() } {
-            eprintln!(
+            writeln!(
+                e,
                 "code {:#X} at address {:?}",
                 exc.ExceptionCode.0, exc.ExceptionAddress
-            );
+            )?;
             match exc.ExceptionCode {
                 Foundation::EXCEPTION_ACCESS_VIOLATION => {
-                    display_exception_info("EXCEPTION_ACCESS_VIOLATION", &exc.ExceptionInformation);
+                    display_exception_info(
+                        &mut e,
+                        "EXCEPTION_ACCESS_VIOLATION",
+                        &exc.ExceptionInformation,
+                    )?;
                 }
                 Foundation::EXCEPTION_IN_PAGE_ERROR => {
-                    display_exception_info("EXCEPTION_IN_PAGE_ERROR", &exc.ExceptionInformation);
+                    display_exception_info(
+                        &mut e,
+                        "EXCEPTION_IN_PAGE_ERROR",
+                        &exc.ExceptionInformation,
+                    )?;
                 }
                 Foundation::EXCEPTION_ILLEGAL_INSTRUCTION => {
-                    eprintln!("EXCEPTION_ILLEGAL_INSTRUCTION");
+                    writeln!(e, "EXCEPTION_ILLEGAL_INSTRUCTION")?;
                 }
                 Foundation::EXCEPTION_STACK_OVERFLOW => {
-                    eprintln!("EXCEPTION_STACK_OVERFLOW");
+                    writeln!(e, "EXCEPTION_STACK_OVERFLOW")?;
                 }
                 _ => {}
             }
         } else {
-            eprintln!("(ExceptionRecord is NULL)");
+            writeln!(e, "(ExceptionRecord is NULL)")?;
         }
         // SAFETY: Pointer comes from the OS
         context = unsafe { info.ContextRecord.as_ref() };
     } else {
-        eprintln!("(ExceptionInfo is NULL)");
+        writeln!(e, "(ExceptionInfo is NULL)")?;
     }
+    // TODO: std::backtrace does a lot of allocations, so we are no longer async-signal-safe at
+    // this point, but hopefully we got a useful error message on screen already. We could do a
+    // better job by using backtrace-rs directly + arrayvec.
     let backtrace = std::backtrace::Backtrace::capture();
     if backtrace.status() == std::backtrace::BacktraceStatus::Disabled {
-        eprintln!("note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace");
+        writeln!(
+            e,
+            "note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace"
+        )?;
     } else {
         if let Some(context) = context {
-            dump_regs(context);
+            dump_regs(&mut e, context)?;
         }
-        eprintln!("stack backtrace:\n{backtrace:#}");
+        writeln!(e, "stack backtrace:\n{backtrace:#}")?;
     }
+    Ok(())
+}
+
+unsafe extern "system" fn unhandled_exception_filter(
+    exception_info: *const EXCEPTION_POINTERS,
+) -> i32 {
+    let _ = dump_exception(exception_info);
     EXCEPTION_CONTINUE_SEARCH
 }
 
 /// Set up our handler for unhandled exceptions.
 pub(crate) fn setup() {
-    // SAFETY: winapi call
+    // SAFETY: winapi call, argument is a mostly async-signal-safe function
     unsafe {
         SetUnhandledExceptionFilter(Some(Some(unhandled_exception_filter)));
     }