rust-mobile/android-activity

Possible unsoundness in `AssetManager`

blaind opened this issue · 3 comments

blaind commented

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 commented

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 commented

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.