Add panic guards to callbacks (#412)

Co-authored-by: Marijn Suijten <[email protected]>
rust-mobile · Aug 9, 2023 · ca8adb8 · ca8adb8
1 parent 13ec1c0
commit ca8adb8
Show file tree

Hide file tree

Showing 5 changed files with 95 additions and 27 deletions.
diff --git a/ndk/CHANGELOG.md b/ndk/CHANGELOG.md
@@ -14,6 +14,7 @@
 - native_window: Add `lock()` to blit raw pixel data. (#404)
 - hardware_buffer_format: Add `YCbCr_P010` and `R8_UNORM` variants. (#405)
 - **Breaking:** hardware_buffer_format: Add catch-all variant. (#407)
+- Add panic guards to callbacks. (#412)
 
 # 0.7.0 (2022-07-24)
 

diff --git a/ndk/Cargo.toml b/ndk/Cargo.toml
@@ -33,6 +33,7 @@ test = ["ffi/test", "jni", "all"]
 [dependencies]
 bitflags = "2.0.0"
 jni-sys = "0.3.0"
+log = "0.4"
 num_enum = "0.6"
 raw-window-handle = "0.5"
 thiserror = "1.0.23"

diff --git a/ndk/src/audio.rs b/ndk/src/audio.rs
@@ -8,6 +8,7 @@
 //! [`AAudioStreamBuilder`]: https://developer.android.com/ndk/reference/group/audio#aaudiostreambuilder
 #![cfg(feature = "audio")]
 
+use crate::utils::abort_on_panic;
 use num_enum::{IntoPrimitive, TryFromPrimitive};
 use std::{
     borrow::Cow,
@@ -573,15 +574,17 @@ impl AudioStreamBuilder {
             audio_data: *mut c_void,
             num_frames: i32,
         ) -> ffi::aaudio_data_callback_result_t {
-            let callback = user_data as *mut AudioStreamDataCallback;
-            let stream = AudioStream {
-                inner: NonNull::new_unchecked(stream),
-                data_callback: None,
-                error_callback: None,
-            };
-            let result = (*callback)(&stream, audio_data, num_frames);
-            std::mem::forget(stream);
-            result as ffi::aaudio_data_callback_result_t
+            abort_on_panic(|| {
+                let callback = user_data as *mut AudioStreamDataCallback;
+                let stream = AudioStream {
+                    inner: NonNull::new_unchecked(stream),
+                    data_callback: None,
+                    error_callback: None,
+                };
+                let result = (*callback)(&stream, audio_data, num_frames);
+                std::mem::forget(stream);
+                result as ffi::aaudio_data_callback_result_t
+            })
         }
 
         unsafe {
@@ -657,15 +660,17 @@ impl AudioStreamBuilder {
             user_data: *mut c_void,
             error: ffi::aaudio_result_t,
         ) {
-            let callback = user_data as *mut AudioStreamErrorCallback;
-            let stream = AudioStream {
-                inner: NonNull::new_unchecked(stream),
-                data_callback: None,
-                error_callback: None,
-            };
-            let err = AudioError::from_result(error, || ()).unwrap_err();
-            (*callback)(&stream, err);
-            std::mem::forget(stream);
+            abort_on_panic(|| {
+                let callback = user_data as *mut AudioStreamErrorCallback;
+                let stream = AudioStream {
+                    inner: NonNull::new_unchecked(stream),
+                    data_callback: None,
+                    error_callback: None,
+                };
+                let err = AudioError::from_result(error, || ()).unwrap_err();
+                (*callback)(&stream, err);
+                std::mem::forget(stream);
+            })
         }
 
         unsafe {

diff --git a/ndk/src/media/image_reader.rs b/ndk/src/media/image_reader.rs
@@ -6,6 +6,7 @@
 
 use crate::media_error::{construct, construct_never_null, MediaError, MediaStatus, Result};
 use crate::native_window::NativeWindow;
+use crate::utils::abort_on_panic;
 use num_enum::{IntoPrimitive, TryFromPrimitive};
 use std::{
     convert::TryInto,
@@ -128,10 +129,12 @@ impl ImageReader {
             context: *mut c_void,
             reader: *mut ffi::AImageReader,
         ) {
-            let reader = ImageReader::from_ptr(NonNull::new_unchecked(reader));
-            let listener: *mut ImageListener = context as *mut _;
-            (*listener)(&reader);
-            std::mem::forget(reader);
+            abort_on_panic(|| {
+                let reader = ImageReader::from_ptr(NonNull::new_unchecked(reader));
+                let listener: *mut ImageListener = context as *mut _;
+                (*listener)(&reader);
+                std::mem::forget(reader);
+            })
         }
 
         let mut listener = ffi::AImageReader_ImageListener {
@@ -154,11 +157,13 @@ impl ImageReader {
             reader: *mut ffi::AImageReader,
             buffer: *mut ffi::AHardwareBuffer,
         ) {
-            let reader = ImageReader::from_ptr(NonNull::new_unchecked(reader));
-            let buffer = HardwareBuffer::from_ptr(NonNull::new_unchecked(buffer));
-            let listener: *mut BufferRemovedListener = context as *mut _;
-            (*listener)(&reader, &buffer);
-            std::mem::forget(reader);
+            abort_on_panic(|| {
+                let reader = ImageReader::from_ptr(NonNull::new_unchecked(reader));
+                let buffer = HardwareBuffer::from_ptr(NonNull::new_unchecked(buffer));
+                let listener: *mut BufferRemovedListener = context as *mut _;
+                (*listener)(&reader, &buffer);
+                std::mem::forget(reader);
+            })
         }
 
         let mut listener = ffi::AImageReader_BufferRemovedListener {

diff --git a/ndk/src/utils.rs b/ndk/src/utils.rs
@@ -1,4 +1,6 @@
 //! Internal utilities
+use log::{error, log_enabled, Level};
+use std::ffi::{c_int, CStr, CString};
 use std::io::{Error, Result};
 
 /// Turns standard `<errno.h>` status codes - typically rewrapped by Android's [`Errors.h`] - into
@@ -12,3 +14,57 @@ pub(crate) fn status_to_io_result<T>(status: i32, value: T) -> Result<T> {
         r => unreachable!("Status is positive integer {}", r),
     }
 }
+
+pub(crate) fn android_log(level: Level, tag: &CStr, msg: &CStr) {
+    let prio = match level {
+        Level::Error => ffi::android_LogPriority::ANDROID_LOG_ERROR,
+        Level::Warn => ffi::android_LogPriority::ANDROID_LOG_WARN,
+        Level::Info => ffi::android_LogPriority::ANDROID_LOG_INFO,
+        Level::Debug => ffi::android_LogPriority::ANDROID_LOG_DEBUG,
+        Level::Trace => ffi::android_LogPriority::ANDROID_LOG_VERBOSE,
+    };
+    unsafe {
+        ffi::__android_log_write(prio.0 as c_int, tag.as_ptr(), msg.as_ptr());
+    }
+}
+
+pub(crate) fn log_panic(panic: Box<dyn std::any::Any + Send>) {
+    fn log_panic(panic_str: &str) {
+        const RUST_PANIC_TAG: &CStr =
+            unsafe { CStr::from_bytes_with_nul_unchecked(b"RustPanic\0") };
+
+        let panic_str = CString::new(panic_str).unwrap_or_default();
+
+        // Use the Rust logger if installed and enabled, otherwise fall back to the Android system
+        // logger so there is at least some record of the panic
+        if log_enabled!(Level::Error) {
+            error!("RustPanic: {}", panic_str.to_string_lossy());
+            log::logger().flush();
+        } else {
+            android_log(Level::Error, RUST_PANIC_TAG, &panic_str);
+        }
+    }
+
+    match panic.downcast::<String>() {
+        Ok(panic_string) => log_panic(&panic_string),
+        Err(panic) => match panic.downcast::<&str>() {
+            Ok(panic_str) => log_panic(&panic_str),
+            Err(_) => log_panic("Unknown panic message type"),
+        },
+    }
+}
+
+/// Run a closure and abort the program if it panics.
+///
+/// This is generally used to ensure Rust callbacks won't unwind past the FFI boundary, which leads
+/// to undefined behaviour.
+pub(crate) fn abort_on_panic<R>(f: impl FnOnce() -> R) -> R {
+    std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)).unwrap_or_else(|panic| {
+        // Try logging the panic before aborting
+        //
+        // Just in case our attempt to log a panic could itself cause a panic we use a
+        // second catch_unwind here.
+        let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| log_panic(panic)));
+        std::process::abort();
+    })
+}