Skip to content
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

Implement safe io #871

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
25 changes: 12 additions & 13 deletions gio/src/desktop_app_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
#[cfg(all(unix, feature = "v2_58"))]
use std::boxed::Box as Box_;
#[cfg(all(unix, feature = "v2_58"))]
use std::os::unix::io::AsRawFd;
use std::os::unix::io::{AsFd, AsRawFd};
#[cfg(all(unix, feature = "v2_58"))]
use std::ptr;

Expand Down Expand Up @@ -54,21 +54,16 @@ mod sealed {
pub trait DesktopAppInfoExtManual: sealed::Sealed + IsA<DesktopAppInfo> {
#[cfg_attr(docsrs, doc(cfg(all(feature = "v2_58", unix))))]
#[doc(alias = "g_desktop_app_info_launch_uris_as_manager_with_fds")]
fn launch_uris_as_manager_with_fds<
P: IsA<AppLaunchContext>,
T: AsRawFd,
U: AsRawFd,
V: AsRawFd,
>(
fn launch_uris_as_manager_with_fds<P: IsA<AppLaunchContext>>(
A6GibKm marked this conversation as resolved.
Show resolved Hide resolved
&self,
uris: &[&str],
launch_context: Option<&P>,
spawn_flags: glib::SpawnFlags,
user_setup: Option<Box_<dyn FnOnce() + 'static>>,
pid_callback: Option<&mut dyn (FnMut(&DesktopAppInfo, glib::Pid))>,
stdin_fd: &mut T,
stdout_fd: &mut U,
stderr_fd: &mut V,
stdin_fd: Option<impl AsFd>,
stdout_fd: Option<impl AsFd>,
stderr_fd: Option<impl AsFd>,
) -> Result<(), Error> {
let user_setup_data: Box_<Option<Box_<dyn FnOnce() + 'static>>> = Box_::new(user_setup);
unsafe extern "C" fn user_setup_func(user_data: glib::ffi::gpointer) {
Expand Down Expand Up @@ -105,6 +100,10 @@ pub trait DesktopAppInfoExtManual: sealed::Sealed + IsA<DesktopAppInfo> {
let super_callback0: Box_<Option<Box_<dyn FnOnce() + 'static>>> = user_setup_data;
let super_callback1: &Option<&mut dyn (FnMut(&DesktopAppInfo, glib::Pid))> =
&pid_callback_data;

let stdin_raw_fd = stdin_fd.map_or(-1, |fd| fd.as_fd().as_raw_fd());
let stdout_raw_fd = stdout_fd.map_or(-1, |fd| fd.as_fd().as_raw_fd());
let stderr_raw_fd = stderr_fd.map_or(-1, |fd| fd.as_fd().as_raw_fd());
unsafe {
let mut error = ptr::null_mut();
let _ = ffi::g_desktop_app_info_launch_uris_as_manager_with_fds(
Expand All @@ -116,9 +115,9 @@ pub trait DesktopAppInfoExtManual: sealed::Sealed + IsA<DesktopAppInfo> {
Box_::into_raw(super_callback0) as *mut _,
pid_callback,
super_callback1 as *const _ as *mut _,
stdin_fd.as_raw_fd(),
stdout_fd.as_raw_fd(),
stderr_fd.as_raw_fd(),
stdin_raw_fd,
stdout_raw_fd,
stderr_raw_fd,
&mut error,
);
if error.is_null() {
Expand Down
39 changes: 28 additions & 11 deletions gio/src/socket.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Take a look at the license at the top of the repository in the LICENSE file.

#[cfg(unix)]
use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd};
use std::os::unix::io::{AsFd, AsRawFd, BorrowedFd, FromRawFd, IntoRawFd, OwnedFd, RawFd};
#[cfg(windows)]
use std::os::windows::io::{AsRawSocket, FromRawSocket, IntoRawSocket, RawSocket};
#[cfg(feature = "v2_60")]
Expand All @@ -18,15 +18,18 @@ use crate::{ffi, Cancellable, Socket, SocketAddress, SocketControlMessage};
impl Socket {
#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
#[allow(clippy::missing_safety_doc)]
pub unsafe fn from_fd(fd: impl IntoRawFd) -> Result<Socket, glib::Error> {
#[doc(alias = "g_socket_new_from_fd")]
pub fn from_fd(fd: OwnedFd) -> Result<Socket, glib::Error> {
A6GibKm marked this conversation as resolved.
Show resolved Hide resolved
let fd = fd.into_raw_fd();
let mut error = ptr::null_mut();
let ret = ffi::g_socket_new_from_fd(fd, &mut error);
if error.is_null() {
Ok(from_glib_full(ret))
} else {
Err(from_glib_full(error))
unsafe {
let ret = ffi::g_socket_new_from_fd(fd, &mut error);
if error.is_null() {
Ok(from_glib_full(ret))
} else {
libc::close(fd);
Err(from_glib_full(error))
Copy link
Member

Choose a reason for hiding this comment

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

On success, the returned GSocket takes ownership of fd. On failure, the caller must close fd themselves.

So you need to close() the fd here

}
}
}
#[cfg(windows)]
Expand All @@ -52,6 +55,17 @@ impl AsRawFd for Socket {
}
}

#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
impl AsFd for Socket {
fn as_fd(&self) -> BorrowedFd<'_> {
unsafe {
let raw_fd = self.as_raw_fd();
BorrowedFd::borrow_raw(raw_fd)
}
}
}

#[cfg(windows)]
#[cfg_attr(docsrs, doc(cfg(windows)))]
impl AsRawSocket for Socket {
Expand Down Expand Up @@ -806,7 +820,10 @@ mod tests {
#[test]
#[cfg(unix)]
fn socket_messages() {
use std::{io, os::unix::io::AsRawFd};
use std::{
io,
os::unix::io::{AsRawFd, FromRawFd, OwnedFd},
};

use super::Socket;
use crate::{prelude::*, Cancellable, UnixFDMessage};
Expand All @@ -818,8 +835,8 @@ mod tests {
panic!("{}", io::Error::last_os_error());
}
(
Socket::from_fd(fds[0]).unwrap(),
Socket::from_fd(fds[1]).unwrap(),
Socket::from_fd(OwnedFd::from_raw_fd(fds[0])).unwrap(),
Socket::from_fd(OwnedFd::from_raw_fd(fds[1])).unwrap(),
)
};

Expand Down
27 changes: 14 additions & 13 deletions gio/src/subprocess_launcher.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Take a look at the license at the top of the repository in the LICENSE file.

#[cfg(any(unix, all(docsrs, unix)))]
use std::os::unix::io::IntoRawFd;
use std::os::unix::io::{AsFd, AsRawFd, IntoRawFd, OwnedFd};

#[cfg(unix)]
use glib::translate::*;
Expand All @@ -22,40 +22,41 @@ impl SubprocessLauncher {
#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
#[doc(alias = "g_subprocess_launcher_take_fd")]
pub fn take_fd(&self, source_fd: impl IntoRawFd, target_fd: impl IntoRawFd) {
pub fn take_fd(&self, source_fd: OwnedFd, target_fd: impl AsFd) {
let source_raw_fd = source_fd.into_raw_fd();
let target_raw_fd = target_fd.as_fd().as_raw_fd();
unsafe {
ffi::g_subprocess_launcher_take_fd(
self.to_glib_none().0,
source_fd.into_raw_fd(),
target_fd.into_raw_fd(),
);
ffi::g_subprocess_launcher_take_fd(self.to_glib_none().0, source_raw_fd, target_raw_fd);
}
}

#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
#[doc(alias = "g_subprocess_launcher_take_stderr_fd")]
pub fn take_stderr_fd(&self, fd: impl IntoRawFd) {
pub fn take_stderr_fd(&self, fd: Option<OwnedFd>) {
unsafe {
ffi::g_subprocess_launcher_take_stderr_fd(self.to_glib_none().0, fd.into_raw_fd());
let raw_fd = fd.map_or(-1, |fd| fd.into_raw_fd());
ffi::g_subprocess_launcher_take_stderr_fd(self.to_glib_none().0, raw_fd);
}
}

#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
#[doc(alias = "g_subprocess_launcher_take_stdin_fd")]
pub fn take_stdin_fd(&self, fd: impl IntoRawFd) {
pub fn take_stdin_fd(&self, fd: Option<OwnedFd>) {
let raw_fd = fd.map_or(-1, |fd| fd.into_raw_fd());
unsafe {
ffi::g_subprocess_launcher_take_stdin_fd(self.to_glib_none().0, fd.into_raw_fd());
ffi::g_subprocess_launcher_take_stdin_fd(self.to_glib_none().0, raw_fd);
}
}

#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
#[doc(alias = "g_subprocess_launcher_take_stdout_fd")]
pub fn take_stdout_fd(&self, fd: impl IntoRawFd) {
pub fn take_stdout_fd(&self, fd: Option<OwnedFd>) {
let raw_fd = fd.map_or(-1, |fd| fd.into_raw_fd());
unsafe {
ffi::g_subprocess_launcher_take_stdout_fd(self.to_glib_none().0, fd.into_raw_fd());
ffi::g_subprocess_launcher_take_stdout_fd(self.to_glib_none().0, raw_fd);
}
}
}
103 changes: 84 additions & 19 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::{AsRawFd, IntoRawFd, RawFd};
use std::os::unix::io::{AsFd, AsRawFd, BorrowedFd, FromRawFd, IntoRawFd, OwnedFd};
use std::{mem, ptr};

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

use crate::{ffi, UnixFDList};

Expand All @@ -30,12 +30,12 @@ mod sealed {

pub trait UnixFDListExtManual: sealed::Sealed + IsA<UnixFDList> + Sized {
#[doc(alias = "g_unix_fd_list_append")]
fn append<T: AsRawFd>(&self, fd: T) -> Result<i32, glib::Error> {
fn append(&self, fd: impl AsFd) -> Result<i32, glib::Error> {
unsafe {
let mut error = ptr::null_mut();
let ret = ffi::g_unix_fd_list_append(
self.as_ref().to_glib_none().0,
fd.as_raw_fd(),
fd.as_fd().as_raw_fd(),
&mut error,
);
if error.is_null() {
Expand All @@ -47,41 +47,106 @@ pub trait UnixFDListExtManual: sealed::Sealed + IsA<UnixFDList> + Sized {
}

#[doc(alias = "g_unix_fd_list_get")]
fn get(&self, index_: i32) -> Result<RawFd, glib::Error> {
fn get(&self, index_: i32) -> Result<OwnedFd, glib::Error> {
unsafe {
let mut error = ptr::null_mut();
let ret = ffi::g_unix_fd_list_get(self.as_ref().to_glib_none().0, index_, &mut error);
let raw_fd =
ffi::g_unix_fd_list_get(self.as_ref().to_glib_none().0, index_, &mut error);
let fd = OwnedFd::from_raw_fd(raw_fd);
if error.is_null() {
Ok(ret)
Ok(fd)
} else {
Err(from_glib_full(error))
}
}
}

#[doc(alias = "g_unix_fd_list_peek_fds")]

fn peek_fds(&self) -> Vec<RawFd> {
fn peek_fds(&self) -> &[BorrowedFd<'_>] {
unsafe {
let mut length = mem::MaybeUninit::uninit();
let ret = FromGlibContainer::from_glib_none_num(
ffi::g_unix_fd_list_peek_fds(self.as_ref().to_glib_none().0, length.as_mut_ptr()),
std::slice::from_raw_parts(
ffi::g_unix_fd_list_peek_fds(self.as_ref().to_glib_none().0, length.as_mut_ptr())
as *const BorrowedFd,
length.assume_init() as usize,
);
ret
)
}
}

#[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

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) }
}
}
Loading
Loading