Improve and add tests

Author: gleocadie

bin_tests/src/bin/crashing_test_app.rs

@@ -23,8 +23,23 @@ mod unix {
 
     const TEST_COLLECTOR_TIMEOUT: Duration = Duration::from_secs(10);
 
-    #[inline(never)]
-    unsafe fn fn3() {
+    enum CrashType {
+        Segfault,
+        Panic,
+    }
+
+    impl std::str::FromStr for CrashType {
+        type Err = anyhow::Error;
+        fn from_str(s: &str) -> Result<Self, Self::Err> {
+            match s {
+                "segfault" => Ok(CrashType::Segfault),
+                "panic" => Ok(CrashType::Panic),
+                _ => anyhow::bail!("Invalid crash type: {s}"),
+            }
+        }
+    }
+
+    unsafe fn cause_segfault() {
         #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
         {
             std::arch::asm!("mov eax, [0]", options(nostack));
@@ -37,13 +52,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 fn2(crash_type: CrashType) {
+        fn3(crash_type);
     }
 
     #[inline(never)]
-    fn fn1() {
-        fn2()
+    fn fn1(crash_type: CrashType) {
+        fn2(crash_type);
     }
 
     #[inline(never)]
@@ -53,6 +80,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");
@@ -100,7 +128,7 @@ mod unix {
             metadata,
         )?;
 
-        fn1();
+        fn1(crash_type.parse().context("Invalid crash type")?);
         Ok(())
     }
 }

bin_tests/src/lib.rs

@@ -43,6 +43,7 @@ pub struct ArtifactsBuild {
     pub artifact_type: ArtifactType,
     pub build_profile: BuildProfile,
     pub triple_target: Option<String>,
+    pub panic_abort: Option<bool>,
 }
 
 fn inner_build_artifact(c: &ArtifactsBuild) -> anyhow::Result<PathBuf> {
@@ -55,6 +56,13 @@ fn inner_build_artifact(c: &ArtifactsBuild) -> anyhow::Result<PathBuf> {
         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 {
+            build_cmd.env("RUSTFLAGS", "-C panic=abort");
+        }
+    }
+
     build_cmd.arg(&c.name);
 
     let output = build_cmd.output().unwrap();

bin_tests/tests/crashtracker_bin_test.rs

@@ -198,6 +198,88 @@ fn test_crash_tracking_errors_intake_uds_socket() {
     );
 }
 
+#[test]
+#[cfg_attr(miri, ignore)]
+fn test_crash_tracking_bin_panic() {
+    test_crash_tracking_app("panic");
+}
+
+#[test]
+#[cfg_attr(miri, ignore)]
+fn test_crash_tracking_bin_segfault() {
+    test_crash_tracking_app("segfault");
+}
+
+fn test_crash_tracking_app(crash_type: &str) {
+    let (_, crashtracker_receiver) = setup_crashtracking_crates(BuildProfile::Release);
+
+    let crashing_app = ArtifactsBuild {
+        name: "crashing_test_app".to_owned(),
+        build_profile: BuildProfile::Debug,
+        artifact_type: ArtifactType::Bin,
+        triple_target: None,
+        panic_abort: Some(true),
+        ..Default::default()
+    };
+
+    let fixtures = setup_test_fixtures(&[&crashtracker_receiver, &crashing_app]);
+
+    let mut p = process::Command::new(&fixtures.artifacts[&crashing_app])
+        .arg(format!("file://{}", fixtures.crash_profile_path.display()))
+        .arg(fixtures.artifacts[&crashtracker_receiver].as_os_str())
+        .arg(&fixtures.output_dir)
+        .arg(crash_type)
+        .spawn()
+        .unwrap();
+
+    let exit_status = bin_tests::timeit!("exit after signal", {
+        eprintln!("Waiting for exit");
+        p.wait().unwrap()
+    });
+    assert!(!exit_status.success());
+
+    let stderr_path = format!("{0}/out.stderr", fixtures.output_dir.display());
+    let stderr = fs::read(stderr_path)
+        .context("reading crashtracker stderr")
+        .unwrap();
+    let stdout_path = format!("{0}/out.stdout", fixtures.output_dir.display());
+    let stdout = fs::read(stdout_path)
+        .context("reading crashtracker stdout")
+        .unwrap();
+    let s = String::from_utf8(stderr);
+    assert!(
+        matches!(
+            s.as_deref(),
+            Ok("") | Ok("Failed to fully receive crash.  Exit state was: StackTrace([])\n")
+            | Ok("Failed to fully receive crash.  Exit state was: InternalError(\"{\\\"ip\\\": \\\"\")\n"),
+        ),
+        "got {s:?}"
+    );
+    assert_eq!(Ok(""), String::from_utf8(stdout).as_deref());
+
+    let crash_profile = fs::read(fixtures.crash_profile_path)
+        .context("reading crashtracker profiling payload")
+        .unwrap();
+    let crash_payload = serde_json::from_slice::<serde_json::Value>(&crash_profile)
+        .context("deserializing crashtracker profiling payload to json")
+        .unwrap();
+
+    let sig_info = &crash_payload["sig_info"];
+
+    match crash_type {
+        "panic" => {
+            let error = &crash_payload["error"];
+            let expected_message = "program panicked";
+            assert_eq!(error["message"].as_str().unwrap(), expected_message);
+        }
+        "segfault" => {
+            let error = &crash_payload["error"];
+            assert_error_message(&error["message"], sig_info);
+        }
+        _ => unreachable!("Invalid crash type: {crash_type}"),
+    }
+}
+
 // This test is disabled for now on x86_64 musl and macos
 // It seems that on aarch64 musl, libc has CFI which allows
 // unwinding passed the signal frame.
@@ -208,9 +290,6 @@ fn test_crasht_tracking_validate_callstack() {
     test_crash_tracking_callstack()
 }
 
-#[test]
-#[cfg(not(any(all(target_arch = "x86_64", target_env = "musl"), target_os = "macos")))]
-#[cfg_attr(miri, ignore)]
 fn test_crash_tracking_callstack() {
     let (_, crashtracker_receiver) = setup_crashtracking_crates(BuildProfile::Release);
 
@@ -230,6 +309,7 @@ fn test_crash_tracking_callstack() {
         .arg(format!("file://{}", fixtures.crash_profile_path.display()))
         .arg(fixtures.artifacts[&crashtracker_receiver].as_os_str())
         .arg(&fixtures.output_dir)
+        .arg("segfault")
         .spawn()
         .unwrap();
 

libdd-crashtracker/src/collector/crash_handler.rs

@@ -97,32 +97,19 @@ pub fn register_panic_hook() -> anyhow::Result<()> {
     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_hook));
-    Ok(())
-}
-
-/// The panic hook function.
-///
-/// This function is used to update the metadata with the panic message
-/// and call 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 load on an atomic pointer.
-fn panic_hook(panic_info: &PanicHookInfo<'_>) {
-    if let Some(s) = panic_info.payload().downcast_ref::<&str>() {
-        let message_ptr = PANIC_MESSAGE.swap(Box::into_raw(Box::new(s.to_string())), SeqCst);
-        // message_ptr should be null, but just in case.
-        if !message_ptr.is_null() {
-            unsafe {
-                std::mem::drop(Box::from_raw(message_ptr));
+    panic::set_hook(Box::new(|panic_info| {
+        if let Some(s) = panic_info.payload().downcast_ref::<&str>() {
+            let message_ptr = PANIC_MESSAGE.swap(Box::into_raw(Box::new(s.to_string())), 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);
+        call_previous_panic_hook(panic_info);
+    }));
+    Ok(())
 }
 
 /// Call the previous panic hook.

libdd-crashtracker/src/collector/emitters.rs

@@ -143,10 +143,13 @@ pub(crate) fn emit_crashreport(
     ucontext: *const ucontext_t,
     ppid: i32,
 ) -> Result<(), EmitterError> {
-    emit_message(pipe, message_ptr)?;
-    emit_metadata(pipe, metadata_string)?;
     emit_config(pipe, config_str)?;
+    emit_message(pipe, message_ptr)?;
     emit_siginfo(pipe, sig_info)?;
+    // send metadata after the config (needed to send the ping/report)
+    // after the message
+    // after the siginfo (if the message is not set, we use the siginfo to generate the message)
+    emit_metadata(pipe, metadata_string)?;
     emit_ucontext(pipe, ucontext)?;
     emit_procinfo(pipe, ppid)?;
     emit_counters(pipe)?;
@@ -469,4 +472,27 @@ 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)) };
+    }
 }

libdd-crashtracker/src/crash_info/builder.rs

@@ -398,13 +398,17 @@ 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<CrashPing> {
+        let message = self.error.message.clone();
         let sig_info = self.sig_info.clone();
         let metadata = self.metadata.clone().context("metadata is required")?;
 
         let mut builder = CrashPingBuilder::new(self.uuid).with_metadata(metadata);
         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()
     }
 

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(),
@@ -802,6 +802,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<()> {