Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin_tests/tests/crashtracker_bin_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ fn test_collector_no_allocations_stacktrace_modes() {
let _ = fs::remove_file(&detector_log_path);

let config = CrashTestConfig::new(
BuildProfile::Debug,
Copy link
Copy Markdown
Contributor Author

@gyuheon0h gyuheon0h Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I do this

SIGCHLD -> SIG_IGN -> kernel auto-reaps collector child -> waitpid returns ECHILD -> reap_child_non_blocking returns unexpected result -> debug_assert_eq! at process_handle.rs:33 fails -> format! allocates to build the panic message -> preload detector catches malloc.

We should be running this test in release mode anyways

BuildProfile::Release,
TestMode::RuntimePreloadLogger,
CrashType::NullDeref,
)
Expand Down
10 changes: 10 additions & 0 deletions libdd-crashtracker/src/collector/crash_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

use super::collector_manager::Collector;
use super::receiver_manager::Receiver;
use super::saguard::SaGuard;
use super::signal_handler_manager::chain_signal_handler;
use crate::crash_info::Metadata;
use crate::shared::configuration::CrashtrackerConfiguration;
Expand Down Expand Up @@ -263,6 +264,15 @@ fn handle_posix_signal_impl(
return Ok(());
}

// Block SIGCHLD and SIGPIPE during crash handling. Our collector spawns child processes
// and writes to pipes, both of which can generate these signals. Rather than risk
// re-entering a signal handler or aborting due to SIGPIPE, suppress them for the
// duration and restore when the guard drops
let _sa_guard = SaGuard::new(&[
nix::sys::signal::Signal::SIGCHLD,
nix::sys::signal::Signal::SIGPIPE,
Comment on lines +272 to +273
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure those are the only signals to suppress?

Copy link
Copy Markdown
Contributor Author

@gyuheon0h gyuheon0h Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so, because those are the only two signals the crash handler itself can cause:

SIGCHLD: handle_posix_signal_impl calls alt_fork() to spawn the collector child process and the receiver. When those children exit, the kernel delivers SIGCHLD to the parent.
SIGPIPE: The collector communicates over Unix socket pipes. If the receiver side closes early, writing to the pipe generates SIGPIPE.

The SaGuard's purpose isn't to block all signals but to prevent the crash handler from re-entering the application's signal handlers due to side effects of its own work

WDYT?

Copy link
Copy Markdown
Contributor Author

@gyuheon0h gyuheon0h Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For papertrail and context:

Suppressing SIGPIPE AND SIGCHLD was the previous behavior before this guard was accidentally removed.

I will investigate chaining SIGCHLD

]);

// Take config and metadata out of global storage.
// We borrow via raw pointer and intentionally leak (do not reconstruct the Box) to avoid
// calling `drop`, and therefore `free`, inside a signal handler, which is not
Expand Down
1 change: 1 addition & 0 deletions libdd-crashtracker/src/collector/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ mod crash_handler;
mod emitters;
mod process_handle;
mod receiver_manager;
mod saguard;
mod signal_handler_manager;
mod spans;

Expand Down
73 changes: 73 additions & 0 deletions libdd-crashtracker/src/collector/saguard.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,76 @@ impl<const N: usize> Drop for SaGuard<N> {
);
}
}

#[cfg(test)]
mod tests {
use super::*;
use nix::sys::signal::{self, Signal};
use nix::unistd::Pid;
use std::sync::atomic::{AtomicBool, Ordering};

#[test]
#[cfg_attr(miri, ignore)]
fn signal_is_ignored_while_guard_is_active() {
let _guard = SaGuard::<1>::new(&[Signal::SIGUSR1]).unwrap();

// Send SIGUSR1 to the process. The default action is to terminate, so if
// the guard didn't set SIG_IGN this test process would die
signal::kill(Pid::this(), Signal::SIGUSR1).unwrap();
}

/// After the guard is dropped, the original handler should be restored.
/// Install a custom handler, create a guard,drop the guard, then send the
/// signal and verify the custom handler fires
#[test]
#[cfg_attr(miri, ignore)]
fn original_handler_restored_after_drop() {
static HANDLER_CALLED: AtomicBool = AtomicBool::new(false);

extern "C" fn custom_handler(_: libc::c_int) {
HANDLER_CALLED.store(true, Ordering::SeqCst);
}

// Install a custom handler
let custom_action = SigAction::new(
SigHandler::Handler(custom_handler),
SaFlags::empty(),
signal::SigSet::empty(),
);
let prev = unsafe { signal::sigaction(Signal::SIGUSR2, &custom_action).unwrap() };

// Create then drop the guard (dropped when out of scope)
{
let _guard = SaGuard::<1>::new(&[Signal::SIGUSR2]).unwrap();
signal::kill(Pid::this(), Signal::SIGUSR2).unwrap();
assert!(
!HANDLER_CALLED.load(Ordering::SeqCst),
"custom handler should not fire while guard is active"
);
}
// Guard is dropped; custom handler should be restored
HANDLER_CALLED.store(false, Ordering::SeqCst);
unsafe {
libc::raise(Signal::SIGUSR2 as libc::c_int);
}
assert!(
HANDLER_CALLED.load(Ordering::SeqCst),
"custom handler should fire after guard is dropped"
);

// Restore original handler
unsafe {
signal::sigaction(Signal::SIGUSR2, &prev).unwrap();
}
}

#[test]
#[cfg_attr(miri, ignore)]
fn multiple_signals_ignored() {
let _guard = SaGuard::<2>::new(&[Signal::SIGUSR1, Signal::SIGUSR2]).unwrap();

// Both signals should be safely ignored
signal::kill(Pid::this(), Signal::SIGUSR1).unwrap();
signal::kill(Pid::this(), Signal::SIGUSR2).unwrap();
}
}
Loading