From 6782ea538e0ae015d212e1c4016d97e6bb1ed520 Mon Sep 17 00:00:00 2001 From: sanchda <838104+sanchda@users.noreply.github.com> Date: Fri, 8 Nov 2024 16:12:12 +0000 Subject: [PATCH 1/6] WIP --- crashtracker/src/collector/faultinfo.rs | 36 +++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 crashtracker/src/collector/faultinfo.rs diff --git a/crashtracker/src/collector/faultinfo.rs b/crashtracker/src/collector/faultinfo.rs new file mode 100644 index 000000000..4b60d2a18 --- /dev/null +++ b/crashtracker/src/collector/faultinfo.rs @@ -0,0 +1,36 @@ +// Copyright 2023-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +#![cfg(unix)] + +// Enum for supported signal types +#[derive(Debug, Clone, Copy, PartialEq)] +enum SignalType { + SIGSEGV, + SIGBUS, + SIGILL, + SIGFPE, + SIGABRT, + SIGTRAP, + SIGSYS, + SIGXCPU, + SIGXFSZ, + SIGSTKFLT, + SIGPOLL, + SIGPROF, + SIGVTALRM, + SIGIO, + SIGPWR, + SIGWINCH, + SIGUNUSED, + SIGRTMIN, + SIGRTMAX, +} + +struct FaultInfo { + pub fault_type: String, + pub fault_addr: u64, + pub fault_pid: u32, + pub fault_tid: u32, + pub fault_time: u64, +} From 2ba59492382c73e2ce1341d0abc4cb0a5af25097 Mon Sep 17 00:00:00 2001 From: sanchda <838104+sanchda@users.noreply.github.com> Date: Mon, 11 Nov 2024 13:46:32 +0000 Subject: [PATCH 2/6] Initial --- crashtracker/src/collector/crash_handler.rs | 19 +-- crashtracker/src/collector/emitters.rs | 63 ++++++---- crashtracker/src/collector/faultinfo.rs | 36 ------ crashtracker/src/collector/mod.rs | 1 + crashtracker/src/collector/siginfo_strings.rs | 111 ++++++++++++++++++ 5 files changed, 160 insertions(+), 70 deletions(-) delete mode 100644 crashtracker/src/collector/faultinfo.rs create mode 100644 crashtracker/src/collector/siginfo_strings.rs diff --git a/crashtracker/src/collector/crash_handler.rs b/crashtracker/src/collector/crash_handler.rs index 617bc38d1..4bbcbcb83 100644 --- a/crashtracker/src/collector/crash_handler.rs +++ b/crashtracker/src/collector/crash_handler.rs @@ -382,10 +382,10 @@ pub fn configure_receiver(config: CrashtrackerReceiverConfig) { } } -extern "C" fn handle_posix_sigaction(signum: i32, sig_info: *mut siginfo_t, ucontext: *mut c_void) { +extern "C" fn handle_posix_sigaction(signum: i32, siginfo: *mut siginfo_t, ucontext: *mut c_void) { // Handle the signal. Note this has a guard to ensure that we only generate // one crash report per process. - let _ = handle_posix_signal_impl(signum, sig_info); + let _ = handle_posix_signal_impl(siginfo); // Once we've handled the signal, chain to any previous handlers. // SAFETY: This was created by [register_crash_handlers]. There is a tiny @@ -430,11 +430,11 @@ extern "C" fn handle_posix_sigaction(signum: i32, sig_info: *mut siginfo_t, ucon } SigHandler::SigIgn => (), // Return and ignore the signal. SigHandler::Handler(f) => f(signum), - SigHandler::SigAction(f) => f(signum, sig_info, ucontext), + SigHandler::SigAction(f) => f(signum, siginfo, ucontext), }; } -fn handle_posix_signal_impl(signum: i32, sig_info: *mut siginfo_t) -> anyhow::Result<()> { +fn handle_posix_signal_impl(siginfo: *mut siginfo_t) -> anyhow::Result<()> { // If this is a SIGSEGV signal, it could be called due to a stack overflow. In that case, since // this signal allocates to the stack and cannot guarantee it is running without SA_NODEFER, it // is possible that we will re-emit the signal. Contemporary unices handle this just fine (no @@ -476,14 +476,6 @@ fn handle_posix_signal_impl(signum: i32, sig_info: *mut siginfo_t) -> anyhow::Re let timeout_ms = config.timeout_ms; let start_time = Instant::now(); // This is the time at which the signal was received - // Derive the faulting address from `sig_info` - let faulting_address: Option = - if !sig_info.is_null() && (signum == libc::SIGSEGV || signum == libc::SIGBUS) { - unsafe { Some((*sig_info).si_addr() as usize) } - } else { - None - }; - // During the execution of this signal handler, block ALL other signals, especially because we // cannot control whether or not we run with SA_NODEFER (crashtracker might have been chained). // The especially problematic signals are SIGCHLD and SIGPIPE, which are possibly delivered due @@ -509,8 +501,7 @@ fn handle_posix_signal_impl(signum: i32, sig_info: *mut siginfo_t) -> anyhow::Re config, config_str, metadata_string, - signum, - faulting_address, + unsafe { *siginfo }, ); let _ = unix_stream.flush(); diff --git a/crashtracker/src/collector/emitters.rs b/crashtracker/src/collector/emitters.rs index 1c4a3dfb9..2d5afe7da 100644 --- a/crashtracker/src/collector/emitters.rs +++ b/crashtracker/src/collector/emitters.rs @@ -4,6 +4,7 @@ use crate::collector::counters::emit_counters; use crate::collector::spans::emit_spans; use crate::collector::spans::emit_traces; +use crate::collector::siginfo_strings; use crate::shared::constants::*; use crate::CrashtrackerConfiguration; use crate::StacktraceCollection; @@ -100,12 +101,11 @@ pub(crate) fn emit_crashreport( config: &CrashtrackerConfiguration, config_str: &str, metadata_string: &str, - signum: i32, - faulting_address: Option, + siginfo: libc::siginfo_t, ) -> anyhow::Result<()> { emit_metadata(pipe, metadata_string)?; emit_config(pipe, config_str)?; - emit_siginfo(pipe, signum, faulting_address)?; + emit_siginfo(pipe, siginfo)?; emit_procinfo(pipe)?; pipe.flush()?; emit_counters(pipe)?; @@ -164,28 +164,51 @@ fn emit_proc_self_maps(w: &mut impl Write) -> anyhow::Result<()> { Ok(()) } +/// Serialize a siginfo_t to the given handle. +/// +/// This function eagerly emits various parts of the siginfo_t, even though some components are +/// only circumstantially useful. The kernel generally provides sane defaults to unused +/// members, and for downstream consumers it is probably easier to present a consistent keyspace +/// than to conditionally emit fields. +/// +/// In particular, note that `si_addr` is a repeat of `faulting_address`, the latter of which is +/// emitted only for `SIGSEGV` signals. This data layout should be considered unstable until +/// canonized in a data format RFC. +/// +/// PRECONDITIONS: +/// This function assumes that the crash-tracker is initialized. +/// SAFETY: +/// Crash-tracking functions are not reentrant. +/// No other crash-handler functions should be called concurrently. +/// ATOMICITY: +/// This function is not atomic. A crash during its execution may lead to +/// unexpected crash-handling behaviour. +/// SIGNAL SAFETY: +/// This function is careful to only write to the handle, without doing any +/// unnecessary mutexes or memory allocation. fn emit_siginfo( w: &mut impl Write, - signum: i32, - faulting_address: Option, + siginfo: libc::siginfo_t, ) -> anyhow::Result<()> { - let signame = if signum == libc::SIGSEGV { - "SIGSEGV" - } else if signum == libc::SIGBUS { - "SIGBUS" - } else { - "UNKNOWN" - }; + let signame = siginfo_strings::get_signal_name(&siginfo); + let codename = siginfo_strings::get_code_name(&siginfo); + let si_addr = unsafe { siginfo.si_addr() } as usize; + let si_signo = siginfo.si_signo; + let si_pid = unsafe { siginfo.si_pid() }; + let si_code = siginfo.si_code; writeln!(w, "{DD_CRASHTRACK_BEGIN_SIGINFO}")?; - if let Some(addr) = faulting_address { - writeln!( - w, - "{{\"signum\": {signum}, \"signame\": \"{signame}\", \"faulting_address\": {addr}}}" - )?; - } else { - writeln!(w, "{{\"signum\": {signum}, \"signame\": \"{signame}\"}}")?; - }; + write!(w, "{{")?; + write!(w, "\"signum\": {si_signo}, ")?; + write!(w, "\"signame\": \"{signame}\", ")?; + write!(w, "\"pid\": {si_pid}, ")?; + write!(w, "\"code\": {si_code}, ")?; + write!(w, "\"codename\": \"{codename}\", ")?; + if siginfo.si_signo == libc::SIGSEGV { + write!(w, "\"faulting_address\": {si_addr}, ")?; + } + write!(w, "\"si_addr\": {si_addr}")?; // last field, no trailing comma + write!(w, "}}")?; writeln!(w, "{DD_CRASHTRACK_END_SIGINFO}")?; Ok(()) } diff --git a/crashtracker/src/collector/faultinfo.rs b/crashtracker/src/collector/faultinfo.rs deleted file mode 100644 index 4b60d2a18..000000000 --- a/crashtracker/src/collector/faultinfo.rs +++ /dev/null @@ -1,36 +0,0 @@ -// Copyright 2023-Present Datadog, Inc. https://www.datadoghq.com/ -// SPDX-License-Identifier: Apache-2.0 - -#![cfg(unix)] - -// Enum for supported signal types -#[derive(Debug, Clone, Copy, PartialEq)] -enum SignalType { - SIGSEGV, - SIGBUS, - SIGILL, - SIGFPE, - SIGABRT, - SIGTRAP, - SIGSYS, - SIGXCPU, - SIGXFSZ, - SIGSTKFLT, - SIGPOLL, - SIGPROF, - SIGVTALRM, - SIGIO, - SIGPWR, - SIGWINCH, - SIGUNUSED, - SIGRTMIN, - SIGRTMAX, -} - -struct FaultInfo { - pub fault_type: String, - pub fault_addr: u64, - pub fault_pid: u32, - pub fault_tid: u32, - pub fault_time: u64, -} diff --git a/crashtracker/src/collector/mod.rs b/crashtracker/src/collector/mod.rs index 470f70d91..5628245d3 100644 --- a/crashtracker/src/collector/mod.rs +++ b/crashtracker/src/collector/mod.rs @@ -7,6 +7,7 @@ mod crash_handler; mod emitters; mod saguard; mod spans; +mod siginfo_strings; pub use api::*; pub use counters::{begin_op, end_op, reset_counters, OpTypes}; diff --git a/crashtracker/src/collector/siginfo_strings.rs b/crashtracker/src/collector/siginfo_strings.rs new file mode 100644 index 000000000..cbeee351e --- /dev/null +++ b/crashtracker/src/collector/siginfo_strings.rs @@ -0,0 +1,111 @@ +// Copyright 2023-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 +#![cfg(unix)] +use libc; + +pub fn get_signal_name(siginfo: &libc::siginfo_t) -> &'static str { + let signal = siginfo.si_signo; + match signal { + libc::SIGSEGV => "SIGSEGV", + libc::SIGBUS => "SIGBUS", + libc::SIGILL => "SIGILL", + libc::SIGABRT => "SIGABRT", + libc::SIGFPE => "SIGFPE", + libc::SIGTRAP => "SIGTRAP", + libc::SIGSYS => "SIGSYS", + _ => "UNKNOWN", + } +} + +// These are defined in siginfo.h +// They are only derived here because there doesn't appear to be a crate that provides them in a +// comparable way. There's no other reason for this, so it can be replaced as soon as a better +// solution is found. +// (this only copies the most common, actionable values) +pub mod siginfo_code{ + pub const SI_USER: i32 = 0; + pub const SI_KERNEL: i32 = 0x80; + pub const SI_QUEUE: i32 = -1; + pub const SI_TIMER: i32 = -2; + pub const SI_TKILL: i32 = -6; + + pub mod ill { + pub const ILLOPC: i32 = 1; + pub const ILLOPN: i32 = 2; + pub const ILLADR: i32 = 3; + pub const ILLTRP: i32 = 4; + pub const PRVOPC: i32 = 5; + pub const PRVREG: i32 = 6; + pub const COPROC: i32 = 7; + pub const BADSTK: i32 = 8; + } + + pub mod segv { + pub const MAPERR: i32 = 1; + pub const ACCERR: i32 = 2; + } + + pub mod bus { + pub const ADRALN: i32 = 1; + pub const ADRERR: i32 = 2; + pub const OBJERR: i32 = 3; + } + + pub mod trap { + pub const BRKPT: i32 = 1; + pub const TRACE: i32 = 2; + } + + pub mod sys { + pub const SECCOMP: i32 = 1; + } +} + +pub fn get_code_name(siginfo: &libc::siginfo_t) -> &'static str { + let signo = siginfo.si_signo; + + // Strip out the high byte for PTRACE_EVENT_* flags (defensive coding) + let code = siginfo.si_code & 0x7f; + + match signo { + libc::SIGILL => match code { + siginfo_code::ill::ILLOPC => "ILL_ILLOPC", + siginfo_code::ill::ILLOPN => "ILL_ILLOPN", + siginfo_code::ill::ILLADR => "ILL_ILLADR", + siginfo_code::ill::ILLTRP => "ILL_ILLTRP", + siginfo_code::ill::PRVOPC => "ILL_PRVOPC", + siginfo_code::ill::PRVREG => "ILL_PRVREG", + siginfo_code::ill::COPROC => "ILL_COPROC", + siginfo_code::ill::BADSTK => "ILL_BADSTK", + _ => "UNKNOWN_SIGILL", + }, + libc::SIGSEGV => match code { + siginfo_code::segv::MAPERR => "SEGV_MAPERR", + siginfo_code::segv::ACCERR => "SEGV_ACCERR", + _ => "UNKNOWN_SIGSEGV", + }, + libc::SIGBUS => match code { + siginfo_code::bus::ADRALN => "BUS_ADRALN", + siginfo_code::bus::ADRERR => "BUS_ADRERR", + siginfo_code::bus::OBJERR => "BUS_OBJERR", + _ => "UNKNOWN_SIGBUS", + }, + libc::SIGTRAP => match code { + siginfo_code::trap::BRKPT => "TRAP_BRKPT", + siginfo_code::trap::TRACE => "TRAP_TRACE", + _ => "UNKNOWN_SIGTRAP", + }, + libc::SIGSYS => match code { + siginfo_code::sys::SECCOMP => "SYS_SECCOMP", + _ => "UNKNOWN_SIGSYS", + }, + _ => match code { + siginfo_code::SI_USER => "SI_USER", + siginfo_code::SI_KERNEL => "SI_KERNEL", + siginfo_code::SI_QUEUE => "SI_QUEUE", + siginfo_code::SI_TIMER => "SI_TIMER", + siginfo_code::SI_TKILL => "SI_TKILL", + _ => "UNKNOWN_GENERAL", + }, + } +} From b428d49781e1be5e9fcfa1b656b66e0096f0351a Mon Sep 17 00:00:00 2001 From: sanchda <838104+sanchda@users.noreply.github.com> Date: Mon, 11 Nov 2024 15:47:51 +0000 Subject: [PATCH 3/6] Add tests --- Cargo.lock | 1 + crashtracker/Cargo.toml | 1 + crashtracker/src/collector/emitters.rs | 66 +++++++++++++++++-- crashtracker/src/collector/mod.rs | 2 +- crashtracker/src/collector/siginfo_strings.rs | 2 +- crashtracker/src/crash_info/mod.rs | 8 ++- crashtracker/src/crash_info/telemetry.rs | 10 +-- crashtracker/src/receiver.rs | 6 +- 8 files changed, 81 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 358a8a408..bf8cac07a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1358,6 +1358,7 @@ dependencies = [ "page_size", "portable-atomic", "rand", + "regex", "serde", "serde_json", "tempfile", diff --git a/crashtracker/Cargo.toml b/crashtracker/Cargo.toml index cd83337ed..50fbadc92 100644 --- a/crashtracker/Cargo.toml +++ b/crashtracker/Cargo.toml @@ -48,3 +48,4 @@ rand = "0.8.5" [dev-dependencies] tempfile = { version = "3.3" } +regex = "1.5" diff --git a/crashtracker/src/collector/emitters.rs b/crashtracker/src/collector/emitters.rs index 2d5afe7da..28bfc27ff 100644 --- a/crashtracker/src/collector/emitters.rs +++ b/crashtracker/src/collector/emitters.rs @@ -2,9 +2,9 @@ // SPDX-License-Identifier: Apache-2.0 use crate::collector::counters::emit_counters; +use crate::collector::siginfo_strings; use crate::collector::spans::emit_spans; use crate::collector::spans::emit_traces; -use crate::collector::siginfo_strings; use crate::shared::constants::*; use crate::CrashtrackerConfiguration; use crate::StacktraceCollection; @@ -186,10 +186,7 @@ fn emit_proc_self_maps(w: &mut impl Write) -> anyhow::Result<()> { /// SIGNAL SAFETY: /// This function is careful to only write to the handle, without doing any /// unnecessary mutexes or memory allocation. -fn emit_siginfo( - w: &mut impl Write, - siginfo: libc::siginfo_t, -) -> anyhow::Result<()> { +pub(crate) fn emit_siginfo(w: &mut impl Write, siginfo: libc::siginfo_t) -> anyhow::Result<()> { let signame = siginfo_strings::get_signal_name(&siginfo); let codename = siginfo_strings::get_code_name(&siginfo); let si_addr = unsafe { siginfo.si_addr() } as usize; @@ -257,3 +254,62 @@ fn emit_text_file(w: &mut impl Write, path: &str) -> anyhow::Result<()> { w.flush()?; Ok(()) } + +#[cfg(test)] +struct JsonCaptureWriter { + buffer: Vec, +} + +#[cfg(test)] +impl JsonCaptureWriter { + fn new() -> Self { + JsonCaptureWriter { buffer: Vec::new() } + } + + pub fn get_json(&self) -> String { + use regex::Regex; + let output = String::from_utf8_lossy(&self.buffer); + let re = Regex::new(r"DD_CRASHTRACK_\w+").unwrap(); + re.replace_all(&output, "").trim().to_string() + } +} + +#[cfg(test)] +impl Write for JsonCaptureWriter { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.buffer.write(buf) + } + + fn flush(&mut self) -> std::io::Result<()> { + // NOP, but needed for interface + Ok(()) + } +} + +#[test] +fn test_sigaction_siginfo() -> anyhow::Result<()> { + // This is a simple test to make sure that a valid SigInfo can be created from a + // libc::siginfo_t on the current platform. + use crate::SigInfo; + let mut siginfo: libc::siginfo_t = unsafe { std::mem::zeroed() }; + siginfo.si_signo = libc::SIGSEGV; + siginfo.si_code = 1; + + let mut writer = JsonCaptureWriter::new(); + emit_siginfo(&mut writer, siginfo)?; + let siginfo_json_str = writer.get_json(); + + // Is it valid JSON? + let _: serde_json::Value = serde_json::from_str(&siginfo_json_str)?; + + // Is it a valid SigInfo? + let siginfo: SigInfo = serde_json::from_str(&siginfo_json_str)?; + + // Sanity check for the fields + assert_eq!(siginfo.signum, libc::SIGSEGV as u64); + assert_eq!(siginfo.signame, "SIGSEGV"); + assert_eq!(siginfo.pid, 0); + assert_eq!(siginfo.code, 1); + assert_eq!(siginfo.codename, "SEGV_MAPERR"); + Ok(()) +} diff --git a/crashtracker/src/collector/mod.rs b/crashtracker/src/collector/mod.rs index 5628245d3..15dd02ff2 100644 --- a/crashtracker/src/collector/mod.rs +++ b/crashtracker/src/collector/mod.rs @@ -6,8 +6,8 @@ mod counters; mod crash_handler; mod emitters; mod saguard; -mod spans; mod siginfo_strings; +mod spans; pub use api::*; pub use counters::{begin_op, end_op, reset_counters, OpTypes}; diff --git a/crashtracker/src/collector/siginfo_strings.rs b/crashtracker/src/collector/siginfo_strings.rs index cbeee351e..962ac8d0f 100644 --- a/crashtracker/src/collector/siginfo_strings.rs +++ b/crashtracker/src/collector/siginfo_strings.rs @@ -22,7 +22,7 @@ pub fn get_signal_name(siginfo: &libc::siginfo_t) -> &'static str { // comparable way. There's no other reason for this, so it can be replaced as soon as a better // solution is found. // (this only copies the most common, actionable values) -pub mod siginfo_code{ +pub mod siginfo_code { pub const SI_USER: i32 = 0; pub const SI_KERNEL: i32 = 0x80; pub const SI_QUEUE: i32 = -1; diff --git a/crashtracker/src/crash_info/mod.rs b/crashtracker/src/crash_info/mod.rs index ac13af7f9..e32f46eec 100644 --- a/crashtracker/src/crash_info/mod.rs +++ b/crashtracker/src/crash_info/mod.rs @@ -20,9 +20,11 @@ use uuid::Uuid; #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct SigInfo { pub signum: u64, - #[serde(skip_serializing_if = "Option::is_none")] - #[serde(default)] - pub signame: Option, + pub signame: String, + pub pid: i32, + pub code: i32, + pub codename: String, + pub si_addr: usize, #[serde(skip_serializing_if = "Option::is_none")] #[serde(default)] pub faulting_address: Option, diff --git a/crashtracker/src/crash_info/telemetry.rs b/crashtracker/src/crash_info/telemetry.rs index 87a97f2e6..aac87a867 100644 --- a/crashtracker/src/crash_info/telemetry.rs +++ b/crashtracker/src/crash_info/telemetry.rs @@ -188,9 +188,7 @@ fn extract_crash_info_tags(crash_info: &CrashInfo) -> anyhow::Result { write!(&mut tags, "uuid:{}", crash_info.uuid)?; if let Some(siginfo) = &crash_info.siginfo { write!(&mut tags, ",signum:{}", siginfo.signum)?; - if let Some(signame) = &siginfo.signame { - write!(&mut tags, ",signame:{}", signame)?; - } + write!(&mut tags, ",signame:{}", siginfo.signame)?; if let Some(faulting_address) = &siginfo.faulting_address { write!(&mut tags, ",faulting_address:{:#018x}", faulting_address)?; } @@ -278,7 +276,11 @@ mod tests { os_info: os_info::Info::unknown(), siginfo: Some(SigInfo { signum: 11, - signame: Some("SIGSEGV".to_owned()), + signame: "SIGSEGV".to_string(), + pid: 1234, + code: 1, + codename: "SEGV_MAPERR".to_string(), + si_addr: 0, faulting_address: Some(0x1234), }), proc_info: None, diff --git a/crashtracker/src/receiver.rs b/crashtracker/src/receiver.rs index 5b0ab902c..8780cfdcb 100644 --- a/crashtracker/src/receiver.rs +++ b/crashtracker/src/receiver.rs @@ -396,8 +396,12 @@ mod tests { to_socket( sender, serde_json::to_string(&SigInfo { - signame: Some("SIGSEGV".to_string()), signum: 11, + signame: "SIGSEGV".to_string(), + pid: 0, + code: 1, + codename: "SEGV_MAPERR".to_string(), + si_addr: 0, faulting_address: None, })?, ) From 014cc0ce4e80da00055fa7ff3e89008707c9b49b Mon Sep 17 00:00:00 2001 From: sanchda <838104+sanchda@users.noreply.github.com> Date: Mon, 11 Nov 2024 17:04:03 +0000 Subject: [PATCH 4/6] Propagate state better in tests --- bin_tests/tests/crashtracker_bin_test.rs | 8 ++++++-- crashtracker/src/collector/emitters.rs | 2 +- crashtracker/src/collector/siginfo_strings.rs | 1 - 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/bin_tests/tests/crashtracker_bin_test.rs b/bin_tests/tests/crashtracker_bin_test.rs index 3fe98f1ab..5bece6936 100644 --- a/bin_tests/tests/crashtracker_bin_test.rs +++ b/bin_tests/tests/crashtracker_bin_test.rs @@ -121,9 +121,13 @@ fn test_crash_tracking_bin(crash_tracking_receiver_profile: BuildProfile, mode: ); assert_eq!( serde_json::json!({ - "signum": 11, - "signame": "SIGSEGV", + "code": 1, + "codename": "SEGV_MAPERR", "faulting_address": 0, + "pid": 0, + "si_addr": 0, + "signame": "SIGSEGV", + "signum": 11, }), crash_payload["siginfo"] ); diff --git a/crashtracker/src/collector/emitters.rs b/crashtracker/src/collector/emitters.rs index 28bfc27ff..84335cad8 100644 --- a/crashtracker/src/collector/emitters.rs +++ b/crashtracker/src/collector/emitters.rs @@ -205,7 +205,7 @@ pub(crate) fn emit_siginfo(w: &mut impl Write, siginfo: libc::siginfo_t) -> anyh write!(w, "\"faulting_address\": {si_addr}, ")?; } write!(w, "\"si_addr\": {si_addr}")?; // last field, no trailing comma - write!(w, "}}")?; + writeln!(w, "}}")?; writeln!(w, "{DD_CRASHTRACK_END_SIGINFO}")?; Ok(()) } diff --git a/crashtracker/src/collector/siginfo_strings.rs b/crashtracker/src/collector/siginfo_strings.rs index 962ac8d0f..19ca362a0 100644 --- a/crashtracker/src/collector/siginfo_strings.rs +++ b/crashtracker/src/collector/siginfo_strings.rs @@ -1,7 +1,6 @@ // Copyright 2023-Present Datadog, Inc. https://www.datadoghq.com/ // SPDX-License-Identifier: Apache-2.0 #![cfg(unix)] -use libc; pub fn get_signal_name(siginfo: &libc::siginfo_t) -> &'static str { let signal = siginfo.si_signo; From 02e7762cf8717ce034aadbbc130e6542bda7dba8 Mon Sep 17 00:00:00 2001 From: sanchda <838104+sanchda@users.noreply.github.com> Date: Mon, 11 Nov 2024 17:11:21 +0000 Subject: [PATCH 5/6] Comment --- crashtracker/src/collector/emitters.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crashtracker/src/collector/emitters.rs b/crashtracker/src/collector/emitters.rs index 84335cad8..c6d83c623 100644 --- a/crashtracker/src/collector/emitters.rs +++ b/crashtracker/src/collector/emitters.rs @@ -201,6 +201,12 @@ pub(crate) fn emit_siginfo(w: &mut impl Write, siginfo: libc::siginfo_t) -> anyh write!(w, "\"pid\": {si_pid}, ")?; write!(w, "\"code\": {si_code}, ")?; write!(w, "\"codename\": \"{codename}\", ")?; + + // For now, we'll double-emit data which is backed by `si_addr` because the telemetry backend + // won't do anything fancy with this payload, and `faulting_address` has significance to + // developers who are querying for specific faults. However, we should probably decide how + // this gets canonized and which component is responsible. + // For now, double-ship the address. if siginfo.si_signo == libc::SIGSEGV { write!(w, "\"faulting_address\": {si_addr}, ")?; } From c1e4fe390bc5213fdb6c8bf703de7e53c9aa7120 Mon Sep 17 00:00:00 2001 From: sanchda <838104+sanchda@users.noreply.github.com> Date: Mon, 11 Nov 2024 17:50:07 +0000 Subject: [PATCH 6/6] Fix FFI --- crashtracker-ffi/src/crash_info/datatypes.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/crashtracker-ffi/src/crash_info/datatypes.rs b/crashtracker-ffi/src/crash_info/datatypes.rs index 1b01ec2d4..f75a887a1 100644 --- a/crashtracker-ffi/src/crash_info/datatypes.rs +++ b/crashtracker-ffi/src/crash_info/datatypes.rs @@ -229,6 +229,10 @@ impl<'a> TryFrom<&StackFrame<'a>> for datadog_crashtracker::StackFrame { pub struct SigInfo<'a> { pub signum: u64, pub signame: CharSlice<'a>, + pub pid: i32, + pub code: i32, + pub code_name: CharSlice<'a>, + pub si_addr: usize, } impl<'a> TryFrom> for datadog_crashtracker::SigInfo { @@ -236,12 +240,20 @@ impl<'a> TryFrom> for datadog_crashtracker::SigInfo { fn try_from(value: SigInfo<'a>) -> Result { let signum = value.signum; - let signame = option_from_char_slice(value.signame)?; + let signame = value.signame.to_string(); + let code = value.code; + let codename = value.code_name.to_string(); + let pid = value.pid; let faulting_address = None; // TODO: Expose this to FFI + let si_addr = value.si_addr; Ok(Self { signum, signame, + pid, + code, + codename, faulting_address, + si_addr, }) } }