[PROF-12853] Catch panics inside wrap_with_ffi_result and wrap_with_void_ffi_result (#1334)

[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.

The return type change in `ddog_crasht_CrashInfoBuilder_build`
does change the tag enum entries, which unfortunately is a
breaking change.

Ideas on how to work around this? This makes the following
enum entries change:

* `DDOG_CRASHT_CRASH_INFO_NEW_RESULT_OK` =>
  `DDOG_CRASHT_RESULT_HANDLE_CRASH_INFO_OK_HANDLE_CRASH_INFO`
* `DDOG_CRASHT_CRASH_INFO_NEW_RESULT_ERR` =>
  `DDOG_CRASHT_RESULT_HANDLE_CRASH_INFO_ERR_HANDLE_CRASH_INFO`

**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.

Improve documentation around empty vec not allocating

Merge branch 'main' into ivoanjo/crash-handling-experiments

Fix off-by-one (including terminator in length)

I suspect in practice, since this is a static string, it doesn't make
a difference but let's fix it still.

Remove leftover comment

Ooops!

Clarify that failed allocation is the only expected source of an empty error

Linting fixes

Co-authored-by: taegyunkim <taegyun.kim@datadoghq.com>
Co-authored-by: ivo.anjo <ivo.anjo@datadoghq.com>

Author: ivoanjo

Cargo.lock

@@ -66,7 +66,7 @@ CHECK_RESULT(ddog_Vec_Tag_PushResult, DDOG_VEC_TAG_PUSH_RESULT_OK)
 
 EXTRACT_RESULT(CrashInfoBuilder,
                DDOG_CRASHT_CRASH_INFO_BUILDER_NEW_RESULT_OK)
-EXTRACT_RESULT(CrashInfo, DDOG_CRASHT_CRASH_INFO_NEW_RESULT_OK)
+EXTRACT_RESULT(CrashInfo, DDOG_CRASHT_RESULT_HANDLE_CRASH_INFO_OK_HANDLE_CRASH_INFO)
 EXTRACT_RESULT(StackTrace, DDOG_CRASHT_STACK_TRACE_NEW_RESULT_OK)
 EXTRACT_RESULT(StackFrame, DDOG_CRASHT_STACK_FRAME_NEW_RESULT_OK)
 

examples/ffi/crashinfo.cpp

@@ -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,22 @@ 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.
+///
+/// Note: This vec is 0-sized and thus does not allocate any memory (or need to be dropped).
+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"libdatadog failed: (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().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 +120,17 @@ 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.
+        // In particular this means we'll use the CANNOT_ALLOCATE_CHAR_SLICE error message for
+        // **every** empty error, and no other kinds of errors are expected to be empty.
+        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,30 @@
 // 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 +37,23 @@ 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(|| {
+            $crate::wrap_with_void_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_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 +74,93 @@ 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!("{} failed: (panic) {}", function_name, s)
+        } else if let Some(s) = error.downcast_ref::<&str>() {
+            anyhow::anyhow!("{} failed: (panic) {}", function_name, s)
+        } else {
+            anyhow::anyhow!(
+                "{} failed: (panic) Unable to retrieve panic context",
+                function_name
+            )
+        }
+        .into()
+    }))
+    .unwrap_or(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: [std::os::raw::c_char; 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(),
+                "libdatadog failed: (panic) Cannot allocate error message"
+            );
+        };
+    }
+
+    #[test]
+    #[named]
+    #[allow(clippy::redundant_closure_call)]
+    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(), "test_wrap_with_ffi_result_turns_panic_into_error failed: (panic) this is a test panic message");
+    }
+
+    #[test]
+    #[named]
+    fn test_wrap_with_ffi_result_does_not_modify_other_kinds_of_errors() {
+        let result: crate::result::VoidResult = wrap_with_void_ffi_result!({
+            Err(anyhow::anyhow!("this is a test error message"))?;
+        });
+
+        assert_eq!(result.unwrap_err().to_string(), "test_wrap_with_ffi_result_does_not_modify_other_kinds_of_errors failed: this is a test error message");
+    }
+}

libdd-common-ffi/src/utils.rs

@@ -53,6 +53,16 @@ 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)
@@ -106,6 +116,7 @@ impl<T> Vec<T> {
         unsafe { Slice::from_raw_parts(self.ptr, self.len) }
     }
 
+    /// Note: Like the regular rust `Vec`, this doesn't allocate memory when capacity is zero.
     pub const fn new() -> Self {
         Vec {
             ptr: NonNull::dangling().as_ptr(),

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()) })
 }