diff --git a/Cargo.lock b/Cargo.lock index 1a4c45ea9a..8bac9938ed 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4,11 +4,11 @@ version = 4 [[package]] name = "addr2line" -version = "0.24.2" +version = "0.25.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dfbe277e56a376000877090da837660b4427aad530e3028d44e0bffe4f89a1c1" +checksum = "1b5d307320b3181d6d7954e663bd7c774a838b8220fe0593c86d9fb09f498b4b" dependencies = [ - "gimli 0.31.1", + "gimli", ] [[package]] @@ -433,17 +433,17 @@ dependencies = [ [[package]] name = "backtrace" -version = "0.3.74" +version = "0.3.76" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8d82cb332cdfaed17ae235a638438ac4d4839913cc2af585c3c6746e8f8bee1a" +checksum = "bb531853791a215d7c62a30daf0dde835f381ab5de4589cfe7c649d2cbe92bd6" dependencies = [ "addr2line", "cfg-if", "libc", "miniz_oxide 0.8.8", - "object 0.36.5", + "object 0.37.3", "rustc-demangle", - "windows-targets 0.52.6", + "windows-link", ] [[package]] @@ -543,7 +543,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "48ceccc54b9c3e60e5f36b0498908c8c0f87387229cb0e0e5d65a074e00a8ba4" dependencies = [ "cpp_demangle 0.5.1", - "gimli 0.32.0", + "gimli", "libc", "memmap2", "miniz_oxide 0.9.0", @@ -2090,12 +2090,6 @@ dependencies = [ "wasi 0.14.2+wasi-0.2.4", ] -[[package]] -name = "gimli" -version = "0.31.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "07e28edb80900c19c28f1072f2e8aeca7fa06b23cd4169cefe1af5aa3260783f" - [[package]] name = "gimli" version = "0.32.0" @@ -2922,6 +2916,7 @@ name = "libdd-crashtracker" version = "1.0.0" dependencies = [ "anyhow", + "backtrace", "blazesym", "cc", "chrono", @@ -3936,9 +3931,9 @@ dependencies = [ [[package]] name = "object" -version = "0.36.5" +version = "0.37.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "aedf0a2d09c573ed1d8d85b30c119153926a2b36dce0ab28322c09a117a4683e" +checksum = "ff76201f031d8863c38aa7f905eca4f53abbfa15f609db4277d44cd8938f33fe" dependencies = [ "memchr", ] diff --git a/bin_tests/tests/crashtracker_bin_test.rs b/bin_tests/tests/crashtracker_bin_test.rs index 700c87f129..262150d628 100644 --- a/bin_tests/tests/crashtracker_bin_test.rs +++ b/bin_tests/tests/crashtracker_bin_test.rs @@ -646,8 +646,13 @@ fn test_crash_tracking_app(crash_type: &str) { let sig_info = &payload["sig_info"]; let error = &payload["error"]; + let kind = error["kind"] + .as_str() + .expect("error.kind should be a string"); + match crash_type_owned.as_str() { "panic" => { + assert_eq!(kind, "Panic", "Expected error kind 'Panic', got '{kind}'"); let message = error["message"].as_str().unwrap(); assert!( message.contains("Process panicked with message") && message.contains("program panicked"), @@ -656,6 +661,7 @@ fn test_crash_tracking_app(crash_type: &str) { ); } "segfault" => { + assert_eq!(kind, "UnixSignal", "Expected error kind 'UnixSignal', got '{kind}'"); assert_error_message(&error["message"], sig_info); } _ => unreachable!("Invalid crash type: {}", crash_type_owned), diff --git a/libdd-crashtracker/Cargo.toml b/libdd-crashtracker/Cargo.toml index 876b8c6de7..e9b0323cb7 100644 --- a/libdd-crashtracker/Cargo.toml +++ b/libdd-crashtracker/Cargo.toml @@ -68,6 +68,7 @@ symbolic-common = { version = "12.8.0", default-features = false } tokio = { version = "1.23", features = ["rt", "macros", "io-std", "io-util"] } uuid = { version = "1.4.1", features = ["v4", "serde"] } thiserror = "1.0" +backtrace = "0.3.76" [target.'cfg(windows)'.dependencies] windows = { version = "0.59.0", features = ["Win32_System_Diagnostics_Debug", "Win32_System_Diagnostics_ToolHelp", "Win32_System_ErrorReporting", "Win32_System_Kernel", "Win32_System_ProcessStatus", "Win32_System_Registry", "Win32_System_SystemInformation", "Win32_System_SystemServices", "Win32_System_Threading", "Win32_Security"] } diff --git a/libdd-crashtracker/src/collector/collector_manager.rs b/libdd-crashtracker/src/collector/collector_manager.rs index 3a3f134ea7..4e50542eec 100644 --- a/libdd-crashtracker/src/collector/collector_manager.rs +++ b/libdd-crashtracker/src/collector/collector_manager.rs @@ -7,7 +7,6 @@ use libdd_common::timeout::TimeoutManager; use super::emitters::{emit_crashreport, CrashKindData}; use crate::shared::configuration::CrashtrackerConfiguration; -use libc::{siginfo_t, ucontext_t}; use libdd_common::unix_utils::{alt_fork, terminate}; use nix::sys::signal::{self, SaFlags, SigAction, SigHandler, SigSet}; use std::os::unix::io::RawFd; @@ -30,9 +29,7 @@ impl Collector { config: &CrashtrackerConfiguration, config_str: &str, metadata_str: &str, - message_ptr: *mut String, - sig_info: *const siginfo_t, - ucontext: *const ucontext_t, + crash_kind: CrashKindData, ) -> Result { // When we spawn the child, our pid becomes the ppid. // SAFETY: This function has no safety requirements. @@ -49,9 +46,7 @@ impl Collector { config, config_str, metadata_str, - message_ptr, - sig_info, - ucontext, + crash_kind, receiver.handle.uds_fd, pid, tid, @@ -89,9 +84,7 @@ pub(crate) fn run_collector_child( config: &CrashtrackerConfiguration, config_str: &str, metadata_str: &str, - message_ptr: *mut String, - sig_info: *const siginfo_t, - ucontext: *const ucontext_t, + crash_kind: CrashKindData, uds_fd: RawFd, ppid: libc::pid_t, crashing_tid: libc::pid_t, @@ -117,8 +110,7 @@ pub(crate) fn run_collector_child( config, config_str, metadata_str, - message_ptr, - CrashKindData::UnixSignal { sig_info, ucontext }, + crash_kind, ppid, crashing_tid, ); diff --git a/libdd-crashtracker/src/collector/crash_handler.rs b/libdd-crashtracker/src/collector/crash_handler.rs index 2febb153d8..e2fe07496b 100644 --- a/libdd-crashtracker/src/collector/crash_handler.rs +++ b/libdd-crashtracker/src/collector/crash_handler.rs @@ -7,9 +7,10 @@ use super::collector_manager::Collector; use super::receiver_manager::Receiver; use super::saguard::{SaGuard, SuppressionMode}; use super::signal_handler_manager::chain_signal_handler; +use crate::collector::emitters::CrashKindData; use crate::crash_info::Metadata; use crate::shared::configuration::CrashtrackerConfiguration; -use crate::StackTrace; +use crate::{StackFrame, StackTrace}; use errno::{errno, set_errno}; use libc::{c_void, siginfo_t, ucontext_t}; use libdd_common::timeout::TimeoutManager; @@ -44,6 +45,7 @@ use std::sync::atomic::{AtomicBool, AtomicPtr, AtomicU64}; static METADATA: AtomicPtr<(Metadata, String)> = AtomicPtr::new(ptr::null_mut()); static CONFIG: AtomicPtr<(CrashtrackerConfiguration, String)> = AtomicPtr::new(ptr::null_mut()); static PANIC_MESSAGE: AtomicPtr = AtomicPtr::new(ptr::null_mut()); +static PANIC_CALLSTACK: AtomicPtr = AtomicPtr::new(ptr::null_mut()); type PanicHook = Box) + Send + Sync>; static PREVIOUS_PANIC_HOOK: AtomicPtr = AtomicPtr::new(ptr::null_mut()); @@ -102,6 +104,52 @@ fn format_message( } } +unsafe fn capture_panic_callstack() -> anyhow::Result<()> { + let mut stacktrace = StackTrace::new_incomplete(); + backtrace::trace_unsynchronized(|bt_frame| { + let mut frame = StackFrame::new(); + frame.with_ip(bt_frame.ip() as usize); + frame.with_symbol_address(bt_frame.symbol_address() as usize); + if let Some(module_base_address) = bt_frame.module_base_address() { + frame.with_module_base_address(module_base_address as usize); + } + frame.with_sp(bt_frame.sp() as usize); + let _ = stacktrace.push_frame(frame, true); + true + }); + + stacktrace.set_complete()?; + + let stacktrace_ptr = PANIC_CALLSTACK.swap(Box::into_raw(Box::new(stacktrace)), SeqCst); + // stacktrace_ptr should be null, but just in case. + if !stacktrace_ptr.is_null() { + unsafe { + std::mem::drop(Box::from_raw(stacktrace_ptr)); + } + } + Ok(()) +} + +fn capture_panic_message(panic_info: &PanicHookInfo<'_>) { + // Extract panic message from payload (supports &str and String) + let message = if let Some(&s) = panic_info.payload().downcast_ref::<&str>() { + format_message("message", s, panic_info.location()) + } else if let Some(s) = panic_info.payload().downcast_ref::() { + format_message("message", s.as_str(), panic_info.location()) + } else { + // For non-string types, use a generic message + format_message("unknown type", "", panic_info.location()) + }; + + let message_ptr = PANIC_MESSAGE.swap(Box::into_raw(Box::new(message)), SeqCst); + // message_ptr should be null, but just in case. + if !message_ptr.is_null() { + unsafe { + std::mem::drop(Box::from_raw(message_ptr)); + } + } +} + /// Register the panic hook. /// /// This function is used to register the panic hook and store the previous hook. @@ -122,24 +170,8 @@ pub fn register_panic_hook() -> anyhow::Result<()> { let old_hook_ptr = Box::into_raw(Box::new(old_hook)); PREVIOUS_PANIC_HOOK.swap(old_hook_ptr, SeqCst); panic::set_hook(Box::new(|panic_info| { - // Extract panic message from payload (supports &str and String) - let message = if let Some(&s) = panic_info.payload().downcast_ref::<&str>() { - format_message("message", s, panic_info.location()) - } else if let Some(s) = panic_info.payload().downcast_ref::() { - format_message("message", s.as_str(), panic_info.location()) - } else { - // For non-string types, use a generic message - format_message("unknown type", "", panic_info.location()) - }; - - // Store the message, cleaning up any old message - let message_ptr = PANIC_MESSAGE.swap(Box::into_raw(Box::new(message)), SeqCst); - // message_ptr should be null, but just in case. - if !message_ptr.is_null() { - unsafe { - std::mem::drop(Box::from_raw(message_ptr)); - } - } + let _ = unsafe { capture_panic_callstack() }; + capture_panic_message(panic_info); call_previous_panic_hook(panic_info); })); @@ -298,20 +330,22 @@ fn handle_posix_signal_impl( // The collector child process will handle converting this to a String after forking. // Leak of the message pointer is ok here. let message_ptr = PANIC_MESSAGE.swap(ptr::null_mut(), SeqCst); + let callstack_ptr = PANIC_CALLSTACK.swap(ptr::null_mut(), SeqCst); + + let crash_kind = if !message_ptr.is_null() { + CrashKindData::Panic { + stacktrace: callstack_ptr, + message: message_ptr, + } + } else { + CrashKindData::UnixSignal { sig_info, ucontext } + }; let timeout_manager = TimeoutManager::new(config.timeout()); let receiver = Receiver::from_crashtracker_config(config)?; - let collector = Collector::spawn( - &receiver, - config, - config_str, - metadata_string, - message_ptr, - sig_info, - ucontext, - )?; + let collector = Collector::spawn(&receiver, config, config_str, metadata_string, crash_kind)?; // We're done. Wrap up our interaction with the receiver. collector.finish(&timeout_manager); @@ -453,8 +487,10 @@ pub fn report_unhandled_exception( &config, &config_str, &metadata_str, - message_ptr, - super::emitters::CrashKindData::UnhandledException { stacktrace }, + super::emitters::CrashKindData::UnhandledException { + stacktrace, + message: message_ptr, + }, pid, tid, ); @@ -572,6 +608,77 @@ mod tests { } } + fn make_test_stacktrace() -> StackTrace { + let mut st = StackTrace::new_incomplete(); + let mut frame = StackFrame::new(); + frame.with_ip(0xdead); + frame.with_sp(0xbeef); + let _ = st.push_frame(frame, true); + st.set_complete().unwrap(); + st + } + + fn clear_panic_callstack() { + let ptr = PANIC_CALLSTACK.swap(ptr::null_mut(), SeqCst); + if !ptr.is_null() { + unsafe { drop(Box::from_raw(ptr)) }; + } + } + + #[test] + fn test_panic_callstack_storage_and_retrieval() { + clear_panic_callstack(); + let test_stacktrace = make_test_stacktrace(); + let stacktrace_ptr = Box::into_raw(Box::new(test_stacktrace.clone())); + + let old_ptr = PANIC_CALLSTACK.swap(stacktrace_ptr, SeqCst); + assert!(old_ptr.is_null()); + + let retrieved_ptr = PANIC_CALLSTACK.swap(ptr::null_mut(), SeqCst); + assert!(!retrieved_ptr.is_null()); + + unsafe { + let retrieved_stacktrace = *Box::from_raw(retrieved_ptr); + assert_eq!(retrieved_stacktrace, test_stacktrace); + } + } + + #[test] + fn test_panic_callstack_null_handling() { + clear_panic_callstack(); + + let callstack_ptr = PANIC_CALLSTACK.load(SeqCst); + assert!(callstack_ptr.is_null()); + + let old_ptr = PANIC_CALLSTACK.swap(ptr::null_mut(), SeqCst); + assert!(old_ptr.is_null()); + } + + #[test] + fn test_panic_callstack_replacement() { + clear_panic_callstack(); + let stacktrace1 = make_test_stacktrace(); + let mut stacktrace2 = make_test_stacktrace(); + let mut extra_frame = StackFrame::new(); + extra_frame.with_ip(0xcafe); + stacktrace2.frames.push(extra_frame); + + let ptr1 = Box::into_raw(Box::new(stacktrace1)); + let ptr2 = Box::into_raw(Box::new(stacktrace2.clone())); + + PANIC_CALLSTACK.store(ptr1, SeqCst); + let old_ptr = PANIC_CALLSTACK.swap(ptr2, SeqCst); + + assert_eq!(old_ptr, ptr1); + + unsafe { + drop(Box::from_raw(old_ptr)); + let final_ptr = PANIC_CALLSTACK.swap(ptr::null_mut(), SeqCst); + let final_stacktrace = *Box::from_raw(final_ptr); + assert_eq!(final_stacktrace, stacktrace2); + } + } + #[test] fn test_metadata_update_atomic() { // Test that metadata updates are atomic diff --git a/libdd-crashtracker/src/collector/emitters.rs b/libdd-crashtracker/src/collector/emitters.rs index 36bead6d6f..302d2ca3c4 100644 --- a/libdd-crashtracker/src/collector/emitters.rs +++ b/libdd-crashtracker/src/collector/emitters.rs @@ -50,6 +50,11 @@ pub(crate) enum CrashKindData { }, UnhandledException { stacktrace: StackTrace, + message: *mut String, + }, + Panic { + stacktrace: *mut StackTrace, + message: *mut String, }, } @@ -58,6 +63,7 @@ impl CrashKindData { match self { CrashKindData::UnixSignal { .. } => ErrorKind::UnixSignal, CrashKindData::UnhandledException { .. } => ErrorKind::UnhandledException, + CrashKindData::Panic { .. } => ErrorKind::Panic, } } } @@ -68,7 +74,6 @@ pub(crate) fn emit_crashreport( config: &CrashtrackerConfiguration, config_str: &str, metadata_string: &str, - message_ptr: *mut String, crash: CrashKindData, ppid: i32, crashing_tid: libc::pid_t, @@ -78,14 +83,22 @@ pub(crate) fn emit_crashreport( // section, so try to emit message, siginfo, and kind before it to make sure // we have an enhanced crash ping message emit_config(pipe, config_str)?; - emit_message(pipe, message_ptr)?; match &crash { CrashKindData::UnixSignal { sig_info, .. } => { emit_siginfo(pipe, *sig_info)?; } - CrashKindData::UnhandledException { .. } => { - // Unhandled exceptions have no signal info + CrashKindData::UnhandledException { + message, + stacktrace: _, + } => { + emit_message(pipe, *message)?; + } + CrashKindData::Panic { + message, + stacktrace: _, + } => { + emit_message(pipe, *message)?; } } @@ -117,11 +130,14 @@ pub(crate) fn emit_crashreport( emit_runtime_stack(pipe)?; } } - CrashKindData::UnhandledException { stacktrace } => { + CrashKindData::UnhandledException { stacktrace, .. } => { // SAFETY: This branch only executes for unhandled exceptions, never // from a signal handler unsafe { emit_whole_stacktrace(pipe, stacktrace)? }; } + CrashKindData::Panic { stacktrace, .. } => { + unsafe { emit_whole_stacktrace(pipe, (*stacktrace).clone())? }; + } } writeln!(pipe, "{DD_CRASHTRACK_DONE}")?;