diff --git a/Cargo.lock b/Cargo.lock index f48205594a..c5866a0c68 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -367,6 +367,7 @@ dependencies = [ "nix 0.29.0", "once_cell", "os_info", + "regex", "serde_json", "serial_test", "strum", diff --git a/bin_tests/Cargo.toml b/bin_tests/Cargo.toml index 86b745883b..7e5e9c3c45 100644 --- a/bin_tests/Cargo.toml +++ b/bin_tests/Cargo.toml @@ -23,6 +23,7 @@ libc = "0.2" nix = { version = "0.29", features = ["signal", "socket"] } hex = "0.4" os_info = "3.7.0" +regex = "1.0" [dev-dependencies] serial_test = "3.2" diff --git a/bin_tests/src/bin/crashing_test_app.rs b/bin_tests/src/bin/crashing_test_app.rs index 42802a16c4..6789d51bc6 100644 --- a/bin_tests/src/bin/crashing_test_app.rs +++ b/bin_tests/src/bin/crashing_test_app.rs @@ -14,6 +14,8 @@ mod unix { use anyhow::ensure; use anyhow::Context; use std::env; + use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::Arc; use std::time::Duration; use libdd_common::{tag, Endpoint}; @@ -23,8 +25,25 @@ mod unix { const TEST_COLLECTOR_TIMEOUT: Duration = Duration::from_secs(10); - #[inline(never)] - unsafe fn fn3() { + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + enum CrashType { + Segfault, + Panic, + } + + impl std::str::FromStr for CrashType { + type Err = anyhow::Error; + fn from_str(s: &str) -> Result { + match s { + "segfault" => Ok(CrashType::Segfault), + "panic" => Ok(CrashType::Panic), + _ => anyhow::bail!("Invalid crash type: {s}"), + } + } + } + + #[inline(always)] + unsafe fn cause_segfault() { #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] { std::arch::asm!("mov eax, [0]", options(nostack)); @@ -37,13 +56,25 @@ mod unix { } #[inline(never)] - fn fn2() { - unsafe { fn3() } + fn fn3(crash_type: CrashType) { + match crash_type { + CrashType::Segfault => { + unsafe { cause_segfault() }; + } + CrashType::Panic => { + panic!("program panicked"); + } + } } #[inline(never)] - fn fn1() { - fn2() + fn fn2(crash_type: CrashType) { + fn3(crash_type); + } + + #[inline(never)] + fn fn1(crash_type: CrashType) { + fn2(crash_type); } #[inline(never)] @@ -53,6 +84,7 @@ mod unix { let output_url = args.next().context("Unexpected number of arguments 1")?; let receiver_binary = args.next().context("Unexpected number of arguments 2")?; let output_dir = args.next().context("Unexpected number of arguments 3")?; + let crash_type = args.next().context("Unexpected number of arguments 4")?; anyhow::ensure!(args.next().is_none(), "unexpected extra arguments"); let stderr_filename = format!("{output_dir}/out.stderr"); @@ -88,6 +120,19 @@ mod unix { .collect(), }; + let crash_type = crash_type.parse().context("Invalid crash type")?; + let is_panic_mode = matches!(crash_type, CrashType::Panic); + + let called_panic_hook = Arc::new(AtomicBool::new(false)); + let old_hook = std::panic::take_hook(); + if is_panic_mode { + let called_panic_hook_clone = Arc::clone(&called_panic_hook); + std::panic::set_hook(Box::new(move |panic_info| { + called_panic_hook_clone.store(true, Ordering::SeqCst); + old_hook(panic_info); + })); + } + crashtracker::init( config, CrashtrackerReceiverConfig::new( @@ -100,7 +145,13 @@ mod unix { metadata, )?; - fn1(); + fn1(crash_type); + + // If the panic hook was chained, it should have been called. + anyhow::ensure!( + !is_panic_mode || called_panic_hook.load(Ordering::SeqCst), + "panic hook was not called" + ); Ok(()) } } diff --git a/bin_tests/src/lib.rs b/bin_tests/src/lib.rs index 4aec1f6ef0..5c525782c0 100644 --- a/bin_tests/src/lib.rs +++ b/bin_tests/src/lib.rs @@ -46,6 +46,7 @@ pub struct ArtifactsBuild { pub artifact_type: ArtifactType, pub build_profile: BuildProfile, pub triple_target: Option, + pub panic_abort: Option, } fn inner_build_artifact(c: &ArtifactsBuild) -> anyhow::Result { @@ -58,6 +59,19 @@ fn inner_build_artifact(c: &ArtifactsBuild) -> anyhow::Result { ArtifactType::ExecutablePackage | ArtifactType::CDylib => build_cmd.arg("-p"), ArtifactType::Bin => build_cmd.arg("--bin"), }; + + if let Some(panic_abort) = c.panic_abort { + if panic_abort { + let existing_rustflags = std::env::var("RUSTFLAGS").unwrap_or_default(); + let new_rustflags = if existing_rustflags.is_empty() { + "-C panic=abort".to_string() + } else { + format!("{} -C panic=abort", existing_rustflags) + }; + build_cmd.env("RUSTFLAGS", new_rustflags); + } + } + build_cmd.arg(&c.name); let output = build_cmd.output().unwrap(); diff --git a/bin_tests/src/modes/behavior.rs b/bin_tests/src/modes/behavior.rs index ce66270643..c5bdc51102 100644 --- a/bin_tests/src/modes/behavior.rs +++ b/bin_tests/src/modes/behavior.rs @@ -134,6 +134,9 @@ pub fn get_behavior(mode_str: &str) -> Box { "runtime_callback_frame_invalid_utf8" => { Box::new(test_012_runtime_callback_frame_invalid_utf8::Test) } + "panic_hook_after_fork" => Box::new(test_013_panic_hook_after_fork::Test), + "panic_hook_string" => Box::new(test_014_panic_hook_string::Test), + "panic_hook_unknown_type" => Box::new(test_015_panic_hook_unknown_type::Test), _ => panic!("Unknown mode: {mode_str}"), } } diff --git a/bin_tests/src/modes/unix/mod.rs b/bin_tests/src/modes/unix/mod.rs index 394d693877..2b8c2b0f2d 100644 --- a/bin_tests/src/modes/unix/mod.rs +++ b/bin_tests/src/modes/unix/mod.rs @@ -13,3 +13,6 @@ pub mod test_009_prechain_with_abort; pub mod test_010_runtime_callback_frame; pub mod test_011_runtime_callback_string; pub mod test_012_runtime_callback_frame_invalid_utf8; +pub mod test_013_panic_hook_after_fork; +pub mod test_014_panic_hook_string; +pub mod test_015_panic_hook_unknown_type; diff --git a/bin_tests/src/modes/unix/test_013_panic_hook_after_fork.rs b/bin_tests/src/modes/unix/test_013_panic_hook_after_fork.rs new file mode 100644 index 0000000000..e0366b838a --- /dev/null +++ b/bin_tests/src/modes/unix/test_013_panic_hook_after_fork.rs @@ -0,0 +1,120 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 +// +// Test that panic hooks registered before fork() continue to work in child processes. +// This validates that: +// 1. The panic hook survives fork() +// 2. The panic message is captured in the child process +// 3. The crash report is correctly generated +use crate::modes::behavior::Behavior; +use libdd_crashtracker::{self as crashtracker, CrashtrackerConfiguration}; +use nix::sys::wait::{waitpid, WaitStatus}; +use nix::unistd::Pid; +use std::fs; +use std::path::Path; +use std::time::{Duration, Instant}; + +pub struct Test; + +impl Behavior for Test { + fn setup( + &self, + _output_dir: &Path, + _config: &mut CrashtrackerConfiguration, + ) -> anyhow::Result<()> { + Ok(()) + } + + fn pre(&self, output_dir: &Path) -> anyhow::Result<()> { + pre(output_dir) + } + + fn post(&self, output_dir: &Path) -> anyhow::Result<()> { + post(output_dir) + } +} + +fn pre(output_dir: &Path) -> anyhow::Result<()> { + let old_hook = std::panic::take_hook(); + let output_dir = output_dir.to_path_buf(); + + // Set up a panic hook BEFORE crashtracker::init to verify the hook chain works + std::panic::set_hook(Box::new(move |panic_info| { + // Mark that our custom hook was called by writing a marker file + // This works across fork() because it's persistent storage + let marker_path = output_dir.join("panic_hook_called.marker"); + let _ = fs::write(marker_path, "hook was called"); + + // Call the previous hook (usually the default panic hook) + old_hook(panic_info); + })); + + Ok(()) +} + +fn post(output_dir: &Path) -> anyhow::Result<()> { + match unsafe { libc::fork() } { + -1 => { + anyhow::bail!("Failed to fork"); + } + 0 => { + // Child - panic with a specific message + // The crashtracker should capture both the panic hook execution + // and the panic message + crashtracker::begin_op(crashtracker::OpTypes::ProfilerCollectingSample)?; + + // Give parent time to set up wait + std::thread::sleep(Duration::from_millis(10)); + + panic!("child panicked after fork - hook should fire"); + } + pid => { + // Parent - wait for child to panic and crash + let start_time = Instant::now(); + let max_wait = Duration::from_secs(5); + + loop { + match waitpid(Pid::from_raw(pid), None)? { + WaitStatus::StillAlive => { + if start_time.elapsed() > max_wait { + anyhow::bail!("Child process did not exit within 5 seconds"); + } + std::thread::sleep(Duration::from_millis(10)); + } + WaitStatus::Exited(_pid, exit_code) => { + // Child exited - this is what we expect after panic + eprintln!("Child exited with code: {}", exit_code); + break; + } + WaitStatus::Signaled(_pid, signal, _) => { + // Child was killed by signal (also acceptable for panic) + eprintln!("Child killed by signal: {:?}", signal); + break; + } + _ => { + // Other status - continue waiting + } + } + } + + // Verify that our custom panic hook was called by checking for the marker file + // This proves that the hook chain works correctly: + // crashtracker's hook -> our custom hook -> default hook + let marker_path = output_dir.join("panic_hook_called.marker"); + + if !marker_path.exists() { + anyhow::bail!( + "Custom panic hook was not called - hook chaining failed! \ + Expected marker file at: {}", + marker_path.display() + ); + } + + // Parent exits with error code to indicate test completion + // The test harness will verify the crash report contains the panic message + unsafe { + libc::_exit(1); + } + } + } +} diff --git a/bin_tests/src/modes/unix/test_014_panic_hook_string.rs b/bin_tests/src/modes/unix/test_014_panic_hook_string.rs new file mode 100644 index 0000000000..f0b34bbc24 --- /dev/null +++ b/bin_tests/src/modes/unix/test_014_panic_hook_string.rs @@ -0,0 +1,31 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 +// +// Test that panic hooks work correctly with String payloads (not just &str). +// This validates that: +// 1. String panic payloads are correctly captured +// 2. The panic message format is "Process panicked with message: " +use crate::modes::behavior::Behavior; +use libdd_crashtracker::CrashtrackerConfiguration; +use std::path::Path; + +pub struct Test; + +impl Behavior for Test { + fn setup( + &self, + _output_dir: &Path, + _config: &mut CrashtrackerConfiguration, + ) -> anyhow::Result<()> { + Ok(()) + } + + fn pre(&self, _output_dir: &Path) -> anyhow::Result<()> { + Ok(()) + } + + fn post(&self, _output_dir: &Path) -> anyhow::Result<()> { + let dynamic_value = 42; + panic!("Panic with value: {}", dynamic_value); + } +} diff --git a/bin_tests/src/modes/unix/test_015_panic_hook_unknown_type.rs b/bin_tests/src/modes/unix/test_015_panic_hook_unknown_type.rs new file mode 100644 index 0000000000..74d8504ca5 --- /dev/null +++ b/bin_tests/src/modes/unix/test_015_panic_hook_unknown_type.rs @@ -0,0 +1,30 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 +// +// Test that panic hooks work correctly with unknown types via panic_any. +// This validates that: +// 1. panic_any() with non-string types is handled gracefully +// 2. The message format is: "Process panicked with unknown type ()" +use crate::modes::behavior::Behavior; +use libdd_crashtracker::CrashtrackerConfiguration; +use std::path::Path; + +pub struct Test; + +impl Behavior for Test { + fn setup( + &self, + _output_dir: &Path, + _config: &mut CrashtrackerConfiguration, + ) -> anyhow::Result<()> { + Ok(()) + } + + fn pre(&self, _output_dir: &Path) -> anyhow::Result<()> { + Ok(()) + } + + fn post(&self, _output_dir: &Path) -> anyhow::Result<()> { + std::panic::panic_any(42i32); + } +} diff --git a/bin_tests/src/test_runner.rs b/bin_tests/src/test_runner.rs index 8a179551c1..1f20dcb53b 100644 --- a/bin_tests/src/test_runner.rs +++ b/bin_tests/src/test_runner.rs @@ -190,7 +190,9 @@ where /// This is more flexible than `run_crash_test_with_artifacts` and allows for: /// - Custom binary selection (e.g., crashing_test_app instead of crashtracker_bin_test) /// - Custom command arguments -/// - Custom exit status expectations +/// +/// Note: This function always expects the test to crash (exit with non-success status). +/// All current uses of this function test crash scenarios, not successful exits. /// /// # Example /// ```no_run @@ -223,7 +225,6 @@ where /// .arg(&artifacts_map[&receiver]) /// .arg(&fixtures.output_dir); /// }, -/// false, // expect crash (not success) /// |payload, _fixtures| { /// // Custom validation /// Ok(()) @@ -235,7 +236,6 @@ where pub fn run_custom_crash_test( binary_path: &std::path::Path, command_builder: CB, - expect_success: bool, validator: V, ) -> Result<()> where @@ -251,13 +251,10 @@ where let exit_status = crate::timeit!("exit after signal", { p.wait()? }); - // Validate exit status - let actual_success = exit_status.success(); + // Validate exit status - custom crash tests always expect failure anyhow::ensure!( - expect_success == actual_success, - "Exit status mismatch: expected success={}, got success={} (exit code: {:?})", - expect_success, - actual_success, + !exit_status.success(), + "Expected test to crash (non-success exit), but it succeeded with code: {:?}", exit_status.code() ); diff --git a/bin_tests/tests/crashtracker_bin_test.rs b/bin_tests/tests/crashtracker_bin_test.rs index 5c70bdb5a9..2c1da53c1c 100644 --- a/bin_tests/tests/crashtracker_bin_test.rs +++ b/bin_tests/tests/crashtracker_bin_test.rs @@ -291,6 +291,163 @@ fn test_crash_tracking_errors_intake_uds_socket() { ); } +/// For some reason, the next two tests fail on MacOS, because the callstack cannot be collected. +/// We get this error: +/// thread 'test_crash_tracking_bin_segfault' (88268) panicked at +/// bin_tests/tests/crashtracker_bin_test.rs:250:5: got Ok("Unable to process line: +/// DD_CRASHTRACK_END_STACKTRACE. Error: Can't set non-existant stack complete\n") +#[test] +#[cfg_attr(miri, ignore)] +#[cfg(not(target_os = "macos"))] +fn test_crash_tracking_bin_panic() { + test_crash_tracking_app("panic"); +} + +#[test] +#[cfg(not(target_os = "macos"))] +#[cfg_attr(miri, ignore)] +fn test_crash_tracking_bin_segfault() { + test_crash_tracking_app("segfault"); +} + +fn test_crash_tracking_app(crash_type: &str) { + use bin_tests::test_runner::run_custom_crash_test; + + // Set up custom artifacts: receiver + crashing_test_app with panic_abort + let crashtracker_receiver = create_crashtracker_receiver(BuildProfile::Release); + let crashing_app = create_crashing_app(BuildProfile::Debug, true); + + let artifacts_map = build_artifacts(&[&crashtracker_receiver, &crashing_app]).unwrap(); + + // Create validator based on crash type + let crash_type_owned = crash_type.to_owned(); + let validator: ValidatorFn = Box::new(move |payload, _fixtures| { + let sig_info = &payload["sig_info"]; + let error = &payload["error"]; + + match crash_type_owned.as_str() { + "panic" => { + let message = error["message"].as_str().unwrap(); + assert!( + message.contains("Process panicked with message") && message.contains("program panicked"), + "Expected panic message to contain 'Process panicked with message' and 'program panicked', got: {}", + message + ); + } + "segfault" => { + assert_error_message(&error["message"], sig_info); + } + _ => unreachable!("Invalid crash type: {}", crash_type_owned), + } + + Ok(()) + }); + + run_custom_crash_test( + &artifacts_map[&crashing_app], + |cmd, fixtures| { + cmd.arg(format!("file://{}", fixtures.crash_profile_path.display())) + .arg(&artifacts_map[&crashtracker_receiver]) + .arg(&fixtures.output_dir) + .arg(crash_type); + }, + validator, + ) + .unwrap(); +} + +#[test] +#[cfg_attr(miri, ignore)] +#[cfg(not(target_os = "macos"))] // Same restriction as other panic tests +fn test_crash_tracking_bin_panic_hook_after_fork() { + test_panic_hook_mode( + "panic_hook_after_fork", + "message", + Some("child panicked after fork"), + ); +} + +#[test] +#[cfg_attr(miri, ignore)] +#[cfg(not(target_os = "macos"))] // Same restriction as other panic tests +fn test_crash_tracking_bin_panic_hook_string() { + test_panic_hook_mode("panic_hook_string", "message", Some("Panic with value: 42")); +} + +#[test] +#[cfg_attr(miri, ignore)] +#[cfg(not(target_os = "macos"))] // Same restriction as other panic tests +fn test_crash_tracking_bin_panic_hook_unknown_type() { + test_panic_hook_mode( + "panic_hook_unknown_type", + "unknown type", + None, // no panic message for unknown type + ); +} + +/// Helper function to run panic hook tests with different payload types. +/// Note: Since tests are built with Debug profile, location is always expected. +fn test_panic_hook_mode(mode: &str, expected_category: &str, expected_panic_message: Option<&str>) { + use bin_tests::test_runner::run_custom_crash_test; + + // Set up custom artifacts: receiver + crashtracker_bin_test + let crashtracker_receiver = create_crashtracker_receiver(BuildProfile::Release); + let crashtracker_bin_test = create_crashtracker_bin_test(BuildProfile::Debug, true); + + let artifacts_map = build_artifacts(&[&crashtracker_receiver, &crashtracker_bin_test]).unwrap(); + + let expected_category = expected_category.to_owned(); + let expected_panic_message = expected_panic_message.map(|s| s.to_owned()); + let validator: ValidatorFn = Box::new(move |payload, _fixtures| { + // Verify the panic message is captured + let error = &payload["error"]; + let message = error["message"].as_str().unwrap(); + + // Check the message starts with "Process panicked with " + let expected_prefix = format!("Process panicked with {}", expected_category); + assert!( + message.starts_with(&expected_prefix), + "Expected panic message to start with '{}', got: {}", + expected_prefix, + message + ); + + // Check the panic message if expected (the message passed to panic! macro) + if let Some(ref panic_msg) = expected_panic_message { + assert!( + message.contains(panic_msg), + "Expected panic message to contain '{}', got: {}", + panic_msg, + message + ); + } + + // Check for location format (file:line:column) - always present in Debug builds + // Location should end with pattern like " (path/file.rs:123:45)" + let location_regex = regex::Regex::new(r" \(.+?:\d+:\d+\)$").unwrap(); + assert!( + location_regex.is_match(message), + "Expected panic message to end with location ' (file:line:column)', got: {}", + message + ); + + Ok(()) + }); + + run_custom_crash_test( + &artifacts_map[&crashtracker_bin_test], + |cmd, fixtures| { + cmd.arg(format!("file://{}", fixtures.crash_profile_path.display())) + .arg(&artifacts_map[&crashtracker_receiver]) + .arg(&fixtures.output_dir) + .arg(mode) + .arg("donothing"); // crash method (not used in panic hook tests) + }, + validator, + ) + .unwrap(); +} + // ==================================================================================== // CALLSTACK VALIDATION TESTS - MIGRATED TO CUSTOM TEST RUNNER // ==================================================================================== @@ -310,22 +467,9 @@ fn test_crash_tracking_callstack() { use bin_tests::test_runner::run_custom_crash_test; // Set up custom artifacts: receiver + crashing_test_app (in Debug mode) - let crashtracker_receiver = ArtifactsBuild { - name: "test_crashtracker_receiver".to_owned(), - build_profile: BuildProfile::Release, - artifact_type: ArtifactType::Bin, - triple_target: None, - ..Default::default() - }; - - let crashing_app = ArtifactsBuild { - name: "crashing_test_app".to_owned(), - // compile in debug so we avoid inlining and can check the callchain - build_profile: BuildProfile::Debug, - artifact_type: ArtifactType::Bin, - triple_target: None, - ..Default::default() - }; + let crashtracker_receiver = create_crashtracker_receiver(BuildProfile::Release); + // compile in debug so we avoid inlining and can check the callchain + let crashing_app = create_crashing_app(BuildProfile::Debug, false); let artifacts_map = build_artifacts(&[&crashtracker_receiver, &crashing_app]).unwrap(); @@ -344,9 +488,9 @@ fn test_crash_tracking_callstack() { |cmd, fixtures| { cmd.arg(format!("file://{}", fixtures.crash_profile_path.display())) .arg(&artifacts_map[&crashtracker_receiver]) - .arg(&fixtures.output_dir); + .arg(&fixtures.output_dir) + .arg("segfault"); }, - false, // expect crash (not success) |payload, _fixtures| { // Use the new callstack validator PayloadValidator::new(payload).validate_callstack_functions(&expected_functions)?; @@ -1166,21 +1310,43 @@ fn setup_test_fixtures<'a>(crates: &[&'a ArtifactsBuild]) -> TestFixtures<'a> { fn setup_crashtracking_crates( crash_tracking_receiver_profile: BuildProfile, ) -> (ArtifactsBuild, ArtifactsBuild) { - let crashtracker_bin = ArtifactsBuild { - name: "crashtracker_bin_test".to_owned(), - build_profile: crash_tracking_receiver_profile, + let crashtracker_bin = create_crashtracker_bin_test(crash_tracking_receiver_profile, false); + let crashtracker_receiver = create_crashtracker_receiver(crash_tracking_receiver_profile); + (crashtracker_bin, crashtracker_receiver) +} + +// Helper functions for creating common artifact configurations + +fn create_crashtracker_receiver(profile: BuildProfile) -> ArtifactsBuild { + ArtifactsBuild { + name: "test_crashtracker_receiver".to_owned(), + build_profile: profile, artifact_type: ArtifactType::Bin, triple_target: None, ..Default::default() - }; - let crashtracker_receiver = ArtifactsBuild { - name: "test_crashtracker_receiver".to_owned(), - build_profile: crash_tracking_receiver_profile, + } +} + +fn create_crashing_app(profile: BuildProfile, panic_abort: bool) -> ArtifactsBuild { + ArtifactsBuild { + name: "crashing_test_app".to_owned(), + build_profile: profile, artifact_type: ArtifactType::Bin, triple_target: None, + panic_abort: if panic_abort { Some(true) } else { None }, ..Default::default() - }; - (crashtracker_bin, crashtracker_receiver) + } +} + +fn create_crashtracker_bin_test(profile: BuildProfile, panic_abort: bool) -> ArtifactsBuild { + ArtifactsBuild { + name: "crashtracker_bin_test".to_owned(), + build_profile: profile, + artifact_type: ArtifactType::Bin, + triple_target: None, + panic_abort: if panic_abort { Some(true) } else { None }, + ..Default::default() + } } #[cfg(unix)] diff --git a/bin_tests/tests/test_the_tests.rs b/bin_tests/tests/test_the_tests.rs index 9495387f52..afb3933acc 100644 --- a/bin_tests/tests/test_the_tests.rs +++ b/bin_tests/tests/test_the_tests.rs @@ -34,6 +34,7 @@ fn test_the_tests_inner(profile: BuildProfile) { build_profile: profile, artifact_type: ArtifactType::CDylib, triple_target: None, + panic_abort: None, }, &test_the_tests, ]; diff --git a/libdd-crashtracker/src/collector/api.rs b/libdd-crashtracker/src/collector/api.rs index a934c05d41..91e1e78a10 100644 --- a/libdd-crashtracker/src/collector/api.rs +++ b/libdd-crashtracker/src/collector/api.rs @@ -4,9 +4,10 @@ use super::{crash_handler::enable, receiver_manager::Receiver}; use crate::{ - clear_spans, clear_traces, collector::signal_handler_manager::register_crash_handlers, - crash_info::Metadata, reset_counters, shared::configuration::CrashtrackerReceiverConfig, - update_config, update_metadata, CrashtrackerConfiguration, + clear_spans, clear_traces, collector::crash_handler::register_panic_hook, + collector::signal_handler_manager::register_crash_handlers, crash_info::Metadata, + reset_counters, shared::configuration::CrashtrackerReceiverConfig, update_config, + update_metadata, CrashtrackerConfiguration, }; pub static DEFAULT_SYMBOLS: [libc::c_int; 4] = @@ -43,6 +44,8 @@ pub fn on_fork( // The altstack (if any) is similarly unaffected by fork: // https://man7.org/linux/man-pages/man2/sigaltstack.2.html + // panic hook is unaffected by fork. + update_metadata(metadata)?; update_config(config)?; Receiver::update_stored_config(receiver_config)?; @@ -68,6 +71,7 @@ pub fn init( update_config(config.clone())?; Receiver::update_stored_config(receiver_config)?; register_crash_handlers(&config)?; + register_panic_hook()?; enable(); Ok(()) } diff --git a/libdd-crashtracker/src/collector/collector_manager.rs b/libdd-crashtracker/src/collector/collector_manager.rs index 3c818b0154..a3e3fb1e65 100644 --- a/libdd-crashtracker/src/collector/collector_manager.rs +++ b/libdd-crashtracker/src/collector/collector_manager.rs @@ -30,6 +30,7 @@ impl Collector { config: &CrashtrackerConfiguration, config_str: &str, metadata_str: &str, + message_ptr: *mut String, sig_info: *const siginfo_t, ucontext: *const ucontext_t, ) -> Result { @@ -45,6 +46,7 @@ impl Collector { config, config_str, metadata_str, + message_ptr, sig_info, ucontext, receiver.handle.uds_fd, @@ -66,10 +68,12 @@ impl Collector { } } +#[allow(clippy::too_many_arguments)] 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, uds_fd: RawFd, @@ -96,6 +100,7 @@ pub(crate) fn run_collector_child( config, config_str, metadata_str, + message_ptr, sig_info, ucontext, ppid, diff --git a/libdd-crashtracker/src/collector/crash_handler.rs b/libdd-crashtracker/src/collector/crash_handler.rs index eaaf188abb..413b78224e 100644 --- a/libdd-crashtracker/src/collector/crash_handler.rs +++ b/libdd-crashtracker/src/collector/crash_handler.rs @@ -10,6 +10,8 @@ use crate::crash_info::Metadata; use crate::shared::configuration::CrashtrackerConfiguration; use libc::{c_void, siginfo_t, ucontext_t}; use libdd_common::timeout::TimeoutManager; +use std::panic; +use std::panic::PanicHookInfo; use std::ptr; use std::sync::atomic::Ordering::SeqCst; use std::sync::atomic::{AtomicBool, AtomicPtr, AtomicU64}; @@ -35,6 +37,10 @@ use std::sync::atomic::{AtomicBool, AtomicPtr, AtomicU64}; // `Box::from_raw` to recreate the box, then dropping it. 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()); + +type PanicHook = Box) + Send + Sync>; +static PREVIOUS_PANIC_HOOK: AtomicPtr = AtomicPtr::new(ptr::null_mut()); #[derive(Debug, thiserror::Error)] pub enum CrashHandlerError { @@ -72,6 +78,88 @@ pub fn update_metadata(metadata: Metadata) -> anyhow::Result<()> { Ok(()) } +/// Format a panic message with optional location information. +fn format_message( + category: &str, + panic_message: &str, + location: Option<&panic::Location>, +) -> String { + let base = if panic_message.is_empty() { + format!("Process panicked with {}", category) + } else { + format!("Process panicked with {} \"{}\"", category, panic_message) + }; + + match location { + Some(loc) => format!("{} ({}:{}:{})", base, loc.file(), loc.line(), loc.column()), + None => base, + } +} + +/// Register the panic hook. +/// +/// This function is used to register the panic hook and store the previous hook. +/// PRECONDITIONS: +/// None +/// SAFETY: +/// Crash-tracking functions are not guaranteed to be reentrant. +/// No other crash-handler functions should be called concurrently. +/// ATOMICITY: +/// This function uses a swap on an atomic pointer. +pub fn register_panic_hook() -> anyhow::Result<()> { + // register only once, if it is already registered, do nothing + if !PREVIOUS_PANIC_HOOK.load(SeqCst).is_null() { + return Ok(()); + } + + let old_hook = panic::take_hook(); + 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)); + } + } + + call_previous_panic_hook(panic_info); + })); + Ok(()) +} + +/// Call the previous panic hook. +/// +/// This function is used to call the previous panic hook. +/// PRECONDITIONS: +/// None +/// SAFETY: +/// Crash-tracking functions are not guaranteed to be reentrant. +/// No other crash-handler functions should be called concurrently. +fn call_previous_panic_hook(panic_info: &PanicHookInfo<'_>) { + let old_hook_ptr = PREVIOUS_PANIC_HOOK.load(SeqCst); + if !old_hook_ptr.is_null() { + // Safety: This pointer can only come from Box::into_raw above in register_panic_hook. + // We borrow it here without taking ownership so it remains valid for future calls. + unsafe { + let old_hook = &*old_hook_ptr; + old_hook(panic_info); + } + } +} + /// Updates the crashtracker config for this process /// Config is stored in a global variable and sent to the crashtracking /// receiver when a crash occurs. @@ -172,6 +260,11 @@ fn handle_posix_signal_impl( } let (_metadata, metadata_string) = unsafe { &*metadata_ptr }; + // Get the panic message pointer but don't dereference or deallocate in signal handler. + // 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 timeout_manager = TimeoutManager::new(config.timeout()); // Optionally, create the receiver. This all hinges on whether or not the configuration has a @@ -190,6 +283,7 @@ fn handle_posix_signal_impl( config, config_str, metadata_string, + message_ptr, sig_info, ucontext, )?; @@ -200,3 +294,152 @@ fn handle_posix_signal_impl( Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_register_panic_hook() { + assert!(PREVIOUS_PANIC_HOOK.load(SeqCst).is_null()); + + let result = register_panic_hook(); + assert!(result.is_ok()); + + assert!(!PREVIOUS_PANIC_HOOK.load(SeqCst).is_null()); + } + + #[test] + fn test_panic_message_storage_and_retrieval() { + // Test that panic messages can be stored and retrieved via atomic pointer + let test_message = "test panic message".to_string(); + let message_ptr = Box::into_raw(Box::new(test_message.clone())); + + // Store the message + let old_ptr = PANIC_MESSAGE.swap(message_ptr, SeqCst); + assert!(old_ptr.is_null()); // Should be null initially + + // Retrieve and verify + let retrieved_ptr = PANIC_MESSAGE.swap(ptr::null_mut(), SeqCst); + assert!(!retrieved_ptr.is_null()); + + unsafe { + let retrieved_message = *Box::from_raw(retrieved_ptr); + assert_eq!(retrieved_message, test_message); + } + } + + #[test] + fn test_panic_message_null_handling() { + // Test that null message pointers are handled correctly + PANIC_MESSAGE.store(ptr::null_mut(), SeqCst); + + let message_ptr = PANIC_MESSAGE.load(SeqCst); + assert!(message_ptr.is_null()); + + // Swapping null with null should be safe + let old_ptr = PANIC_MESSAGE.swap(ptr::null_mut(), SeqCst); + assert!(old_ptr.is_null()); + } + + #[test] + fn test_panic_message_replacement() { + // Test that replacing an existing message cleans up the old one + let message1 = "first message".to_string(); + let message2 = "second message".to_string(); + + let ptr1 = Box::into_raw(Box::new(message1)); + let ptr2 = Box::into_raw(Box::new(message2.clone())); + + PANIC_MESSAGE.store(ptr1, SeqCst); + let old_ptr = PANIC_MESSAGE.swap(ptr2, SeqCst); + + // Old pointer should be the first one + assert_eq!(old_ptr, ptr1); + + // Clean up both + unsafe { + drop(Box::from_raw(old_ptr)); + let final_ptr = PANIC_MESSAGE.swap(ptr::null_mut(), SeqCst); + let final_message = *Box::from_raw(final_ptr); + assert_eq!(final_message, message2); + } + } + + #[test] + fn test_metadata_update_atomic() { + // Test that metadata updates are atomic + let metadata = Metadata { + library_name: "test".to_string(), + library_version: "1.0.0".to_string(), + family: "test_family".to_string(), + tags: vec![], + }; + + let result = update_metadata(metadata.clone()); + assert!(result.is_ok()); + + // Verify metadata was stored + let metadata_ptr = METADATA.load(SeqCst); + assert!(!metadata_ptr.is_null()); + + unsafe { + let (stored_metadata, _) = &*metadata_ptr; + assert_eq!(stored_metadata.library_name, "test"); + } + } + + #[test] + fn test_format_message_with_message_and_location() { + let location = panic::Location::caller(); + let result = format_message("message", "test panic", Some(location)); + + assert!(result.starts_with("Process panicked with message \"test panic\" (")); + assert!(result.contains(&format!("{}:", location.file()))); + assert!(result.contains(&format!(":{}", location.line()))); + assert!(result.ends_with(&format!("{})", location.column()))); + } + + #[test] + fn test_format_message_with_message_no_location() { + let result = format_message("message", "test panic", None); + assert_eq!(result, "Process panicked with message \"test panic\""); + } + + #[test] + fn test_format_message_empty_message_with_location() { + let location = panic::Location::caller(); + let result = format_message("unknown type", "", Some(location)); + + assert!(result.starts_with("Process panicked with unknown type (")); + assert!(result.contains(&format!("{}:", location.file()))); + assert!(result.ends_with(&format!("{})", location.column()))); + } + + #[test] + fn test_format_message_empty_message_no_location() { + let result = format_message("unknown type", "", None); + assert_eq!(result, "Process panicked with unknown type"); + } + + #[test] + fn test_format_message_different_categories() { + let result1 = format_message("message", "test", None); + assert_eq!(result1, "Process panicked with message \"test\""); + + let result2 = format_message("unknown type", "", None); + assert_eq!(result2, "Process panicked with unknown type"); + + let result3 = format_message("custom category", "content", None); + assert_eq!(result3, "Process panicked with custom category \"content\""); + } + + #[test] + fn test_format_message_with_special_characters() { + let result = format_message("message", "test \"quoted\" 'text'", None); + assert_eq!( + result, + "Process panicked with message \"test \"quoted\" 'text'\"" + ); + } +} diff --git a/libdd-crashtracker/src/collector/emitters.rs b/libdd-crashtracker/src/collector/emitters.rs index f609c9f9a1..35e061d4e6 100644 --- a/libdd-crashtracker/src/collector/emitters.rs +++ b/libdd-crashtracker/src/collector/emitters.rs @@ -132,18 +132,27 @@ unsafe fn emit_backtrace_by_frames( Ok(()) } +#[allow(clippy::too_many_arguments)] pub(crate) fn emit_crashreport( pipe: &mut impl Write, config: &CrashtrackerConfiguration, config_str: &str, metadata_string: &str, + message_ptr: *mut String, sig_info: *const siginfo_t, ucontext: *const ucontext_t, ppid: i32, ) -> Result<(), EmitterError> { - emit_metadata(pipe, metadata_string)?; + // The following order is important in order to emit the crash ping: + // - receiver expects the config + // - then message if any + // - then siginfo (if the message is not set, we use the siginfo to generate the message) + // - then metadata emit_config(pipe, config_str)?; + emit_message(pipe, message_ptr)?; emit_siginfo(pipe, sig_info)?; + emit_metadata(pipe, metadata_string)?; + // after the metadata the ping should have been sent emit_ucontext(pipe, ucontext)?; emit_procinfo(pipe, ppid)?; emit_counters(pipe)?; @@ -191,6 +200,19 @@ fn emit_metadata(w: &mut impl Write, metadata_str: &str) -> Result<(), EmitterEr Ok(()) } +fn emit_message(w: &mut impl Write, message_ptr: *mut String) -> Result<(), EmitterError> { + if !message_ptr.is_null() { + let message = unsafe { &*message_ptr }; + if !message.trim().is_empty() { + writeln!(w, "{DD_CRASHTRACK_BEGIN_MESSAGE}")?; + writeln!(w, "{message}")?; + writeln!(w, "{DD_CRASHTRACK_END_MESSAGE}")?; + w.flush()?; + } + } + Ok(()) +} + fn emit_procinfo(w: &mut impl Write, pid: i32) -> Result<(), EmitterError> { writeln!(w, "{DD_CRASHTRACK_BEGIN_PROCINFO}")?; writeln!(w, "{{\"pid\": {pid} }}")?; @@ -455,4 +477,124 @@ mod tests { "crashtracker itself must be filtered away, found 'backtrace::backtrace'" ); } + + #[test] + #[cfg_attr(miri, ignore)] + fn test_emit_message_nullptr() { + let mut buf = Vec::new(); + emit_message(&mut buf, std::ptr::null_mut()).expect("to work ;-)"); + assert!(buf.is_empty()); + } + + #[test] + #[cfg_attr(miri, ignore)] + fn test_emit_message() { + let message = "test message"; + let message_ptr = Box::into_raw(Box::new(message.to_string())); + let mut buf = Vec::new(); + emit_message(&mut buf, message_ptr).expect("to work ;-)"); + let out = str::from_utf8(&buf).expect("to be valid UTF8"); + assert!(out.contains("BEGIN_MESSAGE")); + assert!(out.contains("END_MESSAGE")); + assert!(out.contains(message)); + // Clean up the allocated String + unsafe { drop(Box::from_raw(message_ptr)) }; + } + + #[test] + #[cfg_attr(miri, ignore)] + fn test_emit_message_empty_string() { + let empty_message = String::new(); + let message_ptr = Box::into_raw(Box::new(empty_message)); + let mut buf = Vec::new(); + + emit_message(&mut buf, message_ptr).expect("to work"); + + // Empty messages should not emit anything + assert!(buf.is_empty()); + + unsafe { drop(Box::from_raw(message_ptr)) }; + } + + #[test] + #[cfg_attr(miri, ignore)] + fn test_emit_message_whitespace_only() { + // Whitespace-only messages should not be emitted + let whitespace_message = " \n\t ".to_string(); + let message_ptr = Box::into_raw(Box::new(whitespace_message)); + let mut buf = Vec::new(); + + emit_message(&mut buf, message_ptr).expect("to work"); + + // Whitespace-only messages should not emit anything + assert!(buf.is_empty()); + + unsafe { drop(Box::from_raw(message_ptr)) }; + } + + #[test] + #[cfg_attr(miri, ignore)] + fn test_emit_message_with_leading_trailing_whitespace() { + // Messages with content and whitespace should be emitted (with the whitespace) + let message_with_whitespace = " error message ".to_string(); + let message_ptr = Box::into_raw(Box::new(message_with_whitespace.clone())); + let mut buf = Vec::new(); + + emit_message(&mut buf, message_ptr).expect("to work"); + let out = str::from_utf8(&buf).expect("to be valid UTF8"); + + // Should emit markers and preserve whitespace in content + assert!(out.contains("BEGIN_MESSAGE")); + assert!(out.contains("END_MESSAGE")); + assert!(out.contains(&message_with_whitespace)); + + unsafe { drop(Box::from_raw(message_ptr)) }; + } + + #[test] + #[cfg_attr(miri, ignore)] + fn test_emit_message_with_newlines() { + let message_with_newlines = "line1\nline2\nline3".to_string(); + let message_ptr = Box::into_raw(Box::new(message_with_newlines)); + let mut buf = Vec::new(); + + emit_message(&mut buf, message_ptr).expect("to work"); + let out = str::from_utf8(&buf).expect("to be valid UTF8"); + + assert!(out.contains("line1")); + assert!(out.contains("line2")); + assert!(out.contains("line3")); + + unsafe { drop(Box::from_raw(message_ptr)) }; + } + + #[test] + #[cfg_attr(miri, ignore)] + fn test_emit_message_unicode() { + let unicode_message = "Hello δΈ–η•Œ πŸ¦€ Rust!".to_string(); + let message_ptr = Box::into_raw(Box::new(unicode_message.clone())); + let mut buf = Vec::new(); + + emit_message(&mut buf, message_ptr).expect("to work"); + let out = str::from_utf8(&buf).expect("to be valid UTF8"); + + assert!(out.contains(&unicode_message)); + + unsafe { drop(Box::from_raw(message_ptr)) }; + } + + #[test] + #[cfg_attr(miri, ignore)] + fn test_emit_message_very_long() { + let long_message = "x".repeat(100000); // 100KB + let message_ptr = Box::into_raw(Box::new(long_message.clone())); + let mut buf = Vec::new(); + + emit_message(&mut buf, message_ptr).expect("to work"); + let out = str::from_utf8(&buf).expect("to be valid UTF8"); + + assert!(out.contains(&long_message[..100])); // At least first 100 chars + + unsafe { drop(Box::from_raw(message_ptr)) }; + } } diff --git a/libdd-crashtracker/src/crash_info/builder.rs b/libdd-crashtracker/src/crash_info/builder.rs index fdff37f213..c4dc262384 100644 --- a/libdd-crashtracker/src/crash_info/builder.rs +++ b/libdd-crashtracker/src/crash_info/builder.rs @@ -383,6 +383,7 @@ impl CrashInfoBuilder { /// This method requires that the builder has a UUID and metadata set. /// Siginfo is optional for platforms that don't support it (like Windows) pub fn build_crash_ping(&self) -> anyhow::Result { + let message = self.error.message.clone(); let sig_info = self.sig_info.clone(); let metadata = self.metadata.clone().context("metadata is required")?; @@ -390,6 +391,9 @@ impl CrashInfoBuilder { if let Some(sig_info) = sig_info { builder = builder.with_sig_info(sig_info); } + if let Some(message) = message { + builder = builder.with_custom_message(message); + } builder.build() } @@ -405,6 +409,10 @@ impl CrashInfoBuilder { self.metadata.is_some() } } + + pub fn has_message(&self) -> bool { + self.error.message.is_some() + } } #[cfg(test)] @@ -430,4 +438,96 @@ mod tests { assert_eq!(crash_ping.metadata(), &metadata); assert!(crash_ping.message().contains("crash processing started")); } + + #[test] + fn test_with_message() { + let mut builder = CrashInfoBuilder::new(); + let test_message = "Test error message".to_string(); + + let result = builder.with_message(test_message.clone()); + assert!(result.is_ok()); + assert!(builder.has_message()); + + // Build and verify message is present + let sig_info = SigInfo::test_instance(42); + builder.with_sig_info(sig_info).unwrap(); + builder.with_metadata(Metadata::test_instance(1)).unwrap(); + + let crash_ping = builder.build_crash_ping().unwrap(); + assert!(crash_ping.message().contains(&test_message)); + } + + #[test] + fn test_has_message_empty() { + let builder = CrashInfoBuilder::new(); + assert!(!builder.has_message()); + } + + #[test] + fn test_has_message_after_setting() { + let mut builder = CrashInfoBuilder::new(); + builder.with_message("test".to_string()).unwrap(); + assert!(builder.has_message()); + } + + #[test] + fn test_message_overwrite() { + let mut builder = CrashInfoBuilder::new(); + + builder.with_message("first message".to_string()).unwrap(); + assert!(builder.has_message()); + + // Overwrite with second message + builder.with_message("second message".to_string()).unwrap(); + assert!(builder.has_message()); + + // Build and verify only second message is present + let sig_info = SigInfo::test_instance(42); + builder.with_sig_info(sig_info).unwrap(); + builder.with_metadata(Metadata::test_instance(1)).unwrap(); + + let crash_ping = builder.build_crash_ping().unwrap(); + assert!(crash_ping.message().contains("second message")); + assert!(!crash_ping.message().contains("first message")); + builder.with_kind(ErrorKind::Panic).unwrap(); + + let report = builder.build().unwrap(); + assert_eq!(report.error.message.as_deref(), Some("second message")); + } + + #[test] + fn test_message_with_special_characters() { + let mut builder = CrashInfoBuilder::new(); + let special_message = "Error: 'panic' with \"quotes\" and\nnewlines\t\ttabs"; + + builder.with_message(special_message.to_string()).unwrap(); + builder.with_sig_info(SigInfo::test_instance(42)).unwrap(); + builder.with_metadata(Metadata::test_instance(1)).unwrap(); + + let crash_ping = builder.build_crash_ping().unwrap(); + assert!(crash_ping.message().contains(special_message)); + builder.with_kind(ErrorKind::UnixSignal).unwrap(); + + let report = builder.build().unwrap(); + assert_eq!(report.error.message.as_deref(), Some(special_message)); + } + + #[test] + fn test_very_long_message() { + let mut builder = CrashInfoBuilder::new(); + let long_message = "x".repeat(10000); // 10KB message + + builder.with_message(long_message.clone()).unwrap(); + assert!(builder.has_message()); + + builder.with_sig_info(SigInfo::test_instance(42)).unwrap(); + builder.with_metadata(Metadata::test_instance(1)).unwrap(); + + let crash_ping = builder.build_crash_ping().unwrap(); + assert!(crash_ping.message().len() >= 10000); + + builder.with_kind(ErrorKind::UnixSignal).unwrap(); + let report = builder.build().unwrap(); + assert!(report.error.message.as_ref().unwrap().len() >= 10000); + } } diff --git a/libdd-crashtracker/src/crash_info/telemetry.rs b/libdd-crashtracker/src/crash_info/telemetry.rs index d54d8a8925..1b9dda6608 100644 --- a/libdd-crashtracker/src/crash_info/telemetry.rs +++ b/libdd-crashtracker/src/crash_info/telemetry.rs @@ -63,16 +63,16 @@ impl CrashPingBuilder { let sig_info = self.sig_info; let metadata = self.metadata.context("metadata is required")?; - let message = self.custom_message.unwrap_or_else(|| { - if let Some(ref sig_info) = sig_info { - format!( - "Crashtracker crash ping: crash processing started - Process terminated with {:?} ({:?})", - sig_info.si_code_human_readable, sig_info.si_signo_human_readable - ) - } else { - "Crashtracker crash ping: crash processing started - Process terminated".to_string() - } - }); + let message = if let Some(custom_message) = self.custom_message { + format!("Crashtracker crash ping: crash processing started - {custom_message}") + } else if let Some(ref sig_info) = sig_info { + format!( + "Crashtracker crash ping: crash processing started - Process terminated with {:?} ({:?})", + sig_info.si_code_human_readable, sig_info.si_signo_human_readable + ) + } else { + "Crashtracker crash ping: crash processing started - Process terminated".to_string() + }; Ok(CrashPing { crash_uuid: crash_uuid.to_string(), @@ -824,6 +824,52 @@ mod tests { assert_eq!(crash_ping.siginfo(), Some(&sig_info)); } + #[test] + #[cfg_attr(miri, ignore)] + fn test_crash_ping_with_message_generated_from_sig_info() { + let sig_info = crate::SigInfo::test_instance(99); + let metadata = Metadata::test_instance(2); + + // Build crash ping through CrashInfoBuilder + let mut crash_info_builder = CrashInfoBuilder::new(); + crash_info_builder.with_sig_info(sig_info.clone()).unwrap(); + crash_info_builder.with_metadata(metadata.clone()).unwrap(); + let crash_ping = crash_info_builder.build_crash_ping().unwrap(); + + assert!(!crash_ping.crash_uuid().is_empty()); + assert!(Uuid::parse_str(crash_ping.crash_uuid()).is_ok()); + assert_eq!(crash_ping.message(), format!( + "Crashtracker crash ping: crash processing started - Process terminated with {:?} ({:?})", + sig_info.si_code_human_readable, sig_info.si_signo_human_readable + )); + assert_eq!(crash_ping.metadata(), &metadata); + assert_eq!(crash_ping.siginfo(), Some(&sig_info)); + } + + #[test] + #[cfg_attr(miri, ignore)] + fn test_crash_ping_with_custom_message() { + let sig_info = crate::SigInfo::test_instance(99); + let metadata = Metadata::test_instance(2); + + // Build crash ping through CrashInfoBuilder + let mut crash_info_builder = CrashInfoBuilder::new(); + crash_info_builder.with_sig_info(sig_info.clone()).unwrap(); + crash_info_builder.with_metadata(metadata.clone()).unwrap(); + crash_info_builder + .with_message("my process panicked".to_string()) + .unwrap(); + let crash_ping = crash_info_builder.build_crash_ping().unwrap(); + + assert!(!crash_ping.crash_uuid().is_empty()); + assert!(Uuid::parse_str(crash_ping.crash_uuid()).is_ok()); + assert!(crash_ping + .message() + .contains("crash processing started - my process panicked")); + assert_eq!(crash_ping.metadata(), &metadata); + assert_eq!(crash_ping.siginfo(), Some(&sig_info)); + } + #[tokio::test] #[cfg_attr(miri, ignore)] async fn test_crash_ping_telemetry_upload_all_fields() -> anyhow::Result<()> { diff --git a/libdd-crashtracker/src/receiver/receive_report.rs b/libdd-crashtracker/src/receiver/receive_report.rs index 5f44cb0e59..f1980a90b6 100644 --- a/libdd-crashtracker/src/receiver/receive_report.rs +++ b/libdd-crashtracker/src/receiver/receive_report.rs @@ -116,6 +116,7 @@ pub(crate) enum StdinState { // may have lines that we need to accumulate depending on runtime (e.g. Python) RuntimeStackFrame(Vec), RuntimeStackString(Vec), + Message, } /// A state machine that processes data from the crash-tracker collector line by @@ -227,19 +228,26 @@ fn process_line( StdinState::SigInfo if line.starts_with(DD_CRASHTRACK_END_SIGINFO) => StdinState::Waiting, StdinState::SigInfo => { let sig_info: SigInfo = serde_json::from_str(line)?; - // By convention, siginfo is the first thing sent. - let message = format!( - "Process terminated with {:?} ({:?})", - sig_info.si_code_human_readable, sig_info.si_signo_human_readable - ); + if !builder.has_message() { + let message = format!( + "Process terminated with {:?} ({:?})", + sig_info.si_code_human_readable, sig_info.si_signo_human_readable + ); + builder.with_message(message)?; + } builder.with_timestamp_now()?; builder.with_sig_info(sig_info)?; builder.with_incomplete(true)?; - builder.with_message(message)?; StdinState::SigInfo } + StdinState::Message if line.starts_with(DD_CRASHTRACK_END_MESSAGE) => StdinState::Waiting, + StdinState::Message => { + builder.with_message(line.to_string())?; + StdinState::Message + } + StdinState::SpanIds if line.starts_with(DD_CRASHTRACK_END_SPAN_IDS) => StdinState::Waiting, StdinState::SpanIds => { let span_ids: Vec = serde_json::from_str(line)?; @@ -289,6 +297,7 @@ fn process_line( StdinState::ProcInfo } StdinState::Waiting if line.starts_with(DD_CRASHTRACK_BEGIN_SIGINFO) => StdinState::SigInfo, + StdinState::Waiting if line.starts_with(DD_CRASHTRACK_BEGIN_MESSAGE) => StdinState::Message, StdinState::Waiting if line.starts_with(DD_CRASHTRACK_BEGIN_SPAN_IDS) => { StdinState::SpanIds } @@ -486,3 +495,132 @@ pub(crate) async fn receive_report_from_stream( Ok(Some((config, crash_info))) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_stdin_state_waiting_to_message() { + let mut builder = CrashInfoBuilder::new(); + let mut config = None; + + let state = StdinState::Waiting; + let line = DD_CRASHTRACK_BEGIN_MESSAGE; + + let next_state = process_line(&mut builder, &mut config, line, state, &None).unwrap(); + + assert!(matches!(next_state, StdinState::Message)); + } + + #[test] + fn test_stdin_state_message_content() { + let mut builder = CrashInfoBuilder::new(); + let mut config = None; + + // Enter message state + let state = StdinState::Message; + let message_line = "program panicked"; + + let next_state = + process_line(&mut builder, &mut config, message_line, state, &None).unwrap(); + + // Should stay in message state + assert!(matches!(next_state, StdinState::Message)); + + // Verify message was stored + assert!(builder.has_message()); + } + + #[test] + fn test_stdin_state_message_to_waiting() { + let mut builder = CrashInfoBuilder::new(); + let mut config = None; + + let state = StdinState::Message; + let line = DD_CRASHTRACK_END_MESSAGE; + + let next_state = process_line(&mut builder, &mut config, line, state, &None).unwrap(); + + assert!(matches!(next_state, StdinState::Waiting)); + } + + #[test] + fn test_message_state_with_empty_line() { + let mut builder = CrashInfoBuilder::new(); + let mut config = None; + + let state = StdinState::Message; + let empty_line = ""; + + let result = process_line(&mut builder, &mut config, empty_line, state, &None); + + // Should handle empty line without error + assert!(result.is_ok()); + } + + #[test] + fn test_message_state_with_multiline_content() { + let mut builder = CrashInfoBuilder::new(); + let mut config = None; + + // First line of message + let state = process_line( + &mut builder, + &mut config, + "Line 1 of panic", + StdinState::Message, + &None, + ) + .unwrap(); + + // Should still be in message state + assert!(matches!(state, StdinState::Message)); + + // Note: Current implementation may only store last message + // This test documents current behavior + } + + #[test] + fn test_message_state_full_workflow() { + let mut builder = CrashInfoBuilder::new(); + let mut config = None; + + // Start in waiting state + let mut state = StdinState::Waiting; + + // Transition to message + state = process_line( + &mut builder, + &mut config, + DD_CRASHTRACK_BEGIN_MESSAGE, + state, + &None, + ) + .unwrap(); + assert!(matches!(state, StdinState::Message)); + + // Add message content + state = process_line( + &mut builder, + &mut config, + "test panic message", + state, + &None, + ) + .unwrap(); + assert!(matches!(state, StdinState::Message)); + assert!(builder.has_message()); + + // End message + state = process_line( + &mut builder, + &mut config, + DD_CRASHTRACK_END_MESSAGE, + state, + &None, + ) + .unwrap(); + assert!(matches!(state, StdinState::Waiting)); + } +} diff --git a/libdd-crashtracker/src/shared/constants.rs b/libdd-crashtracker/src/shared/constants.rs index 43b56a95e3..85d6029ec1 100644 --- a/libdd-crashtracker/src/shared/constants.rs +++ b/libdd-crashtracker/src/shared/constants.rs @@ -17,6 +17,7 @@ pub const DD_CRASHTRACK_BEGIN_SPAN_IDS: &str = "DD_CRASHTRACK_BEGIN_SPAN_IDS"; pub const DD_CRASHTRACK_BEGIN_STACKTRACE: &str = "DD_CRASHTRACK_BEGIN_STACKTRACE"; pub const DD_CRASHTRACK_BEGIN_TRACE_IDS: &str = "DD_CRASHTRACK_BEGIN_TRACE_IDS"; pub const DD_CRASHTRACK_BEGIN_UCONTEXT: &str = "DD_CRASHTRACK_BEGIN_UCONTEXT"; +pub const DD_CRASHTRACK_BEGIN_MESSAGE: &str = "DD_CRASHTRACK_BEGIN_MESSAGE"; pub const DD_CRASHTRACK_DONE: &str = "DD_CRASHTRACK_DONE"; pub const DD_CRASHTRACK_END_ADDITIONAL_TAGS: &str = "DD_CRASHTRACK_END_ADDITIONAL_TAGS"; pub const DD_CRASHTRACK_END_CONFIG: &str = "DD_CRASHTRACK_END_CONFIG"; @@ -31,5 +32,6 @@ pub const DD_CRASHTRACK_END_SPAN_IDS: &str = "DD_CRASHTRACK_END_SPAN_IDS"; pub const DD_CRASHTRACK_END_STACKTRACE: &str = "DD_CRASHTRACK_END_STACKTRACE"; pub const DD_CRASHTRACK_END_TRACE_IDS: &str = "DD_CRASHTRACK_END_TRACE_IDS"; pub const DD_CRASHTRACK_END_UCONTEXT: &str = "DD_CRASHTRACK_END_UCONTEXT"; +pub const DD_CRASHTRACK_END_MESSAGE: &str = "DD_CRASHTRACK_END_MESSAGE"; pub const DD_CRASHTRACK_DEFAULT_TIMEOUT: Duration = Duration::from_millis(5_000);