Skip to content

Implement safe io #871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 71 additions & 8 deletions gio/src/unix_fd_list.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
// Take a look at the license at the top of the repository in the LICENSE file.

#[cfg(unix)]
use std::os::unix::io::{AsFd, AsRawFd, FromRawFd, IntoRawFd, OwnedFd, RawFd};
use std::os::unix::io::{AsFd, AsRawFd, BorrowedFd, FromRawFd, IntoRawFd, OwnedFd, RawFd};
use std::{mem, ptr};

use glib::{prelude::*, translate::*};
#[cfg(all(not(unix), docsrs))]
use socket::{AsFd, AsRawFd, FromRawFd, IntoRawFd, OwnedFd, RawFd};
use socket::{AsFd, AsRawFd, BorrowedFd, FromRawFd, IntoRawFd, OwnedFd, RawFd};

use crate::{ffi, UnixFDList};

Expand Down Expand Up @@ -69,16 +69,79 @@ pub trait UnixFDListExtManual: IsA<UnixFDList> + Sized {
}

#[doc(alias = "g_unix_fd_list_steal_fds")]
fn steal_fds(&self) -> Vec<RawFd> {
fn steal_fds(&self) -> FdArray {
unsafe {
let mut length = mem::MaybeUninit::uninit();
let ret = FromGlibContainer::from_glib_full_num(
ffi::g_unix_fd_list_steal_fds(self.as_ref().to_glib_none().0, length.as_mut_ptr()),
length.assume_init() as usize,
);
ret

let ptr =
ffi::g_unix_fd_list_steal_fds(self.as_ref().to_glib_none().0, length.as_mut_ptr());

FdArray {
ptr: ptr::NonNull::new(ptr).unwrap(),
len: length.assume_init() as usize,
}
}
}
}

impl<O: IsA<UnixFDList>> UnixFDListExtManual for O {}

pub struct FdArray {
ptr: ptr::NonNull<libc::c_int>,
len: usize,
}

pub struct FdIterator {
ptr: ptr::NonNull<libc::c_int>,
len: usize,
}

impl Iterator for FdIterator {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to also implement size_hint() and DoubleEndedIterator and ExactSizeIterator and FusedIterator here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In gstreamer-rs I have this macro for defining iterators like this because they happen quite often. Might be helpful here

type Item = OwnedFd;

fn next(&mut self) -> Option<Self::Item> {
if self.len > 0 {
let current = self.ptr.as_ptr();
if self.len > 1 {
let next = unsafe { self.ptr.as_ptr().add(1) };
self.ptr = ptr::NonNull::new(next).unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work. You'll have to keep track of the original pointer so it can be freed on Drop (of the FdIterator)

}
self.len -= 1;
Some(unsafe { OwnedFd::from_raw_fd(*current) })
} else {
None
}
}
}

impl Drop for FdArray {
fn drop(&mut self) {
while self.len > 0 {
unsafe {
let current = self.ptr.as_ptr();
libc::close(*current);
}
if self.len > 1 {
let next = unsafe { self.ptr.as_ptr().add(1) };
self.ptr = ptr::NonNull::new(next).unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work, you'll also have to free the original pointer

}
self.len -= 1;
}
}
}

impl std::iter::IntoIterator for FdArray {
type Item = OwnedFd;
type IntoIter = FdIterator;

fn into_iter(mut self) -> Self::IntoIter {
let len = std::mem::take(&mut self.len);
FdIterator { len, ptr: self.ptr }
}
}

