Ensure AndroidAppWaker owns an ALooper reference

This ensures we call `ALooper_acquire` before `create_waker()` wraps the
Looper pointer with `AndroidAppWaker` and it also ensures that
`::clone()` and `::drop()` call `ALooper_acquire()` and
`ALooper_release()` respectively.

Contrary to what the comment for the `looper` member said previously, it
was not safe to assume that the application's looper pointer had a
`'static` lifetime.

The looper pointer would only be valid up until `android_main` returns,
but unlike a traditional `main()` function an `android_main()` runs
with respect to an `Activity` lifecycle and not a process lifecycle.

It's technically possible for `android_main()` to return (at which point
any looper stored in `'static` storage would have previously become an
invalid pointer) and then JNI could be used to re-enter Rust and
potentially try and dereference that invalid pointer.

This adds a shared implementation of `AndroidAppWaker` to `src/waker.rs`
instead of having each backend implement `AndroidAppWaker`.

Fixes: #226

Author: rib

android-activity/src/game_activity/mod.rs

@@ -15,7 +15,7 @@ use log::{error, trace};
 
 use jni::sys::*;
 
-use ndk_sys::{ALooper, ALooper_pollOnce, ALooper_wake};
+use ndk_sys::ALooper_pollOnce;
 
 use ndk::asset::AssetManager;
 use ndk::configuration::Configuration;
@@ -27,7 +27,8 @@ use crate::util::{
     try_get_path_from_ptr,
 };
 use crate::{
-    AndroidApp, ConfigurationRef, InputStatus, MainEvent, PollEvent, Rect, WindowManagerFlags,
+    AndroidApp, AndroidAppWaker, ConfigurationRef, InputStatus, MainEvent, PollEvent, Rect,
+    WindowManagerFlags,
 };
 
 mod ffi;
@@ -105,24 +106,6 @@ impl StateLoader<'_> {
     }
 }
 
