From b8aadfd248ed492f5e02399a4f3269f2193bbe8a Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Thu, 10 Aug 2023 14:49:46 +0200 Subject: [PATCH] ndk: Use `BorrowedFd` and `OwnedFd` to clarify ownership transitions Some functions consume a file descriptor (will close them on their own regard) or return ownership over a file descriptor (expect the caller to close it), but this is not always clarified in the documentation nor upheld by the caller. Use the new stabilized `BorrowedFd` and `OwnedFd` types since Rust 1.63 to clarify this in the API, noting that `OwnedFd` will instinctively `close()` the file descriptor on drop and doesn't implement `Copy` nor `Clone` (but does provide a `try_clone()` helper using `fcntl()` to create a new owned file descriptor if needed, and if possible by the kernel). For example, while not obvious from `AHardwareBuffer_lock()` docs (though there are hints in the [graphics sync docs]) the source for gralloc buffer locking many function calls down clarifies that the [`acquireFence` is indeed owned and will be closed]. The same [applies to `AImageReader` and its async aquire functions]. [graphics sync docs]: https://source.android.com/docs/core/graphics/sync [`acquireFence` is indeed owned and will be closed]: https://cs.android.com/android/platform/superproject/main/+/refs/heads/main:frameworks/native/libs/ui/Gralloc4.cpp;l=320-323;drc=34edaadf5297f2c066d2cb09a5cc9366dc35b24b [applies to `AImageReader` and its async aquire functions]: https://cs.android.com/android/platform/superproject/main/+/refs/heads/main:frameworks/av/media/ndk/NdkImageReader.cpp;l=498-501;drc=34edaadf5297f2c066d2cb09a5cc9366dc35b24b --- ndk/CHANGELOG.md | 1 + ndk/src/hardware_buffer.rs | 47 +++++++++++++++--------- ndk/src/looper.rs | 68 ++++++++++++++++++++++------------- ndk/src/media/image_reader.rs | 27 +++++++++----- 4 files changed, 92 insertions(+), 51 deletions(-) diff --git a/ndk/CHANGELOG.md b/ndk/CHANGELOG.md index 1ed88f93..a2650603 100644 --- a/ndk/CHANGELOG.md +++ b/ndk/CHANGELOG.md @@ -17,6 +17,7 @@ - **Breaking:** hardware_buffer_format: Add catch-all variant. (#407) - Add panic guards to callbacks. (#412) - looper: Add `remove_fd()` to unregister events/callbacks for a file descriptor. (#416) +- **Breaking:** Use `BorrowedFd` and `OwnedFd` to clarify possible ownership transitions. (#417) # 0.7.0 (2022-07-24) diff --git a/ndk/src/hardware_buffer.rs b/ndk/src/hardware_buffer.rs index d7a7b013..f8047752 100644 --- a/ndk/src/hardware_buffer.rs +++ b/ndk/src/hardware_buffer.rs @@ -9,7 +9,16 @@ use crate::utils::status_to_io_result; pub use super::hardware_buffer_format::HardwareBufferFormat; use jni_sys::{jobject, JNIEnv}; use std::{ - io::Result, mem::MaybeUninit, ops::Deref, os::raw::c_void, os::unix::io::RawFd, ptr::NonNull, + io::Result, + mem::MaybeUninit, + ops::Deref, + os::{ + fd::FromRawFd, + raw::c_void, + // TODO: Import from std::os::fd::{} since Rust 1.66 + unix::io::{AsRawFd, BorrowedFd, OwnedFd}, + }, + ptr::NonNull, }; #[derive(Copy, Clone, Debug, PartialEq, Eq)] @@ -263,10 +272,10 @@ impl HardwareBuffer { pub fn lock( &self, usage: HardwareBufferUsage, - fence: Option, + fence: Option, rect: Option, ) -> Result<*mut c_void> { - let fence = fence.unwrap_or(-1); + let fence = fence.as_ref().map_or(-1, AsRawFd::as_raw_fd); let rect = match rect { Some(rect) => &rect, None => std::ptr::null(), @@ -287,10 +296,10 @@ impl HardwareBuffer { pub fn lock_and_get_info( &self, usage: HardwareBufferUsage, - fence: Option, + fence: Option, rect: Option, ) -> Result { - let fence = fence.unwrap_or(-1); + let fence = fence.as_ref().map_or(-1, AsRawFd::as_raw_fd); let rect = match rect { Some(rect) => &rect, None => std::ptr::null(), @@ -339,10 +348,10 @@ impl HardwareBuffer { pub fn lock_planes( &self, usage: HardwareBufferUsage, - fence: Option, + fence: Option, rect: Option, ) -> Result { - let fence = fence.unwrap_or(-1); + let fence = fence.as_ref().map_or(-1, AsRawFd::as_raw_fd); let rect = match rect { Some(rect) => &rect, None => std::ptr::null(), @@ -373,21 +382,23 @@ impl HardwareBuffer { /// [`None`] if unlocking is already finished. The caller is responsible for closing the file /// descriptor once it's no longer needed. See [`unlock()`][Self::unlock()] for a variant that /// blocks instead. - pub fn unlock_async(&self) -> Result> { + pub fn unlock_async(&self) -> Result> { let fence = construct(|res| unsafe { ffi::AHardwareBuffer_unlock(self.as_ptr(), res) })?; Ok(match fence { -1 => None, - fence => Some(fence), + fence => Some(unsafe { OwnedFd::from_raw_fd(fence) }), }) } /// Receive a [`HardwareBuffer`] from an `AF_UNIX` socket. /// - /// `AF_UNIX` sockets are wrapped by [`std::os::unix::net::UnixListener`] in Rust. - pub fn recv_handle_from_unix_socket(socket_fd: RawFd) -> Result { + /// `AF_UNIX` sockets are wrapped by [`std::os::unix::net::UnixListener`] in Rust and have a + /// corresponding [`std::os::unix::net::UnixListener::as_fd()`] implementation. + pub fn recv_handle_from_unix_socket(socket_fd: BorrowedFd<'_>) -> Result { unsafe { - let ptr = - construct(|res| ffi::AHardwareBuffer_recvHandleFromUnixSocket(socket_fd, res))?; + let ptr = construct(|res| { + ffi::AHardwareBuffer_recvHandleFromUnixSocket(socket_fd.as_raw_fd(), res) + })?; Ok(Self::from_ptr(NonNull::new_unchecked(ptr))) } @@ -395,10 +406,12 @@ impl HardwareBuffer { /// Send the [`HardwareBuffer`] to an `AF_UNIX` socket. /// - /// `AF_UNIX` sockets are wrapped by [`std::os::unix::net::UnixListener`] in Rust. - pub fn send_handle_to_unix_socket(&self, socket_fd: RawFd) -> Result<()> { - let status = - unsafe { ffi::AHardwareBuffer_sendHandleToUnixSocket(self.as_ptr(), socket_fd) }; + /// `AF_UNIX` sockets are wrapped by [`std::os::unix::net::UnixListener`] in Rust and have a + /// corresponding [`std::os::unix::net::UnixListener::as_fd()`] implementation. + pub fn send_handle_to_unix_socket(&self, socket_fd: BorrowedFd<'_>) -> Result<()> { + let status = unsafe { + ffi::AHardwareBuffer_sendHandleToUnixSocket(self.as_ptr(), socket_fd.as_raw_fd()) + }; status_to_io_result(status, ()) } diff --git a/ndk/src/looper.rs b/ndk/src/looper.rs index ea4805bb..471baaf4 100644 --- a/ndk/src/looper.rs +++ b/ndk/src/looper.rs @@ -13,9 +13,9 @@ use bitflags::bitflags; use std::convert::TryInto; use std::mem::ManuallyDrop; use std::os::raw::c_void; -use std::os::unix::io::RawFd; +// TODO: Import from std::os::fd::{} since Rust 1.66 +use std::os::unix::io::{AsRawFd, BorrowedFd, RawFd}; use std::ptr; -use std::ptr::NonNull; use std::time::Duration; use thiserror::Error; @@ -43,7 +43,7 @@ bitflags! { /// The poll result from a [`ThreadLooper`]. #[derive(Debug)] -pub enum Poll { +pub enum Poll<'fd> { /// This looper was woken using [`ForeignLooper::wake()`] Wake, /// For [`ThreadLooper::poll_once*()`][ThreadLooper::poll_once()], an event was received and processed using a callback. @@ -53,7 +53,10 @@ pub enum Poll { /// An event was received Event { ident: i32, - fd: RawFd, + /// # Safety + /// The caller should guarantee that this file descriptor remains open after it was added + /// via [`ThreadLooper::add_fd()`] or [`ThreadLooper::add_fd_with_callback()`]. + fd: BorrowedFd<'fd>, events: FdEvent, data: *mut c_void, }, @@ -68,7 +71,7 @@ impl ThreadLooper { pub fn prepare() -> Self { unsafe { let ptr = ffi::ALooper_prepare(ffi::ALOOPER_PREPARE_ALLOW_NON_CALLBACKS as _); - let foreign = ForeignLooper::from_ptr(NonNull::new(ptr).expect("looper non null")); + let foreign = ForeignLooper::from_ptr(ptr::NonNull::new(ptr).expect("looper non null")); Self { _marker: std::marker::PhantomData, foreign, @@ -84,9 +87,11 @@ impl ThreadLooper { }) } - fn poll_once_ms(&self, ms: i32) -> Result { - let mut fd: RawFd = -1; - let mut events: i32 = -1; + /// Polls the looper, blocking on processing an event, but with a timeout. Give a timeout of 0 + /// to make this non-blocking. + fn poll_once_ms(&self, ms: i32) -> Result, LooperError> { + let mut fd = -1; + let mut events = -1; let mut data: *mut c_void = ptr::null_mut(); match unsafe { ffi::ALooper_pollOnce(ms, &mut fd, &mut events, &mut data) } { ffi::ALOOPER_POLL_WAKE => Ok(Poll::Wake), @@ -95,7 +100,9 @@ impl ThreadLooper { ffi::ALOOPER_POLL_ERROR => Err(LooperError), ident if ident >= 0 => Ok(Poll::Event { ident, - fd, + // SAFETY: Even though this FD at least shouldn't outlive self, a user could have + // closed it after calling add_fd or add_fd_with_callback. + fd: unsafe { BorrowedFd::borrow_raw(fd) }, events: FdEvent::from_bits(events as u32) .expect("poll event contains unknown bits"), data, @@ -126,8 +133,8 @@ impl ThreadLooper { } fn poll_all_ms(&self, ms: i32) -> Result { - let mut fd: RawFd = -1; - let mut events: i32 = -1; + let mut fd = -1; + let mut events = -1; let mut data: *mut c_void = ptr::null_mut(); match unsafe { ffi::ALooper_pollAll(ms, &mut fd, &mut events, &mut data) } { ffi::ALOOPER_POLL_WAKE => Ok(Poll::Wake), @@ -135,7 +142,9 @@ impl ThreadLooper { ffi::ALOOPER_POLL_ERROR => Err(LooperError), ident if ident >= 0 => Ok(Poll::Event { ident, - fd, + // SAFETY: Even though this FD at least shouldn't outlive self, a user could have + // closed it after calling add_fd or add_fd_with_callback. + fd: unsafe { BorrowedFd::borrow_raw(fd) }, events: FdEvent::from_bits(events as u32) .expect("poll event contains unknown bits"), data, @@ -184,7 +193,7 @@ impl ThreadLooper { /// [`ALooper *`]: https://developer.android.com/ndk/reference/group/looper#alooper #[derive(Debug)] pub struct ForeignLooper { - ptr: NonNull, + ptr: ptr::NonNull, } unsafe impl Send for ForeignLooper {} @@ -209,7 +218,8 @@ impl ForeignLooper { /// Returns the looper associated with the current thread, if any. #[inline] pub fn for_thread() -> Option { - NonNull::new(unsafe { ffi::ALooper_forThread() }).map(|ptr| unsafe { Self::from_ptr(ptr) }) + ptr::NonNull::new(unsafe { ffi::ALooper_forThread() }) + .map(|ptr| unsafe { Self::from_ptr(ptr) }) } /// Construct a [`ForeignLooper`] object from the given pointer. @@ -218,14 +228,14 @@ impl ForeignLooper { /// By calling this function, you guarantee that the pointer is a valid, non-null pointer to an /// NDK [`ffi::ALooper`]. #[inline] - pub unsafe fn from_ptr(ptr: NonNull) -> Self { + pub unsafe fn from_ptr(ptr: ptr::NonNull) -> Self { ffi::ALooper_acquire(ptr.as_ptr()); Self { ptr } } /// Returns a pointer to the NDK `ALooper` object. #[inline] - pub fn ptr(&self) -> NonNull { + pub fn ptr(&self) -> ptr::NonNull { self.ptr } @@ -238,13 +248,17 @@ impl ForeignLooper { /// /// See also [the NDK /// docs](https://developer.android.com/ndk/reference/group/looper.html#alooper_addfd). + /// + /// # Safety + /// The caller should guarantee that this file descriptor stays open until it is removed via + /// [`remove_fd()`][Self::remove_fd()]. // `ALooper_addFd` won't dereference `data`; it will only pass it on to the event. // Optionally dereferencing it there already enforces `unsafe` context. #[allow(clippy::not_unsafe_ptr_arg_deref)] pub fn add_fd( &self, - fd: RawFd, + fd: BorrowedFd<'_>, ident: i32, events: FdEvent, data: *mut c_void, @@ -252,7 +266,7 @@ impl ForeignLooper { match unsafe { ffi::ALooper_addFd( self.ptr.as_ptr(), - fd, + fd.as_raw_fd(), ident, events.bits() as i32, None, @@ -275,20 +289,24 @@ impl ForeignLooper { /// /// Note that this will leak a [`Box`] unless the callback returns [`false`] to unregister /// itself. - pub fn add_fd_with_callback bool>( + /// + /// # Safety + /// The caller should guarantee that this file descriptor stays open until it is removed via + /// [`remove_fd()`][Self::remove_fd()]. + pub fn add_fd_with_callback) -> bool>( &self, - fd: RawFd, + fd: BorrowedFd<'_>, events: FdEvent, callback: F, ) -> Result<(), LooperError> { - extern "C" fn cb_handler bool>( + extern "C" fn cb_handler) -> bool>( fd: RawFd, _events: i32, data: *mut c_void, ) -> i32 { unsafe { let mut cb = ManuallyDrop::new(Box::::from_raw(data as *mut _)); - let keep_registered = cb(fd); + let keep_registered = cb(BorrowedFd::borrow_raw(fd)); if !keep_registered { ManuallyDrop::into_inner(cb); } @@ -299,7 +317,7 @@ impl ForeignLooper { match unsafe { ffi::ALooper_addFd( self.ptr.as_ptr(), - fd, + fd.as_raw_fd(), ffi::ALOOPER_POLL_CALLBACK, events.bits() as i32, Some(cb_handler::), @@ -329,8 +347,8 @@ impl ForeignLooper { /// Note that unregistering a file descriptor with callback will leak a [`Box`] created in /// [`add_fd_with_callback()`][Self::add_fd_with_callback()]. Consider returning [`false`] /// from the callback instead to drop it. - pub fn remove_fd(&self, fd: RawFd) -> Result { - match unsafe { ffi::ALooper_removeFd(self.ptr.as_ptr(), fd) } { + pub fn remove_fd(&self, fd: BorrowedFd<'_>) -> Result { + match unsafe { ffi::ALooper_removeFd(self.ptr.as_ptr(), fd.as_raw_fd()) } { 1 => Ok(true), 0 => Ok(false), -1 => Err(LooperError), diff --git a/ndk/src/media/image_reader.rs b/ndk/src/media/image_reader.rs index bfd118c6..d660dab2 100644 --- a/ndk/src/media/image_reader.rs +++ b/ndk/src/media/image_reader.rs @@ -17,7 +17,8 @@ use std::{ }; #[cfg(feature = "api-level-26")] -use std::os::unix::io::RawFd; +// TODO: Import from std::os::fd::{} since Rust 1.66 +use std::os::unix::io::{AsRawFd, FromRawFd, OwnedFd}; #[cfg(feature = "api-level-26")] use crate::hardware_buffer::{HardwareBuffer, HardwareBufferUsage}; @@ -211,11 +212,15 @@ impl ImageReader { } } + /// Acquire the next [`Image`] from the image reader's queue asynchronously. + /// /// # Safety - /// If the returned file descriptor is not `None`, it must be awaited before attempting to access the Image returned. + /// If the returned file descriptor is not [`None`], it must be awaited before attempting to + /// access the [`Image`] returned. + /// /// #[cfg(feature = "api-level-26")] - pub unsafe fn acquire_next_image_async(&self) -> Result<(Image, Option)> { + pub unsafe fn acquire_next_image_async(&self) -> Result<(Image, Option)> { let mut fence = MaybeUninit::uninit(); let inner = construct_never_null(|res| { ffi::AImageReader_acquireNextImageAsync(self.as_ptr(), res, fence.as_mut_ptr()) @@ -225,7 +230,7 @@ impl ImageReader { Ok(match fence.assume_init() { -1 => (image, None), - fence => (image, Some(fence)), + fence => (image, Some(unsafe { OwnedFd::from_raw_fd(fence) })), }) } @@ -241,11 +246,15 @@ impl ImageReader { Ok(Some(Image { inner: res? })) } + /// Acquire the latest [`Image`] from the image reader's queue asynchronously, dropping older images. + /// /// # Safety - /// If the returned file descriptor is not `None`, it must be awaited before attempting to access the Image returned. + /// If the returned file descriptor is not [`None`], it must be awaited before attempting to + /// access the [`Image`] returned. + /// /// #[cfg(feature = "api-level-26")] - pub fn acquire_latest_image_async(&self) -> Result<(Image, Option)> { + pub fn acquire_latest_image_async(&self) -> Result<(Image, Option)> { let mut fence = MaybeUninit::uninit(); let inner = construct_never_null(|res| unsafe { ffi::AImageReader_acquireLatestImageAsync(self.as_ptr(), res, fence.as_mut_ptr()) @@ -255,7 +264,7 @@ impl ImageReader { Ok(match unsafe { fence.assume_init() } { -1 => (image, None), - fence => (image, Some(fence)), + fence => (image, Some(unsafe { OwnedFd::from_raw_fd(fence) })), }) } } @@ -355,8 +364,8 @@ impl Image { } #[cfg(feature = "api-level-26")] - pub fn delete_async(self, release_fence_fd: RawFd) { - unsafe { ffi::AImage_deleteAsync(self.as_ptr(), release_fence_fd) }; + pub fn delete_async(self, release_fence_fd: OwnedFd) { + unsafe { ffi::AImage_deleteAsync(self.as_ptr(), release_fence_fd.as_raw_fd()) }; std::mem::forget(self); } }