impl FdArray {
pub fn as_slice(&self) -> &[BorrowedFd<'_>] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you didn't implement Deref<Target = [OwnedFd]>, it would be nice if you implemented a non-destructive iter() too. Personally, I would just implement Deref :)

unsafe { std::slice::from_raw_parts(self.ptr.as_ptr() as *const BorrowedFd, self.len) }
}
}
14 changes: 1 addition & 13 deletions gio/src/unix_input_stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,13 @@ impl UnixInputStream {
// rustdoc-stripper-ignore-next
/// Creates a new [`Self`] that takes ownership of the passed in fd.
#[doc(alias = "g_unix_input_stream_new")]
pub fn take_fd(fd: OwnedFd) -> UnixInputStream {
pub fn from_fd(fd: OwnedFd) -> UnixInputStream {
let fd = fd.into_raw_fd();
let close_fd = true.into_glib();
unsafe {
InputStream::from_glib_full(ffi::g_unix_input_stream_new(fd, close_fd)).unsafe_cast()
}
}

// rustdoc-stripper-ignore-next
/// Creates a new [`Self`] that does not take ownership of the passed in fd.
///
/// # Safety
/// You may only close the fd if all references to this stream have been dropped.
#[doc(alias = "g_unix_input_stream_new")]
pub unsafe fn with_fd<T: AsRawFd>(fd: T) -> UnixInputStream {
let fd = fd.as_raw_fd();
let close_fd = false.into_glib();
InputStream::from_glib_full(ffi::g_unix_input_stream_new(fd, close_fd)).unsafe_cast()
}
}

impl AsRawFd for UnixInputStream {
Expand Down
14 changes: 1 addition & 13 deletions gio/src/unix_output_stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,13 @@ impl UnixOutputStream {
// rustdoc-stripper-ignore-next
/// Creates a new [`Self`] that takes ownership of the passed in fd.
#[doc(alias = "g_unix_output_stream_new")]
pub fn take_fd(fd: OwnedFd) -> UnixOutputStream {
pub fn from_fd(fd: OwnedFd) -> UnixOutputStream {
let fd = fd.into_raw_fd();
let close_fd = true.into_glib();
unsafe {
OutputStream::from_glib_full(ffi::g_unix_output_stream_new(fd, close_fd)).unsafe_cast()
}
}

// rustdoc-stripper-ignore-next
/// Creates a new [`Self`] that does not take ownership of the passed in fd.
///
/// # Safety
/// You may only close the fd if all references to this stream have been dropped.
#[doc(alias = "g_unix_output_stream_new")]
pub unsafe fn with_fd<T: AsRawFd>(fd: T) -> UnixOutputStream {
let fd = fd.as_raw_fd();
let close_fd = false.into_glib();
OutputStream::from_glib_full(ffi::g_unix_output_stream_new(fd, close_fd)).unsafe_cast()
}
}

impl AsRawFd for UnixOutputStream {
Expand Down
4 changes: 2 additions & 2 deletions glib/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ pub fn spawn_async_with_fds<P: AsRef<std::path::Path>>(
#[cfg(not(windows))]
#[cfg_attr(docsrs, doc(cfg(not(windows))))]
#[doc(alias = "g_spawn_async_with_pipes")]
pub fn spawn_async_with_pipes<
pub unsafe fn spawn_async_with_pipes<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make this use the new traits?

P: AsRef<std::path::Path>,
T: FromRawFd,
U: FromRawFd,
Expand Down Expand Up @@ -241,7 +241,7 @@ pub fn compute_checksum_for_string(

#[cfg(unix)]
#[doc(alias = "g_unix_open_pipe")]
pub fn unix_open_pipe(flags: i32) -> Result<(RawFd, RawFd), Error> {
pub unsafe fn unix_open_pipe(flags: i32) -> Result<(RawFd, RawFd), Error> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably return OwnedFd?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that compatible with the flags argument that allows setting O_CLOEXEC/FD_CLOEXEC?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a prior art rustix::fd::open also returns OwnedFd and allows passing in the CLOEXEC flag.

unsafe {
let mut fds = [0, 2];
let mut error = ptr::null_mut();
Expand Down
44 changes: 22 additions & 22 deletions glib/src/source.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
// Take a look at the license at the top of the repository in the LICENSE file.

#[cfg(unix)]
use std::os::unix::io::RawFd;
use std::os::unix::io::{AsFd, AsRawFd, BorrowedFd, RawFd};
use std::{cell::RefCell, mem::transmute, num::NonZeroU32, time::Duration};

use crate::ffi::{self, gboolean, gpointer};
#[cfg(all(not(unix), docsrs))]
use libc::c_int as RawFd;

#[cfg(unix)]
use crate::IOCondition;
Expand Down Expand Up @@ -154,33 +152,35 @@ fn into_raw_child_watch_local<F: FnMut(Pid, i32) + 'static>(func: F) -> gpointer
#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
unsafe extern "C" fn trampoline_unix_fd<
F: FnMut(RawFd, IOCondition) -> ControlFlow + Send + 'static,
F: FnMut(BorrowedFd, IOCondition) -> ControlFlow + Send + 'static,
>(
fd: i32,
raw_fd: i32,
condition: ffi::GIOCondition,
func: gpointer,
) -> gboolean {
let func: &RefCell<F> = &*(func as *const RefCell<F>);
let fd = BorrowedFd::borrow_raw(raw_fd);
(*func.borrow_mut())(fd, from_glib(condition)).into_glib()
}

#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
unsafe extern "C" fn trampoline_unix_fd_local<
F: FnMut(RawFd, IOCondition) -> ControlFlow + 'static,
F: FnMut(BorrowedFd, IOCondition) -> ControlFlow + 'static,
>(
fd: i32,
raw_fd: i32,
condition: ffi::GIOCondition,
func: gpointer,
) -> gboolean {
let func: &ThreadGuard<RefCell<F>> = &*(func as *const ThreadGuard<RefCell<F>>);
let fd = BorrowedFd::borrow_raw(raw_fd);
(*func.get_ref().borrow_mut())(fd, from_glib(condition)).into_glib()
}

#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
unsafe extern "C" fn destroy_closure_unix_fd<
F: FnMut(RawFd, IOCondition) -> ControlFlow + Send + 'static,
F: FnMut(BorrowedFd, IOCondition) -> ControlFlow + Send + 'static,
>(
ptr: gpointer,
) {
Expand All @@ -190,7 +190,7 @@ unsafe extern "C" fn destroy_closure_unix_fd<
#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
unsafe extern "C" fn destroy_closure_unix_fd_local<
F: FnMut(RawFd, IOCondition) -> ControlFlow + 'static,
F: FnMut(BorrowedFd, IOCondition) -> ControlFlow + 'static,
>(
ptr: gpointer,
) {
Expand All @@ -199,7 +199,7 @@ unsafe extern "C" fn destroy_closure_unix_fd_local<

#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
fn into_raw_unix_fd<F: FnMut(RawFd, IOCondition) -> ControlFlow + Send + 'static>(
fn into_raw_unix_fd<F: FnMut(BorrowedFd, IOCondition) -> ControlFlow + Send + 'static>(
func: F,
) -> gpointer {
let func: Box<RefCell<F>> = Box::new(RefCell::new(func));
Expand All @@ -208,7 +208,7 @@ fn into_raw_unix_fd<F: FnMut(RawFd, IOCondition) -> ControlFlow + Send + 'static

#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
fn into_raw_unix_fd_local<F: FnMut(RawFd, IOCondition) -> ControlFlow + 'static>(
fn into_raw_unix_fd_local<F: FnMut(BorrowedFd, IOCondition) -> ControlFlow + 'static>(
func: F,
) -> gpointer {
let func: Box<ThreadGuard<RefCell<F>>> = Box::new(ThreadGuard::new(RefCell::new(func)));
Expand Down Expand Up @@ -872,14 +872,14 @@ where
/// The default main loop almost always is the main loop of the main thread.
/// Thus, the closure is called on the main thread.
#[doc(alias = "g_unix_fd_add_full")]
pub fn unix_fd_add<F>(fd: RawFd, condition: IOCondition, func: F) -> SourceId
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these functions all need to be unsafe actually. impl AsFd is correct but it's up to the caller to ensure that the fd is valid long enough

pub fn unix_fd_add<F>(fd: impl AsFd, condition: IOCondition, func: F) -> SourceId
where
F: FnMut(RawFd, IOCondition) -> ControlFlow + Send + 'static,
F: FnMut(BorrowedFd, IOCondition) -> ControlFlow + Send + 'static,
{
unsafe {
from_glib(ffi::g_unix_fd_add_full(
ffi::G_PRIORITY_DEFAULT,
fd,
fd.as_fd().as_raw_fd(),
condition.into_glib(),
Some(trampoline_unix_fd::<F>),
into_raw_unix_fd(func),
Expand Down Expand Up @@ -907,7 +907,7 @@ pub fn unix_fd_add_full<F>(
func: F,
) -> SourceId
where
F: FnMut(RawFd, IOCondition) -> ControlFlow + Send + 'static,
F: FnMut(BorrowedFd, IOCondition) -> ControlFlow + Send + 'static,
{
unsafe {
from_glib(ffi::g_unix_fd_add_full(
Expand Down Expand Up @@ -939,9 +939,9 @@ where
/// This function panics if called from a different thread than the one that
/// owns the main context.
#[doc(alias = "g_unix_fd_add_full")]
pub fn unix_fd_add_local<F>(fd: RawFd, condition: IOCondition, func: F) -> SourceId
pub fn unix_fd_add_local<F>(fd: impl AsFd, condition: IOCondition, func: F) -> SourceId
where
F: FnMut(RawFd, IOCondition) -> ControlFlow + 'static,
F: FnMut(BorrowedFd, IOCondition) -> ControlFlow + 'static,
{
unsafe {
let context = MainContext::default();
Expand All @@ -950,7 +950,7 @@ where
.expect("default main context already acquired by another thread");
from_glib(ffi::g_unix_fd_add_full(
ffi::G_PRIORITY_DEFAULT,
fd,
fd.as_fd().as_raw_fd(),
condition.into_glib(),
Some(trampoline_unix_fd_local::<F>),
into_raw_unix_fd_local(func),
Expand Down Expand Up @@ -984,7 +984,7 @@ pub fn unix_fd_add_local_full<F>(
func: F,
) -> SourceId
where
F: FnMut(RawFd, IOCondition) -> ControlFlow + 'static,
F: FnMut(BorrowedFd, IOCondition) -> ControlFlow + 'static,
{
unsafe {
let context = MainContext::default();
Expand Down Expand Up @@ -1230,17 +1230,17 @@ where
/// until it returns `ControlFlow::Break`.
#[doc(alias = "g_unix_fd_source_new")]
pub fn unix_fd_source_new<F>(
fd: RawFd,
fd: impl AsFd,
condition: IOCondition,
name: Option<&str>,
priority: Priority,
func: F,
) -> Source
where
F: FnMut(RawFd, IOCondition) -> ControlFlow + Send + 'static,
F: FnMut(BorrowedFd, IOCondition) -> ControlFlow + Send + 'static,
{
unsafe {
let source = ffi::g_unix_fd_source_new(fd, condition.into_glib());
let source = ffi::g_unix_fd_source_new(fd.as_fd().as_raw_fd(), condition.into_glib());
ffi::g_source_set_callback(
source,
Some(transmute::<
Expand Down
Loading