diff --git a/ndk/CHANGELOG.md b/ndk/CHANGELOG.md index 742d5ea8..5103efb2 100644 --- 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 index 58861092..cf088485 100644 --- 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 index 684070d9..aac98854 100644 --- 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 index da92a24a..bfd118c6 100644 --- 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 index 2c70e364..809d56f2 100644 --- 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 `` status codes - typically rewrapped by Android's [`Errors.h`] - into @@ -12,3 +14,57 @@ pub(crate) fn status_to_io_result(status: i32, value: T) -> Result { 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) { + 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::() { + 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(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(); + }) +}