Possible unsoundness in `AssetManager`

[blaind]

Seems like the method below (in android-activity/src/game_activity/mod.rs) takes a pointer to the NativeAppGlue (self.native_app) struct, which may be dropped before the AssetManager itself.

pub fn asset_manager(&self) -> AssetManager {
    unsafe {
        let app_ptr = self.native_app.as_ptr();
        let am_ptr = NonNull::new_unchecked((*(*app_ptr).activity).assetManager);
        AssetManager::from_ptr(am_ptr)
    }
}

calling an asset manager method such as open is causing a SIGABRT with a message of FORTIFY: pthread_mutex_lock called on a destroyed mutex (0xb400007643074c50)

[MarijnS95]

This looks like a remnant from te horrible original ndk docs that I/we inherited. Currently AssetManager::from_ptr() says:

Create an AssetManager from a pointer

When nothing is created at all, nor is there a refcount to increment. The lifetime of this AssetManager purely depends on the input pointer, which is likely destroyed in native code.

Instead these # Safety docs should state that the caller is still responsible for managing the valid lifetime of the input pointer, and that it's simply wrapping a reference to an AssetManager.

Note that no pointer is taken "to the NativeAppGlue struct"; it's an independent pointer value that they set up for us (internally it may point to an offset within the same struct, but that's not a requirement).

[MarijnS95]

We have a similar function on NativeActivity that returns an owned instance of this struct: https://docs.rs/ndk/latest/ndk/native_activity/struct.NativeActivity.html#method.asset_manager

I'm thinking of giving it a PhantomData lifetime that's tied to &'this_lifetime self.

[MarijnS95]

Quickly hacked-together proposal: https://github.com/rust-mobile/ndk/compare/asset-lifetime

[rib]

while I'm hoping to prepare a 0.6.x release, I'm currently unsure of the best way to handle this issue without an API break.

According to the NDK docs, the underlying AAssetManager lifetime is tied to the corresponding Java AssetManager (you're required to keep VM reference to the AssetManager).

I would have guessed it should be relatively tricky to get into a situation where the reference for the AssetManager has been released, so I'm curious if there might be some other details involved in reproducing this. Looking at the GameActivity backend, it creates a global reference for the AssetManager that should only get dropped after your android_main returns. Since that doesn't mean your process will exit then it certainly is still possible to access an AssetManager that has been kept around too long.

As a hack to try and make the current API safe, we could potentially leak a global reference to the AssetManager the first time it gets called for the current Activity. Obviously that's not ideal but it could ensure that - even if you keep the AssetManager beyond android_main then the leaked JNI reference should keep the corresponding java object alive. So we would effectively leak one global reference for each time that android_main runs in your process - which for most applications currently is likely to be just once.

Otherwise, we could potentially make an ndk 0.9 release that updates AssetManager to also wrap a global JNI global reference, but that would likely also involve adding a jni dependency to ensure it's able to reliably release that reference on Drop

[rib]

Thinking about this further, I'm currently thinking that something similar to above is probably reasonable (not even particularly hacky because it actually better aligns with the API docs)..

We can clarify that AndroidApp::asset_manager() returns an Application context AssetManager (i.e. from Application.getAssets() not Activity.getAssets()) and we can make sure we do a OnceLock allocation of a global reference to guarantee that the pointer will be safe for a 'static lifetime instead of relying on a global JNI reference that currently gets deleted when android_main returns.

This actually aligns with the current API docs:

Queries the Asset Manager instance for the application

We can then later add an API like AndroidApp::activity_asset_manager() that will return some new AssetManager wrapper that will internally pair an AAssetManager pointer with an AssetManager JNI global reference that can be automatically deleted via Drop.

In practice, it's probably reasonable to guess that Rust applications that want the AssetManager aren't expecting to see any difference between the Application AssetManager vs the Activity AssetManager so this tweak in behaviour (which technically matches the documentation) is unlikely to break anything.