rust-mobile / ndk

Rust bindings to the Android NDK
Apache License 2.0
1.14k stars 112 forks source link

ndk/media/image_reader: Don't assume ownership of `NativeWindow` #418

Closed MarijnS95 closed 1 year ago

MarijnS95 commented 1 year ago

When implementing proper ownership acquire() and release() semantics for NativeWindow in https://github.com/rust-mobile/ndk/pull/207, I thought I had checked all existing calls to NativeWindow::from_ptr() to make sure that they return a pointer that we get ownership over, and have to clean up ourselves. This turns out to not be the case for AImageReader_getWindow():

The ANativeWindow is managed by this image reader. Do NOT call ANativeWindow_release on it. Instead, use AImageReader_delete.

And can be trivially reproduced by creating an ImageReader and calling .get_window(). Dropping the NativeWindow is fine but subsequently dropping the ImageReader results in a NULL-pointer SEGFAULT.

For now calling clone_from_ptr() is enough to first acquire an extra reference on the pointer so that ownership remains balanced, but in the future we'd like to investigate a new non-owned NativeWindow type similar to HardwareBuffer.

As of writing the following calls to from_ptr() remain, that are all confirmed to transfer ownership and require cleanup via _release():

MarijnS95 commented 1 year ago

Yup, should be enough asserts there.