From bdbbfc4627795823066bc1aabd2002b978d84ac1 Mon Sep 17 00:00:00 2001 From: Chris Martin Date: Wed, 5 Aug 2026 17:44:40 -0400 Subject: [PATCH 1/2] Isolated Processes 11: Use a user-chosen handler for drop failures Logging drop failures is generally a good idea... Except that logging in the executor could be a disaster if a logger has already been installed that tries to print to STDERR. This allows the user to install their own optional Drop failure handler. In an executor process, this simply wouldn't ever be used, so the executor will always do nothing when a Drop fails. Unfortunate, but not much else we can do. --- Cargo.lock | 1 - crates/linux/process-backend/Cargo.toml | 1 - .../process-backend/src/drop_fail_handler.rs | 31 +++++++++++++++++++ crates/linux/process-backend/src/lib.rs | 5 +++ crates/linux/process-backend/src/local/mod.rs | 4 +-- .../src/local/module_reader.rs | 2 +- src/linux/process_inspection/mod.rs | 11 +++++++ 7 files changed, 50 insertions(+), 5 deletions(-) create mode 100644 crates/linux/process-backend/src/drop_fail_handler.rs diff --git a/Cargo.lock b/Cargo.lock index 3edcd216..8cdfe142 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1664,7 +1664,6 @@ name = "process-backend" version = "0.1.0" dependencies = [ "libc", - "log", "process-reader", "serde", "thiserror 2.0.19", diff --git a/crates/linux/process-backend/Cargo.toml b/crates/linux/process-backend/Cargo.toml index da1a306b..d2329ec2 100644 --- a/crates/linux/process-backend/Cargo.toml +++ b/crates/linux/process-backend/Cargo.toml @@ -13,7 +13,6 @@ testing = [] [target.'cfg(any(target_os = "linux", target_os = "android"))'.dependencies] libc.workspace = true -log.workspace = true process-reader = { version = "0.1.0", path = "../../process-reader", features = ["serde"] } serde.workspace = true thiserror.workspace = true diff --git a/crates/linux/process-backend/src/drop_fail_handler.rs b/crates/linux/process-backend/src/drop_fail_handler.rs new file mode 100644 index 00000000..ce509b36 --- /dev/null +++ b/crates/linux/process-backend/src/drop_fail_handler.rs @@ -0,0 +1,31 @@ +use core::sync::atomic::{AtomicPtr, Ordering}; + +static HANDLER: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + +pub struct Handler { + pub(crate) f: for<'a> fn(core::fmt::Arguments<'a>), +} + +impl Handler { + pub const fn new(f: for<'a> fn(core::fmt::Arguments<'a>)) -> Self { + Self { f } + } + pub fn install_global(&'static self) { + HANDLER.store((self as *const Handler).cast_mut(), Ordering::Release); + } +} + +pub(crate) fn get() -> Option<&'static Handler> { + let ptr: *const Handler = HANDLER.load(Ordering::Acquire).cast_const(); + if ptr.is_null() { + None + } else { + Some(unsafe { &*ptr }) + } +} + +macro_rules! report_drop_failed(($($arg: tt)*) => {{ + if let Some(handler) = $crate::drop_fail_handler::get() { + (handler.f)(::core::format_args!($($arg)*)); + } +}}); diff --git a/crates/linux/process-backend/src/lib.rs b/crates/linux/process-backend/src/lib.rs index 62c5030b..6c9c72ce 100644 --- a/crates/linux/process-backend/src/lib.rs +++ b/crates/linux/process-backend/src/lib.rs @@ -1,6 +1,11 @@ #![no_std] #![cfg(any(target_os = "linux", target_os = "android"))] +pub use drop_fail_handler::Handler as DropFailHandler; + +#[macro_use] +mod drop_fail_handler; + pub mod local; pub mod regs; diff --git a/crates/linux/process-backend/src/local/mod.rs b/crates/linux/process-backend/src/local/mod.rs index f96c75ef..288f4e06 100644 --- a/crates/linux/process-backend/src/local/mod.rs +++ b/crates/linux/process-backend/src/local/mod.rs @@ -387,7 +387,7 @@ impl Drop for DirReader { fn drop(&mut self) { let rv = unsafe { libc::closedir(self.dirp) }; if rv == -1 { - log::debug!("failed to close directory: {}", errno()); + report_drop_failed!("failed to close directory: {}", errno()); } } } @@ -418,7 +418,7 @@ impl Drop for OwnedFd { fn drop(&mut self) { let rv = unsafe { libc::close(self.0) }; if rv == -1 { - log::error!("failed to close file: {}", errno()); + report_drop_failed!("failed to close file: {}", errno()); } } } diff --git a/crates/linux/process-backend/src/local/module_reader.rs b/crates/linux/process-backend/src/local/module_reader.rs index 7871a567..dfcb5762 100644 --- a/crates/linux/process-backend/src/local/module_reader.rs +++ b/crates/linux/process-backend/src/local/module_reader.rs @@ -149,7 +149,7 @@ impl Drop for Mapped { fn drop(&mut self) { let rv = unsafe { libc::munmap(self.ptr, self.len) }; if rv == -1 { - log::error!("failed to unmap memory: {}", errno()); + report_drop_failed!("failed to unmap memory: {}", errno()); } } } diff --git a/src/linux/process_inspection/mod.rs b/src/linux/process_inspection/mod.rs index b186c2e1..52a03f58 100644 --- a/src/linux/process_inspection/mod.rs +++ b/src/linux/process_inspection/mod.rs @@ -31,6 +31,8 @@ pub enum Backend { impl ProcessInspector { pub fn local(pid: libc::pid_t) -> Self { + set_process_backend_drop_fail_handler(); + let backend = local::Backend::new(pid); ProcessInspector { @@ -258,3 +260,12 @@ pub enum Error { #[error("an error occurred running a syscall directly")] Local(#[source] local::Error), } + +fn set_process_backend_drop_fail_handler() { + fn drop_fail_handler(args: core::fmt::Arguments<'_>) { + log::error!("a drop() error occurred in process-backend: {args}"); + } + const HANDLER: process_backend::DropFailHandler = + process_backend::DropFailHandler::new(drop_fail_handler); + HANDLER.install_global(); +} From 5857d3a083f655c48b1d3ff03fc689cc18377414 Mon Sep 17 00:00:00 2001 From: Chris Martin Date: Mon, 17 Aug 2026 11:12:44 -0400 Subject: [PATCH 2/2] Address review feedback from Gabriele --- .../process-backend/src/drop_fail_handler.rs | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/crates/linux/process-backend/src/drop_fail_handler.rs b/crates/linux/process-backend/src/drop_fail_handler.rs index ce509b36..b9056ae7 100644 --- a/crates/linux/process-backend/src/drop_fail_handler.rs +++ b/crates/linux/process-backend/src/drop_fail_handler.rs @@ -1,3 +1,22 @@ +//! Set a custom handler for when a Drop failure occurs +//! +//! Normally, there is not much that can be done when an error occurs during execution of `drop()` +//! -- Common strategies are to just ignore it, or to just log it and ignore it. +//! +//! We prefer the latter approach, but the issue is that if a Rust global logger has been set up +//! in the crashed process that will run the Executor, that could result in `log` being used in +//! a signal handler context. And since common logging behavior is to print to stderr, that will +//! almost-certainly cause a deadlock or some kind of other re-entrancy issue. +//! +//! Instead, we don't prescribe a behavior - We allow the user of minidump-writer to set their +//! own handler for failed `drop()` calls. If the user knows it's safe to log or print or +//! whatever, they can do that. +//! +//! Note that using [`install_global()`] to install a new handler will always overwrite any +//! previously-installed handler. This is intentional, as the user of this crate may want to +//! disable a previously-installed handler after a crash occurs (if that handler would cause +//! re-entrancy issues) + use core::sync::atomic::{AtomicPtr, Ordering}; static HANDLER: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); @@ -7,9 +26,13 @@ pub struct Handler { } impl Handler { + /// Decorate the given function as a potential global Drop handler pub const fn new(f: for<'a> fn(core::fmt::Arguments<'a>)) -> Self { Self { f } } + /// Install this handler as the new global Drop handler + /// + /// Will overwrite any previously installed handler pub fn install_global(&'static self) { HANDLER.store((self as *const Handler).cast_mut(), Ordering::Release); }