Safety comments

Author: gyuheon0h

datadog-crashtracker-ffi/src/runtime_callback.rs

@@ -158,6 +158,11 @@ mod tests {
     use datadog_crashtracker::RuntimeStackFrame;
     use std::ffi::CString;
     use std::ptr;
+    use std::sync::Mutex;
+
+    // Use a mutex to ensure tests run sequentially to avoid race conditions
+    // with the global static variable
+    static TEST_MUTEX: Mutex<()> = Mutex::new(());
 
     unsafe extern "C" fn test_runtime_callback(
         emit_frame: unsafe extern "C" fn(*const RuntimeStackFrame),
@@ -183,6 +188,7 @@ mod tests {
 
     #[test]
     fn test_ffi_callback_registration() {
+        let _guard = TEST_MUTEX.lock().unwrap();
         unsafe {
             // Ensure clean state at start
             clear_runtime_callback();
@@ -227,6 +233,7 @@ mod tests {
 
     #[test]
     fn test_enum_based_registration() {
+        let _guard = TEST_MUTEX.lock().unwrap();
         unsafe {
             clear_runtime_callback();
 

datadog-crashtracker/src/runtime_callback.rs

@@ -111,7 +111,6 @@ impl std::str::FromStr for CallbackType {
 /// Global storage for the runtime callback
 ///
 /// Uses atomic pointer to ensure safe access from signal handlers
-/// Stores enums directly - much simpler and signal-safe
 static RUNTIME_CALLBACK: AtomicPtr<(RuntimeStackCallback, RuntimeType, CallbackType)> =
     AtomicPtr::new(ptr::null_mut());
 
@@ -146,8 +145,14 @@ pub struct RuntimeStackFrame {
 /// - Only async-signal-safe functions
 ///
 /// # Parameters
-/// - `emit_frame`: Function to call for each runtime frame
-/// - `emit_stacktrace_string`: Function to call for complete stacktrace string
+/// - `emit_frame`: Function to call for each runtime frame (marked unsafe because it takes raw pointers)
+/// - `emit_stacktrace_string`: Function to call for complete stacktrace string (marked unsafe because it takes raw C string pointers)
+///
+/// # Safety
+/// The callback function is marked unsafe because:
+/// - It receives function pointers that take raw pointers as parameters
+/// - The callback must ensure any pointers it passes to these functions are valid
+/// - All C strings passed must be null-terminated and remain valid for the call duration
 pub type RuntimeStackCallback = unsafe extern "C" fn(
     emit_frame: unsafe extern "C" fn(*const RuntimeStackFrame),
     emit_stacktrace_string: unsafe extern "C" fn(*const c_char),
@@ -213,6 +218,8 @@ pub fn register_runtime_stack_callback(
     let previous = RUNTIME_CALLBACK.swap(callback_data, Ordering::SeqCst);
 
     if !previous.is_null() {
+        // Safety: previous was returned by Box::into_raw() above or in a previous call,
+        // so it's guaranteed to be a valid Box pointer. We reconstruct the Box to drop it.
         let _ = unsafe { Box::from_raw(previous) };
     }
 
@@ -232,12 +239,19 @@ pub fn is_runtime_callback_registered() -> bool {
 ///
 /// # Safety
 /// This function loads from an atomic pointer and dereferences it.
+/// The caller must ensure that no other thread is calling `clear_runtime_callback`
+/// or `register_runtime_stack_callback` concurrently, as those could invalidate
+/// the pointer between the null check and dereferencing.
 pub unsafe fn get_registered_runtime_type_enum() -> Option<RuntimeType> {
     let callback_ptr = RUNTIME_CALLBACK.load(Ordering::SeqCst);
     if callback_ptr.is_null() {
         return None;
     }
 
+    // Safety: callback_ptr was checked to be non-null above, and was created by
+    // Box::into_raw() in register_runtime_stack_callback(), so it's a valid pointer
+    // to a properly aligned, initialized tuple. The atomic load with SeqCst ordering
+    // ensures we see the pointer after it was stored.
     let (_, runtime_type, _) = &*callback_ptr;
     Some(*runtime_type)
 }
@@ -248,12 +262,19 @@ pub unsafe fn get_registered_runtime_type_enum() -> Option<RuntimeType> {
 ///
 /// # Safety
 /// This function loads from an atomic pointer and dereferences it.
+/// The caller must ensure that no other thread is calling `clear_runtime_callback`
+/// or `register_runtime_stack_callback` concurrently, as those could invalidate
+/// the pointer between the null check and dereferencing.
 pub unsafe fn get_registered_callback_type_enum() -> Option<CallbackType> {
     let callback_ptr = RUNTIME_CALLBACK.load(Ordering::SeqCst);
     if callback_ptr.is_null() {
         return None;
     }
 
+    // Safety: callback_ptr was checked to be non-null above, and was created by
+    // Box::into_raw() in register_runtime_stack_callback(), so it's a valid pointer
+    // to a properly aligned, initialized tuple. The atomic load with SeqCst ordering
+    // ensures we see the pointer after it was stored.
     let (_, _, callback_type) = &*callback_ptr;
     Some(*callback_type)
 }
@@ -262,12 +283,19 @@ pub unsafe fn get_registered_callback_type_enum() -> Option<CallbackType> {
 ///
 /// # Safety
 /// This function loads from an atomic pointer and dereferences it.
+/// The caller must ensure that no other thread is calling `clear_runtime_callback`
+/// or `register_runtime_stack_callback` concurrently, as those could invalidate
+/// the pointer between the null check and dereferencing.
 pub unsafe fn get_registered_runtime_type_ptr() -> *const std::ffi::c_char {
     let callback_ptr = RUNTIME_CALLBACK.load(Ordering::SeqCst);
     if callback_ptr.is_null() {
         return std::ptr::null();
     }
 
+    // Safety: callback_ptr was checked to be non-null above, and was created by
+    // Box::into_raw() in register_runtime_stack_callback(), so it's a valid pointer
+    // to a properly aligned, initialized tuple. The returned C string pointer
+    // points to static string literals, so it's always valid.
     let (_, runtime_type, _) = &*callback_ptr;
     runtime_type.as_cstr().as_ptr()
 }
@@ -276,12 +304,19 @@ pub unsafe fn get_registered_runtime_type_ptr() -> *const std::ffi::c_char {
 ///
 /// # Safety
 /// This function loads from an atomic pointer and dereferences it.
+/// The caller must ensure that no other thread is calling `clear_runtime_callback`
+/// or `register_runtime_stack_callback` concurrently, as those could invalidate
+/// the pointer between the null check and dereferencing.
 pub unsafe fn get_registered_callback_type_ptr() -> *const std::ffi::c_char {
     let callback_ptr = RUNTIME_CALLBACK.load(Ordering::SeqCst);
     if callback_ptr.is_null() {
         return std::ptr::null();
     }
 
+    // Safety: callback_ptr was checked to be non-null above, and was created by
+    // Box::into_raw() in register_runtime_stack_callback(), so it's a valid pointer
+    // to a properly aligned, initialized tuple. The returned C string pointer
+    // points to static string literals, so it's always valid.
     let (_, _, callback_type) = &*callback_ptr;
     callback_type.as_cstr().as_ptr()
 }
@@ -294,11 +329,15 @@ pub unsafe fn get_registered_callback_type_ptr() -> *const std::ffi::c_char {
 ///
 /// # Safety
 /// This function should only be called when it's safe to clear the callback,
-/// such as during testing or application shutdown.
+/// such as during testing or application shutdown. The caller must ensure:
+/// - No other thread is concurrently calling functions that dereference the callback pointer
+/// - No signal handlers are currently executing that might invoke the callback
+/// - The callback is not being used in any other way
 pub unsafe fn clear_runtime_callback() {
     let old_ptr = RUNTIME_CALLBACK.swap(std::ptr::null_mut(), Ordering::SeqCst);
     if !old_ptr.is_null() {
-        // Drop the tuple to clean up
+        // Safety: old_ptr was created by Box::into_raw() in register_runtime_stack_callback(),
+        // so it's a valid Box pointer. We reconstruct the Box to properly drop the tuple.
         let _ = Box::from_raw(old_ptr);
     }
 }
@@ -310,7 +349,10 @@ pub unsafe fn clear_runtime_callback() {
 ///
 /// # Safety
 /// This function is intended to be called from signal handlers and must maintain
-/// signal safety. It does not perform any dynamic allocation.
+/// signal safety. It does not perform any dynamic allocation. The caller must ensure:
+/// - No other thread is calling `clear_runtime_callback` concurrently
+/// - The registered callback function is signal-safe
+/// - The writer parameter remains valid for the duration of the call
 #[cfg(unix)]
 pub(crate) unsafe fn invoke_runtime_callback_with_writer<W: std::io::Write>(
     writer: &mut W,
@@ -320,16 +362,25 @@ pub(crate) unsafe fn invoke_runtime_callback_with_writer<W: std::io::Write>(
         return Err(std::io::Error::other("No runtime callback registered"));
     }
 
+    // Safety: callback_ptr was checked to be non-null above, and was created by
+    // Box::into_raw() in register_runtime_stack_callback(), so it's a valid pointer
+    // to a properly aligned, initialized tuple.
     let (callback_fn, _, _) = &*callback_ptr;
 
     // Reset frame counter
     FRAME_COUNT.with(|count| count.set(0));
 
     // Define the emit_frame function that writes directly to the pipe
+    // Safety: This function is called by the user's runtime callback. It's marked unsafe
+    // because it dereferences raw pointers, but the safety is guaranteed by the contract
+    // with the runtime callback system.
     unsafe extern "C" fn emit_frame_collector(frame: *const RuntimeStackFrame) {
         // We need access to the writer, so we'll use thread-local storage for it
         FRAME_WRITER.with(|writer_cell| {
             if let Some(writer_ptr) = *writer_cell.borrow() {
+                // Safety: writer_ptr was stored in thread-local storage before
+                // calling the user callback, so it's guaranteed to be valid and point
+                // to the same writer object we passed in.
                 let writer = &mut *writer_ptr;
 
                 FRAME_COUNT.with(|count| {
@@ -341,6 +392,7 @@ pub(crate) unsafe fn invoke_runtime_callback_with_writer<W: std::io::Write>(
                     }
 
                     // Write the frame as JSON
+                    // Safety: frame pointer is passed from the runtime callback, writer is valid
                     let _ = emit_frame_as_json(writer, frame);
                     let _ = writer.flush();
 
@@ -350,16 +402,22 @@ pub(crate) unsafe fn invoke_runtime_callback_with_writer<W: std::io::Write>(
         });
     }
 
+    // Safety: This function is called by the user's runtime callback with a C string pointer.
+    // The safety requirements are documented in the RuntimeStackCallback type.
     unsafe extern "C" fn emit_stacktrace_string_collector(stacktrace_string: *const c_char) {
         if stacktrace_string.is_null() {
             return;
         }
 
+        // Safety: stacktrace_string is guaranteed by the runtime callback contract to be
+        // a valid, null-terminated C string that remains valid for the duration of this call.
         let cstr = std::ffi::CStr::from_ptr(stacktrace_string);
         let bytes = cstr.to_bytes();
 
         FRAME_WRITER.with(|writer_cell| {
             if let Some(writer_ptr) = *writer_cell.borrow() {
+                // Safety: writer_ptr was stored in thread-local storage just before
+                // calling the user callback, so it's guaranteed to be valid.
                 let writer = &mut *writer_ptr;
                 let _ = writer.write_all(bytes);
                 let _ = writer.flush();
@@ -373,6 +431,8 @@ pub(crate) unsafe fn invoke_runtime_callback_with_writer<W: std::io::Write>(
     });
 
     // Invoke the user callback
+    // Safety: callback_fn was verified to be non-null during registration, and the
+    // emit functions we're passing are compatible with the expected function signatures.
     callback_fn(emit_frame_collector, emit_stacktrace_string_collector);
 
     // Clear the writer reference
@@ -387,6 +447,11 @@ pub(crate) unsafe fn invoke_runtime_callback_with_writer<W: std::io::Write>(
 ///
 /// This function writes a RuntimeStackFrame directly as JSON without intermediate allocation.
 /// It must be signal-safe.
+///
+/// # Safety
+/// The caller must ensure that `frame` is either null or points to a valid, properly
+/// initialized RuntimeStackFrame. All C string pointers within the frame must be either
+/// null or point to valid, null-terminated C strings.
 #[cfg(unix)]
 unsafe fn emit_frame_as_json(
     writer: &mut dyn std::io::Write,
@@ -396,12 +461,16 @@ unsafe fn emit_frame_as_json(
         return Ok(());
     }
 
+    // Safety: frame was checked to be non-null above. The caller guarantees it points
+    // to a valid RuntimeStackFrame.
     let frame_ref = &*frame;
     write!(writer, "{{")?;
     let mut first = true;
 
     // Convert C strings to Rust strings and write JSON fields
     if !frame_ref.function_name.is_null() {
+        // Safety: frame_ref.function_name was checked to be non-null. The caller
+        // guarantees it points to a valid, null-terminated C string.
         let c_str = std::ffi::CStr::from_ptr(frame_ref.function_name);
         if let Ok(s) = c_str.to_str() {
             if !s.is_empty() {
@@ -412,6 +481,8 @@ unsafe fn emit_frame_as_json(
     }
 
     if !frame_ref.file_name.is_null() {
+        // Safety: frame_ref.file_name was checked to be non-null. The caller
+        // guarantees it points to a valid, null-terminated C string.
         let c_str = std::ffi::CStr::from_ptr(frame_ref.file_name);
         if let Ok(s) = c_str.to_str() {
             if !s.is_empty() {
@@ -441,6 +512,8 @@ unsafe fn emit_frame_as_json(
     }
 
     if !frame_ref.class_name.is_null() {
+        // Safety: frame_ref.class_name was checked to be non-null. The caller
+        // guarantees it points to a valid, null-terminated C string.
         let c_str = std::ffi::CStr::from_ptr(frame_ref.class_name);
         if let Ok(s) = c_str.to_str() {
             if !s.is_empty() {
@@ -454,6 +527,8 @@ unsafe fn emit_frame_as_json(
     }
 
     if !frame_ref.module_name.is_null() {
+        // Safety: frame_ref.module_name was checked to be non-null. The caller
+        // guarantees it points to a valid, null-terminated C string.
         let c_str = std::ffi::CStr::from_ptr(frame_ref.module_name);
         if let Ok(s) = c_str.to_str() {
             if !s.is_empty() {