[PROF-12853] Catch panics inside wrap_with_ffi_result and wrap_with_void_ffi_result

**What does this PR do?**

This PR updates the `wrap_with_ffi_result` and
`wrap_with_void_ffi_result` macros to catch any panics that happen
inside them, returning them as errors.

The error handling is made in such a way (see `handle_panic_error`
for details) that it should be able to report back an error even if we
fail to do any allocations.

Important note: Because only the macros have been changed, and
ffi APIs that don't use the macros are of course not affected and
can still trigger panics. If we like this approach, I'll follow-up
with a separate PR to update other APIs to use the new macros.

**Motivation:**

In <https://docs.google.com/document/d/1weMu9P03KKhPQ-gh9BMqRrEzpa1BnnY0LaSRGJbfc7A/edit?usp=sharing>
(Datadog-only link, sorry!) we saw `ddog_prof_Exporter_send`
crashing due to what can be summed up as

`ddog_prof_Exporter_send` (report a profile) ->
  hyper-util tries to do dns resolution in a separate thread pool ->
    tokio failed to create a new thread ->
      panic and we tear down the app because we can't report a profile

This is not good at all, and this PR solves this inspired by
earlier work in #815 and #1083.

**Additional Notes:**

While I don't predict that will happen very often, callers that
want to opt-out of the catch unwind behavior can still use the
`..._no_catch` variants of the macros.

**How to test the change?**

This change includes test coverage. I've also separately tried to
sprinkle a few `panic!` calls manually and tested that it works as
expected.

Author: ivoanjo

Cargo.lock

@@ -300,7 +300,6 @@ pub unsafe extern "C" fn ddog_prof_Exporter_send(
         let exporter = exporter.to_inner_mut()?;
         let cancel = cancel.to_inner_mut().ok();
         let response = exporter.send(request, cancel.as_deref())?;
-
         anyhow::Ok(HttpStatus(response.status().as_u16()))
     })
 }

datadog-profiling-ffi/src/exporter.rs

@@ -29,3 +29,5 @@ serde = "1.0"
 
 [dev-dependencies]
 bolero = "0.13"
+assert_no_alloc = "1.1.2"
+function_name = "0.3.0"

libdd-common-ffi/Cargo.toml

@@ -5,6 +5,16 @@ use crate::slice::{AsBytes, CharSlice};
 use crate::vec::Vec;
 use std::fmt::{Debug, Display, Formatter};
 
+/// You probably don't want to use this directly. This constant is used by `handle_panic_error` to signal that something
+/// went wrong, but avoid needing any allocations to represent it.
+pub(crate) const CANNOT_ALLOCATE_ERROR: Error = Error { message: Vec::new() };
+
+// This error message is used as a placeholder for errors without message -- corresponding to an error where we
+// couldn't even _allocate_ the message (or some other even weirder error).
+const CANNOT_ALLOCATE: &std::ffi::CStr = c"Panic: Cannot allocate error message";
+const CANNOT_ALLOCATE_CHAR_SLICE: CharSlice = unsafe { crate::Slice::from_raw_parts(
+    CANNOT_ALLOCATE.as_ptr(), CANNOT_ALLOCATE.to_bytes_with_nul().len()) };
+
 /// Please treat this as opaque; do not reach into it, and especially don't
 /// write into it! The most relevant APIs are:
 /// * `ddog_Error_message`, to get the message as a slice.
@@ -104,7 +114,13 @@ pub unsafe extern "C" fn ddog_Error_drop(error: Option<&mut Error>) {
 pub unsafe extern "C" fn ddog_Error_message(error: Option<&Error>) -> CharSlice<'_> {
     match error {
         None => CharSlice::empty(),
-        Some(err) => CharSlice::from(err.as_ref()),
+        // When the error is empty (CANNOT_ALLOCATE_ERROR) we assume we failed to allocate an actual error and
+        // return this placeholder message instead.
+        Some(err) => if *err == CANNOT_ALLOCATE_ERROR {
+            CANNOT_ALLOCATE_CHAR_SLICE
+        } else {
+            CharSlice::from(err.as_ref())
+        },
     }
 }
 

libdd-common-ffi/src/error.rs

@@ -34,6 +34,12 @@ impl From<anyhow::Result<()>> for VoidResult {
     }
 }
 
+impl From<Error> for VoidResult {
+    fn from(value: Error) -> Self {
+        Self::Err(value)
+    }
+}
+
 /// A generic result type for when an operation may fail,
 /// or may return <T> in case of success.
 #[repr(C)]
