Skip to content

Commit adaeb4e

Browse files
authored
fix(crashtracking): guard sigchld and sigpipe during crashtracker signal handler execution (#1771)
# What does this PR do? Guards SIGCHLD and SIGPIPE during crashtracker signal handler execution # Motivation During execution of the signal handler, it cannot be guaranteed that the signal is handled without SA_NODEFER, thus it also cannot be guaranteed that signals like SIGCHLD and SIGPIPE will _not_ be emitted during this handler as a result of the handler itself. At the same time, it isn't known whether it is safe to merely block all signals, as the user's own handler will be given the chance to execute after ours. Thus, we need to prevent the emission of signals we might create (and cannot be created during a signal handler except by our own execution) and defer any other signals. To put it another way, it is conceivable that the crash handling code will emit SIGCHLD or SIGPIPE, and instead of risking responding to those signals, it needs to suppress them. On the other hand, it can't just "block" (`sigprocmask()`) those signals because this will only defer them to the next handler. # Additional Notes This was originally implemented in: [Crashtracker receiver is spawned on crash](#692) but subsequently removed. # How to test the change? Unit tests for saguard.rs. Integration test will be in a following PR Co-authored-by: gyuheon.oh <gyuheon.oh@datadoghq.com>
1 parent d1eb663 commit adaeb4e

4 files changed

Lines changed: 85 additions & 1 deletion

File tree

bin_tests/tests/crashtracker_bin_test.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ fn test_collector_no_allocations_stacktrace_modes() {
299299
let _ = fs::remove_file(&detector_log_path);
300300

301301
let config = CrashTestConfig::new(
302-
BuildProfile::Debug,
302+
BuildProfile::Release,
303303
TestMode::RuntimePreloadLogger,
304304
CrashType::NullDeref,
305305
)

libdd-crashtracker/src/collector/crash_handler.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
use super::collector_manager::Collector;
77
use super::receiver_manager::Receiver;
8+
use super::saguard::SaGuard;
89
use super::signal_handler_manager::chain_signal_handler;
910
use crate::crash_info::Metadata;
1011
use crate::shared::configuration::CrashtrackerConfiguration;
@@ -263,6 +264,15 @@ fn handle_posix_signal_impl(
263264
return Ok(());
264265
}
265266

267+
// Block SIGCHLD and SIGPIPE during crash handling. Our collector spawns child processes
268+
// and writes to pipes, both of which can generate these signals. Rather than risk
269+
// re-entering a signal handler or aborting due to SIGPIPE, suppress them for the
270+
// duration and restore when the guard drops
271+
let _sa_guard = SaGuard::new(&[
272+
nix::sys::signal::Signal::SIGCHLD,
273+
nix::sys::signal::Signal::SIGPIPE,
274+
]);
275+
266276
// Take config and metadata out of global storage.
267277
// We borrow via raw pointer and intentionally leak (do not reconstruct the Box) to avoid
268278
// calling `drop`, and therefore `free`, inside a signal handler, which is not

libdd-crashtracker/src/collector/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ mod crash_handler;
1010
mod emitters;
1111
mod process_handle;
1212
mod receiver_manager;
13+
mod saguard;
1314
mod signal_handler_manager;
1415
mod spans;
1516

libdd-crashtracker/src/collector/saguard.rs

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,3 +84,76 @@ impl<const N: usize> Drop for SaGuard<N> {
8484
);
8585
}
8686
}
87+
88+
#[cfg(test)]
89+
mod tests {
90+
use super::*;
91+
use nix::sys::signal::{self, Signal};
92+
use nix::unistd::Pid;
93+
use std::sync::atomic::{AtomicBool, Ordering};
94+
95+
#[test]
96+
#[cfg_attr(miri, ignore)]
97+
fn signal_is_ignored_while_guard_is_active() {
98+
let _guard = SaGuard::<1>::new(&[Signal::SIGUSR1]).unwrap();
99+
100+
// Send SIGUSR1 to the process. The default action is to terminate, so if
101+
// the guard didn't set SIG_IGN this test process would die
102+
signal::kill(Pid::this(), Signal::SIGUSR1).unwrap();
103+
}
104+
105+
/// After the guard is dropped, the original handler should be restored.
106+
/// Install a custom handler, create a guard,drop the guard, then send the
107+
/// signal and verify the custom handler fires
108+
#[test]
109+
#[cfg_attr(miri, ignore)]
110+
fn original_handler_restored_after_drop() {
111+
static HANDLER_CALLED: AtomicBool = AtomicBool::new(false);
112+
113+
extern "C" fn custom_handler(_: libc::c_int) {
114+
HANDLER_CALLED.store(true, Ordering::SeqCst);
115+
}
116+
117+
// Install a custom handler
118+
let custom_action = SigAction::new(
119+
SigHandler::Handler(custom_handler),
120+
SaFlags::empty(),
121+
signal::SigSet::empty(),
122+
);
123+
let prev = unsafe { signal::sigaction(Signal::SIGUSR2, &custom_action).unwrap() };
124+
125+
// Create then drop the guard (dropped when out of scope)
126+
{
127+
let _guard = SaGuard::<1>::new(&[Signal::SIGUSR2]).unwrap();
128+
signal::kill(Pid::this(), Signal::SIGUSR2).unwrap();
129+
assert!(
130+
!HANDLER_CALLED.load(Ordering::SeqCst),
131+
"custom handler should not fire while guard is active"
132+
);
133+
}
134+
// Guard is dropped; custom handler should be restored
135+
HANDLER_CALLED.store(false, Ordering::SeqCst);
136+
unsafe {
137+
libc::raise(Signal::SIGUSR2 as libc::c_int);
138+
}
139+
assert!(
140+
HANDLER_CALLED.load(Ordering::SeqCst),
141+
"custom handler should fire after guard is dropped"
142+
);
143+
144+
// Restore original handler
145+
unsafe {
146+
signal::sigaction(Signal::SIGUSR2, &prev).unwrap();
147+
}
148+
}
149+
150+
#[test]
151+
#[cfg_attr(miri, ignore)]
152+
fn multiple_signals_ignored() {
153+
let _guard = SaGuard::<2>::new(&[Signal::SIGUSR1, Signal::SIGUSR2]).unwrap();
154+
155+
// Both signals should be safely ignored
156+
signal::kill(Pid::this(), Signal::SIGUSR1).unwrap();
157+
signal::kill(Pid::this(), Signal::SIGUSR2).unwrap();
158+
}
159+
}

0 commit comments

Comments
 (0)