-#[derive(Clone)]
-pub struct AndroidAppWaker {
-    // The looper pointer is owned by the android_app and effectively
-    // has a 'static lifetime, and the ALooper_wake C API is thread
-    // safe, so this can be cloned safely and is send + sync safe
-    looper: NonNull<ALooper>,
-}
-unsafe impl Send for AndroidAppWaker {}
-unsafe impl Sync for AndroidAppWaker {}
-
-impl AndroidAppWaker {
-    pub fn wake(&self) {
-        unsafe {
-            ALooper_wake(self.looper.as_ptr());
-        }
-    }
-}
-
 impl AndroidApp {
     pub(crate) unsafe fn from_ptr(ptr: NonNull<ffi::android_app>, jvm: jni::JavaVM) -> Self {
         // We attach to the thread before creating the AndroidApp
@@ -619,13 +602,10 @@ impl AndroidAppInner {
     }
 
     pub fn create_waker(&self) -> AndroidAppWaker {
+        // Safety: we know that the app and looper pointers are valid
         unsafe {
-            // From the application's pov we assume the app_ptr and looper pointer
-            // have static lifetimes and we can safely assume they are never NULL.
             let app_ptr = self.native_app.as_ptr();
-            AndroidAppWaker {
-                looper: NonNull::new_unchecked((*app_ptr).looper),
-            }
+            AndroidAppWaker::new((*app_ptr).looper)
         }
     }
 

android-activity/src/lib.rs

@@ -187,6 +187,9 @@ mod util;
 
 mod jni_utils;
 
+mod waker;
+pub use waker::AndroidAppWaker;
+
 /// A rectangle with integer edge coordinates. Used to represent window insets, for example.
 #[derive(Clone, Debug, Default, Eq, PartialEq)]
 pub struct Rect {
@@ -344,7 +347,6 @@ pub enum InputStatus {
 }
 
 use activity_impl::AndroidAppInner;
-pub use activity_impl::AndroidAppWaker;
 
 bitflags! {
     /// Flags for [`AndroidApp::set_window_flags`]

android-activity/src/native_activity/mod.rs

@@ -14,7 +14,8 @@ use ndk::{asset::AssetManager, native_window::NativeWindow};
 
 use crate::error::InternalResult;
 use crate::{
-    util, AndroidApp, ConfigurationRef, InputStatus, MainEvent, PollEvent, Rect, WindowManagerFlags,
+    util, AndroidApp, AndroidAppWaker, ConfigurationRef, InputStatus, MainEvent, PollEvent, Rect,
+    WindowManagerFlags,
 };
 
 pub mod input;
@@ -60,31 +61,6 @@ impl StateLoader<'_> {
     }
 }
 
-/// A means to wake up the main thread while it is blocked waiting for I/O
-#[derive(Clone)]
-pub struct AndroidAppWaker {
-    // The looper pointer is owned by the android_app and effectively
-    // has a 'static lifetime, and the ALooper_wake C API is thread
-    // safe, so this can be cloned safely and is send + sync safe
-    looper: NonNull<ndk_sys::ALooper>,
-}
-unsafe impl Send for AndroidAppWaker {}
-unsafe impl Sync for AndroidAppWaker {}
-
-impl AndroidAppWaker {
-    /// Interrupts the main thread if it is blocked within [`AndroidApp::poll_events()`]
-    ///
-    /// If [`AndroidApp::poll_events()`] is interrupted it will invoke the poll
-    /// callback with a [PollEvent::Wake][wake_event] event.
-    ///
-    /// [wake_event]: crate::PollEvent::Wake
-    pub fn wake(&self) {
-        unsafe {
-            ndk_sys::ALooper_wake(self.looper.as_ptr());
-        }
-    }
-}
-
 impl AndroidApp {
     pub(crate) fn new(native_activity: NativeActivityGlue, jvm: JavaVM) -> Self {
         jvm.with_local_frame(10, |env| -> jni::errors::Result<_> {
@@ -305,13 +281,8 @@ impl AndroidAppInner {
     }
 
     pub fn create_waker(&self) -> AndroidAppWaker {
-        unsafe {
-            // From the application's pov we assume the looper pointer has a static
-            // lifetimes and we can safely assume it is never NULL.
-            AndroidAppWaker {
-                looper: NonNull::new_unchecked(self.looper.ptr),
-            }
-        }
+        // Safety: we know that the looper is a valid, non-null pointer
+        unsafe { AndroidAppWaker::new(self.looper.ptr) }
     }
 
     pub fn config(&self) -> ConfigurationRef {

android-activity/src/waker.rs

@@ -0,0 +1,57 @@
+use std::ptr::NonNull;
+
+#[cfg(doc)]
+use crate::AndroidApp;
+
+/// A means to wake up the main thread while it is blocked waiting for I/O
+pub struct AndroidAppWaker {
+    looper: NonNull<ndk_sys::ALooper>,
+}
+
+impl Clone for AndroidAppWaker {
+    fn clone(&self) -> Self {
+        unsafe { ndk_sys::ALooper_acquire(self.looper.as_ptr()) }
+        Self {
+            looper: self.looper,
+        }
+    }
+}
+
+impl Drop for AndroidAppWaker {
+    fn drop(&mut self) {
+        unsafe { ndk_sys::ALooper_release(self.looper.as_ptr()) }
+    }
+}
+
+unsafe impl Send for AndroidAppWaker {}
+unsafe impl Sync for AndroidAppWaker {}
+
+impl AndroidAppWaker {
+    /// Acquire a ref to a looper as a means to be able to wake up the event loop
+    ///
+    /// # Safety
+    ///
+    /// The `ALooper` pointer must be valid and not null.
+    pub(crate) unsafe fn new(looper: *mut ndk_sys::ALooper) -> Self {
+        assert!(!looper.is_null(), "looper pointer must not be null");
+        unsafe {
+            // Give the waker its own reference to the looper
+            ndk_sys::ALooper_acquire(looper);
+            AndroidAppWaker {
+                looper: NonNull::new_unchecked(looper),
+            }
+        }
+    }
+
+    /// Interrupts the main thread if it is blocked within [`AndroidApp::poll_events()`]
+    ///
+    /// If [`AndroidApp::poll_events()`] is interrupted it will invoke the poll
+    /// callback with a [PollEvent::Wake][wake_event] event.
+    ///
+    /// [wake_event]: crate::PollEvent::Wake
+    pub fn wake(&self) {
+        unsafe {
+            ndk_sys::ALooper_wake(self.looper.as_ptr());
+        }
+    }
+}