@@ -68,3 +74,9 @@ impl<T> From<anyhow::Result<T>> for Result<T> {
         }
     }
 }
+
+impl<T> From<Error> for Result<T> {
+    fn from(value: Error) -> Self {
+        Self::Err(value)
+    }
+}

libdd-common-ffi/src/result.rs

@@ -104,3 +104,9 @@ impl From<String> for StringWrapperResult {
         Self::Ok(value.into())
     }
 }
+
+impl From<Error> for StringWrapperResult {
+    fn from(value: Error) -> Self {
+        Self::Err(value)
+    }
+}

libdd-common-ffi/src/string.rs

@@ -1,11 +1,28 @@
 // Copyright 2024-Present Datadog, Inc. https://www.datadoghq.com/
 // SPDX-License-Identifier: Apache-2.0
 
+use std::panic::{catch_unwind, AssertUnwindSafe};
+
 /// Wraps a C-FFI function in standard form
 /// Expects the function to return a result type that implements into and to be decorated with
 /// #[named].
 #[macro_export]
 macro_rules! wrap_with_ffi_result {
+    ($body:block) => {{
+        use std::panic::{catch_unwind, AssertUnwindSafe};
+
+        catch_unwind(AssertUnwindSafe(|| { $crate::wrap_with_ffi_result_no_catch!({ $body }) }))
+            .map_or_else(
+                |e| $crate::utils::handle_panic_error(e, function_name!()).into(),
+                |result| result,
+            )
+    }};
+}
+
+/// Wraps a C-FFI function in standard form (no catch variant).
+/// Same as `wrap_with_ffi_result` but does not try to catch panics.
+#[macro_export]
+macro_rules! wrap_with_ffi_result_no_catch {
     ($body:block) => {{
         use anyhow::Context;
         (|| $body)()
@@ -18,6 +35,21 @@ macro_rules! wrap_with_ffi_result {
 /// Expects the function to return a VoidResult and to be decorated with #[named].
 #[macro_export]
 macro_rules! wrap_with_void_ffi_result {
+    ($body:block) => {{
+        use std::panic::{catch_unwind, AssertUnwindSafe};
+
+        catch_unwind(AssertUnwindSafe(|| { libdd_common_ffi::wrap_with_void_ffi_result_no_catch!({ $body }) }))
+            .map_or_else(
+                |e| libdd_common_ffi::utils::handle_panic_error(e, function_name!()).into(),
+                |result| result,
+            )
+    }};
+}
+
+/// Wraps a C-FFI function in standard form (no catch variant).
+/// Same as `wrap_with_void_ffi_result` but does not try to catch panics.
+#[macro_export]
+macro_rules! wrap_with_void_ffi_result_no_catch {
     ($body:block) => {{
         use anyhow::Context;
         (|| {
@@ -38,3 +70,73 @@ impl ToHexStr for usize {
         format!("0x{self:X}")
     }
 }
+
+/// You probably don't want to use this directly. This is used by `wrap_with_*_ffi_result` macros to turn a panic error
+/// into an actual nice error. Because the original panic may have been caused by being unable to allocate,
+/// this helper handles failures to allocate as well, turning them into a fallback error.
+pub fn handle_panic_error(error: Box<dyn std::any::Any + Send + 'static>, function_name: &str) -> crate::Error {
+    catch_unwind(AssertUnwindSafe(|| {
+        // This pattern of String vs &str comes from
+        // https://doc.rust-lang.org/std/panic/struct.PanicHookInfo.html#method.payload
+        if let Some(s) = error.downcast_ref::<String>() {
+            anyhow::anyhow!("Panic inside {}: {}", function_name, s)
+        } else if let Some(s) = error.downcast_ref::<&str>() {
+            // panic!("double panic");
+            anyhow::anyhow!("Panic inside {}: {}", function_name, s)
+        } else {
+            anyhow::anyhow!("Panic: Unable to retrieve panic context")
+        }.into()
+    })).unwrap_or_else(|_panic_while_allocating_nice_error| crate::error::CANNOT_ALLOCATE_ERROR)
+}
+
+#[cfg(test)]
+mod tests {
+    use assert_no_alloc::{ assert_no_alloc, AllocDisabler };
+    use function_name::named;
+
+    #[cfg(debug_assertions)] // required when disable_release is set (default)
+    #[global_allocator]
+    static ALLOCATOR: AllocDisabler = AllocDisabler;
+
+    #[test]
+    fn test_handle_panic_error_fallback_does_not_allocate() {
+        let mut error_result_buffer: [i8; 100] = [0; 100];
+
+        assert_no_alloc(|| {
+            // Simulate fallback code path of handle_panic_error + ddog_Error_message
+            let fallback_error = crate::error::CANNOT_ALLOCATE_ERROR;
+            let error_message = unsafe {
+                crate::ddog_Error_message(Some(&fallback_error))
+            };
+
+            // Stash error message so we can assert on it
+            let n = error_message.len().min(error_result_buffer.len());
+            error_result_buffer[..n].copy_from_slice(&error_message[..n]);
+        });
+
+        unsafe {
+            let c_str = std::ffi::CStr::from_ptr(error_result_buffer.as_ptr());
+            assert_eq!(c_str.to_str().unwrap(), "Panic: Cannot allocate error message");
+        };
+    }
+
+    #[test]
+    #[named]
+    fn test_wrap_with_ffi_result_turns_panic_into_error() {
+        // Save the current panic handler and replace it with a no-op so that Rust doesn't print anything inside
+        // `wrap_with_ffi_result`...
+        let original_hook = std::panic::take_hook();
+        std::panic::set_hook(Box::new(|_| {}));
+
+        let result: crate::Result<()> = wrap_with_ffi_result!({
+            panic!("this is a test panic message");
+            #[allow(unreachable_code)]
+            anyhow::Ok(())
+        });
+
+        // ...restore original behavior
+        std::panic::set_hook(original_hook);
+
+        assert_eq!(result.unwrap_err().to_string(), "Panic inside test_wrap_with_ffi_result_turns_panic_into_error: this is a test panic message");
+    }
+}

libdd-common-ffi/src/utils.rs

@@ -53,6 +53,12 @@ impl<T: Eq> Eq for Vec<T> {}
 
 impl<T> Drop for Vec<T> {
     fn drop(&mut self) {
+        // A Rust Vec of size 0 [has no allocated memory](https://doc.rust-lang.org/std/vec/struct.Vec.html#guarantees):
+        // "In particular, if you construct a Vec with capacity 0 via Vec::new, vec![], Vec::with_capacity(0), or by calling shrink_to_fit on an empty Vec, it will not allocate memory."
+        // And as per https://doc.rust-lang.org/nomicon/vec/vec-dealloc.html:
+        // "We must not call alloc::dealloc when self.cap == 0, as in this case we haven't actually allocated any memory."
+        if self.capacity == 0 { return; }
+
         let vec =
             unsafe { alloc::vec::Vec::from_raw_parts(self.ptr as *mut T, self.len, self.capacity) };
         drop(vec)

libdd-common-ffi/src/vec.rs

@@ -42,31 +42,18 @@ pub unsafe extern "C" fn ddog_crasht_CrashInfoBuilder_drop(builder: *mut Handle<
     }
 }
 
-#[allow(dead_code)]
-#[repr(C)]
-pub enum CrashInfoNewResult {
-    Ok(Handle<CrashInfo>),
-    Err(Error),
-}
+// Name the type so it's prettier for the consumer
+pub type CrashInfoNewResult = libdd_common_ffi::Result<Handle<CrashInfo>>;
 
 /// # Safety
 /// The `builder` can be null, but if non-null it must point to a Builder made by this module,
 /// which has not previously been dropped.
 #[no_mangle]
 #[must_use]
-pub unsafe extern "C" fn ddog_crasht_CrashInfoBuilder_build(
-    builder: *mut Handle<CrashInfoBuilder>,
-) -> CrashInfoNewResult {
-    match ddog_crasht_crash_info_builder_build_impl(builder) {
-        Ok(crash_info) => CrashInfoNewResult::Ok(crash_info),
-        Err(err) => CrashInfoNewResult::Err(err.into()),
-    }
-}
-
 #[named]
-unsafe fn ddog_crasht_crash_info_builder_build_impl(
+pub unsafe extern "C" fn ddog_crasht_CrashInfoBuilder_build(
     mut builder: *mut Handle<CrashInfoBuilder>,
-) -> anyhow::Result<Handle<CrashInfo>> {
+) -> CrashInfoNewResult {
     wrap_with_ffi_result!({ anyhow::Ok(builder.take()?.build()?.into()) })
 }