From 91cf9d7229f1b72bdd64d44f8a65a2ee035bb1ec Mon Sep 17 00:00:00 2001 From: Robert Bragg Date: Thu, 12 Mar 2026 21:08:01 +0000 Subject: [PATCH] Track ANativeActivity pointer lifetime more carefully Once `on_destroy()` returns then the `NativeActivity.java` code will call an `unloadNativeCode` native method that will `delete` the `ANativeActivity` and invalidate any pointers we hold. Considering the possibility that an `AndroidApp` could be retained beyond the lifetime of the original `NativeActivity`, this ensures we always hold the `WaitableNativeActivityState::mutex` before dereferencing this pointer and ensures we clear the pointer before returning from `on_destroy` so we're also able to perform `null` pointer checks before dereferencing. Considering that `AndroidApp::vm_as_ptr` previously depended on dereferencing the `ANativeActivity`, this updates it to instead use `JavaVM::singleton()` which we guarantee will be initialized before the `AndroidApp` is created. Considering that `AndroidApp::activity_as_ptr()` promises to return a global reference that remains valid for the lifetime of the `AndroidApp`, but the `ANativeActivity::clazz` reference is deleted after `on_destroy()` returns, we now create our own `Global` reference for the `Activity` that is owned by `AndroidAppInner`. --- android-activity/CHANGELOG.md | 11 ++- android-activity/src/game_activity/mod.rs | 5 -- android-activity/src/lib.rs | 29 ++++--- android-activity/src/native_activity/glue.rs | 44 ++++++++--- android-activity/src/native_activity/mod.rs | 80 +++++++++++++++----- 5 files changed, 121 insertions(+), 48 deletions(-) diff --git a/android-activity/CHANGELOG.md b/android-activity/CHANGELOG.md index 71e1a1d..9467b3d 100644 --- a/android-activity/CHANGELOG.md +++ b/android-activity/CHANGELOG.md @@ -14,14 +14,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `AndroidApp::java_main_looper()` gives access to the `ALooper` for the Java main / UI thread ([#198](https://github.com/rust-mobile/android-activity/pull/198)) - `AndroidApp::run_on_java_main_thread()` can be used to run boxed closures on the Java main / UI thread ([#232](https://github.com/rust-mobile/android-activity/pull/232)) -### Fixed - -- *Safety* `AndroidApp::asset_manager()` returns an `AssetManager` that has a safe `'static` lifetime that's not invalidated when `android_main()` returns [#233](https://github.com/rust-mobile/android-activity/pull/233) - ### Changed + - rust-version bumped to 1.85.0 ([#193](https://github.com/rust-mobile/android-activity/pull/193), [#219](https://github.com/rust-mobile/android-activity/pull/219)) - GameActivity updated to 4.0.0 (requires the corresponding 4.0.0 `.aar` release from Google) ([#191](https://github.com/rust-mobile/android-activity/pull/191)) +### Fixed + +- *Safety* `AndroidApp::asset_manager()` returns an `AssetManager` that has a safe `'static` lifetime that's not invalidated when `android_main()` returns ([#233](https://github.com/rust-mobile/android-activity/pull/233)) +- *Safety* The `native-activity` backend clears its `ANativeActivity` ptr after `onDestroy` and `AndroidApp` remains safe to access after `android_main()` returns ([#234](https://github.com/rust-mobile/android-activity/pull/234)) +- *Safety* `AndroidApp::activity_as_ptr()` returns a pointer to a global reference that remains valid until `AndroidApp` is dropped, instead of the `ANativeActivity`'s `clazz` pointer which is only guaranteed to be valid until `onDestroy` returns (`native-activity` backend) ([#234](https://github.com/rust-mobile/android-activity/pull/234)) + ## [0.6.0] - 2024-04-26 ### Changed diff --git a/android-activity/src/game_activity/mod.rs b/android-activity/src/game_activity/mod.rs index 92b0226..0ca8105 100644 --- a/android-activity/src/game_activity/mod.rs +++ b/android-activity/src/game_activity/mod.rs @@ -316,11 +316,6 @@ pub struct AndroidAppInner { } impl AndroidAppInner { - pub fn vm_as_ptr(&self) -> *mut c_void { - let app_ptr = self.native_app.as_ptr(); - unsafe { (*(*app_ptr).activity).vm as _ } - } - pub fn activity_as_ptr(&self) -> *mut c_void { let app_ptr = self.native_app.as_ptr(); unsafe { (*(*app_ptr).activity).javaGameActivity as _ } diff --git a/android-activity/src/lib.rs b/android-activity/src/lib.rs index b5b0366..62af497 100644 --- a/android-activity/src/lib.rs +++ b/android-activity/src/lib.rs @@ -121,6 +121,7 @@ use std::sync::RwLock; use std::time::Duration; use bitflags::bitflags; +use jni::vm::JavaVM; use libc::c_void; use ndk::asset::AssetManager; @@ -596,29 +597,39 @@ impl AndroidApp { /// [`jni`]: https://crates.io/crates/jni /// [`JavaVM`]: https://docs.rs/jni/latest/jni/struct.JavaVM.html pub fn vm_as_ptr(&self) -> *mut c_void { - self.inner.read().unwrap().vm_as_ptr() + JavaVM::singleton().unwrap().get_raw() as _ } - /// Returns a JNI object reference for this application's JVM `Activity` as a pointer + /// Returns an (*unowned*) JNI global object reference for this + /// application's JVM `Activity` as a pointer /// - /// If you use the [`jni`] crate you can cast this as a `JObject` reference via: + /// If you use the [`jni`] crate you can cast this as a `JObject` reference + /// via: /// ```no_run /// # use jni::objects::JObject; /// # use jni::refs::Global; /// # fn use_jni(env: &jni::Env, app: &android_activity::AndroidApp) -> jni::errors::Result<()> { /// let raw_activity_global = app.activity_as_ptr() as jni::sys::jobject; - /// // SAFETY: The pointer is valid as long as `app` is valid. + /// // SAFETY: The reference / pointer is valid as long as `app` is valid /// let activity = unsafe { env.as_cast_raw::>(&raw_activity_global)? }; /// # Ok(()) } /// ``` /// /// # JNI Safety /// - /// Note that the object reference will be a JNI global reference, not a - /// local reference and it should not be deleted. Don't wrap the reference - /// in an [`Auto`] which would try to explicitly delete the reference - /// when dropped. Similarly, don't wrap the reference as a [`Global`] - /// which would also try to explicitly delete the reference when dropped. + /// Note that the returned reference will be a JNI global reference *that + /// you do not own*. + /// - Don't wrap the reference as a [`Global`] which would try to delete the + /// reference when dropped. + /// - Don't wrap the reference in an [`Auto`] which would treat the + /// reference like a local reference and try to delete it when dropped. + /// + /// The reference is only guaranteed to be valid until you drop the + /// [`AndroidApp`]. + /// + /// **Warning:** Don't assume the returned reference has a `'static` lifetime + /// since it's possible for `android_main()` to run multiple times over the + /// lifetime of an application with a new `AndroidApp` instance each time. /// /// [`jni`]: https://crates.io/crates/jni /// [`Auto`]: https://docs.rs/jni/latest/jni/refs/struct.Auto.html diff --git a/android-activity/src/native_activity/glue.rs b/android-activity/src/native_activity/glue.rs index 5735fce..3897f1f 100644 --- a/android-activity/src/native_activity/glue.rs +++ b/android-activity/src/native_activity/glue.rs @@ -75,8 +75,6 @@ pub enum State { #[derive(Debug)] pub struct WaitableNativeActivityState { - pub activity: *mut ndk_sys::ANativeActivity, - pub mutex: Mutex, pub cond: Condvar, } @@ -210,6 +208,29 @@ pub enum NativeThreadState { #[derive(Debug)] pub struct NativeActivityState { + /// Set as soon as the Java main thread notifies us of an `onDestroyed` + /// callback. + pub destroyed: bool, + + /// The `ANativeActivity` associated with the NativeActivity instance + /// + /// # Safety + /// + /// This pointer will be reset to `null` when the NativeActivity is + /// destroyed. + /// + /// Keep in mind that `NativeActivityState` is ref-counted and can + /// potentially out-last an `onDestroy` callback where we may reset this to + /// be a null pointer! + /// + /// For example: + /// - An application could put an `AndroidApp` into a global `'static` and + /// keep it alive beyond `android_main` + /// - An application could schedule a callback to run on the Java main + /// thread with an `AndroidApp` clone and by the time it runs then the + /// associated `ANativeActivity` could have been destroyed. + pub activity: *mut ndk_sys::ANativeActivity, + pub msg_read: libc::c_int, pub msg_write: libc::c_int, pub config: ConfigurationRef, @@ -222,9 +243,6 @@ pub struct NativeActivityState { pub thread_state: NativeThreadState, pub app_has_saved_state: bool, - /// Set as soon as the Java main thread notifies us of an - /// `onDestroyed` callback. - pub destroyed: bool, pub pending_input_queue: *mut ndk_sys::AInputQueue, pub pending_window: Option, } @@ -357,8 +375,8 @@ impl WaitableNativeActivityState { }; Self { - activity, mutex: Mutex::new(NativeActivityState { + activity, msg_read: msgpipe[0], msg_write: msgpipe[1], config, @@ -392,6 +410,11 @@ impl WaitableNativeActivityState { guard.msg_read = -1; libc::close(guard.msg_write); guard.msg_write = -1; + + // The last thing that `NativeActivity` `onDestroy` does is to call a + // native method (`unloadNativeCode`) which will `delete` the + // `ANativeActivity` instance. + guard.activity = ptr::null_mut(); } } @@ -605,7 +628,7 @@ impl WaitableNativeActivityState { AppCmd::ConfigChanged => { let guard = self.mutex.lock().unwrap(); let config = ndk_sys::AConfiguration_new(); - ndk_sys::AConfiguration_fromAssetManager(config, (*self.activity).assetManager); + ndk_sys::AConfiguration_fromAssetManager(config, (*guard.activity).assetManager); let config = Configuration::from_ptr(NonNull::new_unchecked(config)); guard.config.replace(config); log::debug!("Config: {:#?}", guard.config); @@ -896,6 +919,8 @@ fn rust_glue_entry( Some(16), |env| -> jni::errors::Result<()> { // SAFETY: We know jni_activity is a valid JNI global ref to an Activity instance + // that will remain valid until `onDestroy` is handled (not possible until we start + // `android_main()`). let jni_activity = unsafe { env.as_cast_raw::>(&jni_activity)? }; let (app_asset_manager, main_callbacks) = @@ -910,11 +935,12 @@ fn rust_glue_entry( }; let app = AndroidApp::new( - rust_glue.clone(), jvm.clone(), - app_asset_manager, main_looper, main_callbacks, + app_asset_manager, + rust_glue.clone(), + &jni_activity, ); rust_glue.notify_main_thread_running(); diff --git a/android-activity/src/native_activity/mod.rs b/android-activity/src/native_activity/mod.rs index 18c7eef..fab67be 100644 --- a/android-activity/src/native_activity/mod.rs +++ b/android-activity/src/native_activity/mod.rs @@ -5,6 +5,7 @@ use std::ptr; use std::sync::{Arc, Mutex, RwLock, Weak}; use std::time::Duration; +use jni::objects::JObject; use jni::JavaVM; use libc::c_void; use log::{error, trace}; @@ -63,11 +64,12 @@ impl StateLoader<'_> { impl AndroidApp { pub(crate) fn new( - native_activity: NativeActivityGlue, jvm: JavaVM, - app_asset_manager: AssetManager, main_looper: ndk::looper::ForeignLooper, main_callbacks: MainCallbacks, + app_asset_manager: AssetManager, + native_activity: NativeActivityGlue, + jni_activity: &JObject, ) -> Self { jvm.with_local_frame(10, |env| -> jni::errors::Result<_> { if let Err(err) = crate::input::jni_init(env) { @@ -80,16 +82,25 @@ impl AndroidApp { ); ndk::looper::ForeignLooper::from_ptr(ptr::NonNull::new(ptr).unwrap()) }; + + // The global reference in `ANativeActivity` is only guaranteed to be valid until + // `onDestroy` returns, so we create our own global reference that we can guarantee will + // remain valid until `AndroidApp` is dropped. + let activity = env + .new_global_ref(jni_activity) + .expect("Failed to create global ref for Activity instance"); + let app = Self { inner: Arc::new(RwLock::new(AndroidAppInner { jvm: jvm.clone(), - native_activity, - looper, main_looper, main_callbacks, + app_asset_manager, + native_activity, + activity, + looper, key_maps: Mutex::new(HashMap::new()), input_receiver: Mutex::new(None), - app_asset_manager, })), }; @@ -122,6 +133,8 @@ pub(crate) struct AndroidAppInner { pub(crate) native_activity: NativeActivityGlue, + activity: jni::refs::Global>, + main_callbacks: MainCallbacks, /// Looper associated with the Rust `android_main` thread @@ -151,17 +164,11 @@ pub(crate) struct AndroidAppInner { } impl AndroidAppInner { - pub(crate) fn vm_as_ptr(&self) -> *mut c_void { - unsafe { (*self.native_activity.activity).vm as _ } - } - pub(crate) fn activity_as_ptr(&self) -> *mut c_void { - // "clazz" is a completely bogus name; this is the _instance_ not class pointer - unsafe { (*self.native_activity.activity).clazz as _ } - } - - pub(crate) fn native_activity(&self) -> *const ndk_sys::ANativeActivity { - self.native_activity.activity + // Note: The global reference in `ANativeActivity::clazz` (misnomer for instance reference) + // is only guaranteed to be valid until `onDestroy` returns, so we have our own global + // reference that we can instead guarantee will remain valid until `AndroidApp` is dropped. + self.activity.as_raw() as *mut c_void } pub(crate) fn looper_as_ptr(&self) -> *mut ndk_sys::ALooper { @@ -336,7 +343,13 @@ impl AndroidAppInner { add_flags: WindowManagerFlags, remove_flags: WindowManagerFlags, ) { - let na = self.native_activity(); + let guard = self.native_activity.mutex.lock().unwrap(); + let na = guard.activity; + if na.is_null() { + log::error!("Can't set window flags after NativeActivity has been destroyed"); + return; + } + let na_mut = na as *mut ndk_sys::ANativeActivity; unsafe { ndk_sys::ANativeActivity_setWindowFlags( @@ -349,7 +362,12 @@ impl AndroidAppInner { // TODO: move into a trait pub fn show_soft_input(&self, show_implicit: bool) { - let na = self.native_activity(); + let guard = self.native_activity.mutex.lock().unwrap(); + let na = guard.activity; + if na.is_null() { + log::error!("Can't show soft input after NativeActivity has been destroyed"); + return; + } unsafe { let flags = if show_implicit { ndk_sys::ANATIVEACTIVITY_SHOW_SOFT_INPUT_IMPLICIT @@ -362,7 +380,12 @@ impl AndroidAppInner { // TODO: move into a trait pub fn hide_soft_input(&self, hide_implicit_only: bool) { - let na = self.native_activity(); + let guard = self.native_activity.mutex.lock().unwrap(); + let na = guard.activity; + if na.is_null() { + log::error!("Can't hide soft input after NativeActivity has been destroyed"); + return; + } unsafe { let flags = if hide_implicit_only { ndk_sys::ANATIVEACTIVITY_HIDE_SOFT_INPUT_IMPLICIT_ONLY @@ -447,17 +470,32 @@ impl AndroidAppInner { } pub fn internal_data_path(&self) -> Option { - let na = self.native_activity(); + let guard = self.native_activity.mutex.lock().unwrap(); + let na = guard.activity; + if na.is_null() { + log::error!("Can't get internal data path after NativeActivity has been destroyed"); + return None; + } unsafe { util::try_get_path_from_ptr((*na).internalDataPath) } } pub fn external_data_path(&self) -> Option { - let na = self.native_activity(); + let guard = self.native_activity.mutex.lock().unwrap(); + let na = guard.activity; + if na.is_null() { + log::error!("Can't get external data path after NativeActivity has been destroyed"); + return None; + } unsafe { util::try_get_path_from_ptr((*na).externalDataPath) } } pub fn obb_path(&self) -> Option { - let na = self.native_activity(); + let guard = self.native_activity.mutex.lock().unwrap(); + let na = guard.activity; + if na.is_null() { + log::error!("Can't get OBB path after NativeActivity has been destroyed"); + return None; + } unsafe { util::try_get_path_from_ptr((*na).obbPath) } } }