diff --git a/ndk/CHANGELOG.md b/ndk/CHANGELOG.md index 3a41306d..873bbd14 100644 --- a/ndk/CHANGELOG.md +++ b/ndk/CHANGELOG.md @@ -20,6 +20,7 @@ - **Breaking:** media_codec: Add support for asynchronous notification callbacks. (#410) - 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) - **Breaking:** Upgrade to [`ndk-sys 0.5.0`](../ndk-sys/CHANGELOG.md#050-beta0-2023-08-15). (#420) # 0.7.0 (2022-07-24) diff --git a/ndk/src/hardware_buffer.rs b/ndk/src/hardware_buffer.rs index d7a7b013..8f0b7f5b 100644 --- a/ndk/src/hardware_buffer.rs +++ b/ndk/src/hardware_buffer.rs @@ -9,7 +9,15 @@ 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::{ + raw::c_void, + // TODO: Import from std::os::fd::{} since Rust 1.66 + unix::io::{AsRawFd, BorrowedFd, FromRawFd, IntoRawFd, OwnedFd}, + }, + ptr::NonNull, }; #[derive(Copy, Clone, Debug, PartialEq, Eq)] @@ -263,10 +271,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.map_or(-1, IntoRawFd::into_raw_fd); let rect = match rect { Some(rect) => &rect, None => std::ptr::null(), @@ -287,10 +295,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.map_or(-1, IntoRawFd::into_raw_fd); let rect = match rect { Some(rect) => &rect, None => std::ptr::null(), @@ -339,10 +347,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.map_or(-1, IntoRawFd::into_raw_fd); let rect = match rect { Some(rect) => &rect, None => std::ptr::null(), @@ -373,21 +381,24 @@ 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`] and + /// [`std::os::unix::net::UnixStream`] in Rust and have a corresponding + /// [`std::os::unix::io::AsFd::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,13 @@ 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`] and + /// [`std::os::unix::net::UnixStream`] in Rust and have a corresponding + /// [`std::os::unix::io::AsFd::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 4f44234b..fb707df3 100644 --- a/ndk/src/looper.rs +++ b/ndk/src/looper.rs @@ -12,9 +12,9 @@ use bitflags::bitflags; 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; @@ -42,7 +42,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. @@ -52,7 +52,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 [`ForeignLooper::add_fd()`] or [`ForeignLooper::add_fd_with_callback()`]. + fd: BorrowedFd<'fd>, events: FdEvent, data: *mut c_void, }, @@ -67,7 +70,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, @@ -83,9 +86,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 in milliseconds. + /// 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), @@ -94,7 +99,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, @@ -105,17 +112,17 @@ impl ThreadLooper { /// Polls the looper, blocking on processing an event. #[inline] - pub fn poll_once(&self) -> Result { + pub fn poll_once(&self) -> Result, LooperError> { self.poll_once_ms(-1) } - /// Polls the looper, blocking on processing an event, but with a timeout. Give a timeout of 0 - /// to make this non-blocking. + /// Polls the looper, blocking on processing an event, but with a timeout. Give a timeout of + /// [`Duration::ZERO`] to make this non-blocking. /// /// It panics if the timeout is larger than expressible as an [`i32`] of milliseconds (roughly 25 /// days). #[inline] - pub fn poll_once_timeout(&self, timeout: Duration) -> Result { + pub fn poll_once_timeout(&self, timeout: Duration) -> Result, LooperError> { self.poll_once_ms( timeout .as_millis() @@ -124,9 +131,13 @@ impl ThreadLooper { ) } - fn poll_all_ms(&self, ms: i32) -> Result { - let mut fd: RawFd = -1; - let mut events: i32 = -1; + /// Repeatedly polls the looper, blocking on processing an event, but with a timeout in + /// milliseconds. Give a timeout of `0` to make this non-blocking. + /// + /// This function will never return [`Poll::Callback`]. + fn poll_all_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_pollAll(ms, &mut fd, &mut events, &mut data) } { ffi::ALOOPER_POLL_WAKE => Ok(Poll::Wake), @@ -134,7 +145,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, @@ -147,19 +160,19 @@ impl ThreadLooper { /// /// This function will never return [`Poll::Callback`]. #[inline] - pub fn poll_all(&self) -> Result { + pub fn poll_all(&self) -> Result, LooperError> { self.poll_all_ms(-1) } - /// Repeatedly polls the looper, blocking on processing an event, but with a timeout. Give a - /// timeout of 0 to make this non-blocking. + /// Repeatedly polls the looper, blocking on processing an event, but with a timeout. Give a + /// timeout of [`Duration::ZERO`] to make this non-blocking. /// /// This function will never return [`Poll::Callback`]. /// /// It panics if the timeout is larger than expressible as an [`i32`] of milliseconds (roughly 25 /// days). #[inline] - pub fn poll_all_timeout(&self, timeout: Duration) -> Result { + pub fn poll_all_timeout(&self, timeout: Duration) -> Result, LooperError> { self.poll_all_ms( timeout .as_millis() @@ -183,7 +196,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 {} @@ -208,7 +221,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. @@ -217,14 +231,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 } @@ -237,13 +251,18 @@ 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()], and for however long the caller wishes to use this file + /// descriptor when it is returned in [`Poll::Event::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, @@ -251,7 +270,7 @@ impl ForeignLooper { match unsafe { ffi::ALooper_addFd( self.ptr.as_ptr(), - fd, + fd.as_raw_fd(), ident, events.bits() as i32, None, @@ -274,20 +293,25 @@ 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()] or by returning [`false`] from the callback, and for + /// however long the caller wishes to use this file descriptor inside and after the callback. + 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); } @@ -298,7 +322,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::), @@ -328,8 +352,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 dd464f08..c1c74eba 100644 --- a/ndk/src/media/image_reader.rs +++ b/ndk/src/media/image_reader.rs @@ -16,7 +16,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::{FromRawFd, IntoRawFd, OwnedFd}; #[cfg(feature = "api-level-26")] use crate::hardware_buffer::{HardwareBuffer, HardwareBufferUsage}; @@ -213,11 +214,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()) @@ -227,7 +232,7 @@ impl ImageReader { Ok(match fence.assume_init() { -1 => (image, None), - fence => (image, Some(fence)), + fence => (image, Some(unsafe { OwnedFd::from_raw_fd(fence) })), }) } @@ -243,11 +248,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()) @@ -257,7 +266,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) })), }) } } @@ -357,8 +366,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.into_raw_fd()) }; std::mem::forget(self); } }