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

refactor: I/O safety for unistd.rs #2440

Merged
merged 6 commits into from
Jun 10, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
1 change: 1 addition & 0 deletions changelog/2440.changed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Module unistd now adopts I/O safety.
10 changes: 2 additions & 8 deletions src/fcntl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ use std::ffi::CStr;
use std::ffi::OsString;
#[cfg(not(any(target_os = "redox", target_os = "solaris")))]
use std::ops::{Deref, DerefMut};
#[cfg(not(target_os = "redox"))]
use std::os::raw;
use std::os::unix::ffi::OsStringExt;
#[cfg(not(any(target_os = "redox", target_os = "solaris")))]
use std::os::unix::io::OwnedFd;
Expand Down Expand Up @@ -234,12 +232,8 @@ libc_bitflags!(
);

/// Computes the raw fd consumed by a function of the form `*at`.
#[cfg(any(
all(feature = "fs", not(target_os = "redox")),
all(feature = "process", linux_android),
all(feature = "fanotify", target_os = "linux")
))]
pub(crate) fn at_rawfd(fd: Option<RawFd>) -> raw::c_int {
#[cfg(all(feature = "fanotify", target_os = "linux"))]
pub(crate) fn at_rawfd(fd: Option<RawFd>) -> RawFd {
Copy link
Member Author

Choose a reason for hiding this comment

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

This can be removed when we add I/O safety to module fanotify.

fd.unwrap_or(libc::AT_FDCWD)
}

Expand Down
2 changes: 1 addition & 1 deletion src/poll.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ impl<'fd> PollFd<'fd> {
/// let mut fds = [pfd];
/// poll(&mut fds, PollTimeout::NONE).unwrap();
/// let mut buf = [0u8; 80];
/// read(r.as_raw_fd(), &mut buf[..]);
/// read(&r, &mut buf[..]);
/// ```
// Unlike I/O functions, constructors like this must take `BorrowedFd`
// instead of AsFd or &AsFd. Otherwise, an `OwnedFd` argument would be
Expand Down
4 changes: 2 additions & 2 deletions src/pty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ impl IntoRawFd for PtyMaster {

impl io::Read for PtyMaster {
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
unistd::read(self.0.as_raw_fd(), buf).map_err(io::Error::from)
Copy link
Member Author

Choose a reason for hiding this comment

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

This Read implementation (and Write) feels a little bit weird to me, I kinda think we should not implement std Read/Write traits for Nix wrapper types, the I/O syscalls should be used instead.

But this implementation would definitely make our interface more rusty, so I am not quite against it, though I think we should be consistent, i.e., if one wrapper type has this trait implemented, we should do this to all the wrapper types.

unistd::read(&self.0, buf).map_err(io::Error::from)
}
}

Expand All @@ -88,7 +88,7 @@ impl io::Write for PtyMaster {

impl io::Read for &PtyMaster {
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
unistd::read(self.0.as_raw_fd(), buf).map_err(io::Error::from)
unistd::read(&self.0, buf).map_err(io::Error::from)
}
}

Expand Down
30 changes: 18 additions & 12 deletions src/sys/eventfd.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::errno::Errno;
use crate::{Result,unistd};
use std::os::unix::io::{FromRawFd, OwnedFd, AsRawFd, AsFd, RawFd, BorrowedFd};
use crate::{unistd, Result};
use std::os::unix::io::{AsFd, AsRawFd, BorrowedFd, FromRawFd, OwnedFd, RawFd};

libc_bitflags! {
pub struct EfdFlags: libc::c_int {
Expand All @@ -10,7 +10,10 @@ libc_bitflags! {
}
}

#[deprecated(since = "0.28.0", note = "Use EventFd::from_value_and_flags() instead")]
#[deprecated(
since = "0.28.0",
note = "Use EventFd::from_value_and_flags() instead"
)]
pub fn eventfd(initval: libc::c_uint, flags: EfdFlags) -> Result<OwnedFd> {
let res = unsafe { libc::eventfd(initval, flags.bits()) };

Expand All @@ -26,9 +29,12 @@ impl EventFd {
Self::from_value_and_flags(0, EfdFlags::empty())
}
/// Constructs [`EventFd`] with the given `init_val` and `flags`.
///
///
/// Wrapper around [`libc::eventfd`].
pub fn from_value_and_flags(init_val: u32, flags: EfdFlags) -> Result<Self> {
pub fn from_value_and_flags(
init_val: u32,
flags: EfdFlags,
) -> Result<Self> {
let res = unsafe { libc::eventfd(init_val, flags.bits()) };
Errno::result(res).map(|r| Self(unsafe { OwnedFd::from_raw_fd(r) }))
}
Expand All @@ -41,29 +47,29 @@ impl EventFd {
Self::from_value_and_flags(init_val, EfdFlags::empty())
}
/// Arms `self`, a following call to `poll`, `select` or `epoll` will return immediately.
///
///
/// [`EventFd::write`] with `1`.
pub fn arm(&self) -> Result<usize> {
self.write(1)
}
/// Defuses `self`, a following call to `poll`, `select` or `epoll` will block.
///
///
/// [`EventFd::write`] with `0`.
pub fn defuse(&self) -> Result<usize> {
self.write(0)
}
/// Enqueues `value` triggers.
///
///
/// The next `value` calls to `poll`, `select` or `epoll` will return immediately.
///
///
/// [`EventFd::write`] with `value`.
pub fn write(&self, value: u64) -> Result<usize> {
unistd::write(&self.0,&value.to_ne_bytes())
pub fn write(&self, value: u64) -> Result<usize> {
unistd::write(&self.0, &value.to_ne_bytes())
}
// Reads the value from the file descriptor.
pub fn read(&self) -> Result<u64> {
let mut arr = [0; std::mem::size_of::<u64>()];
unistd::read(self.0.as_raw_fd(),&mut arr)?;
unistd::read(&self.0, &mut arr)?;
Ok(u64::from_ne_bytes(arr))
}
}
Expand Down
8 changes: 6 additions & 2 deletions src/sys/fanotify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,11 @@ impl Drop for FanotifyEvent {
if self.0.fd == libc::FAN_NOFD {
return;
}
let e = close(self.0.fd);
// SAFETY:
//
// If this fd is not `FAN_NOFD`, then it should be a valid, owned file
// descriptor, which means we can safely close it.
let e = unsafe { close(self.0.fd) };
if !std::thread::panicking() && e == Err(Errno::EBADF) {
panic!("Closing an invalid file descriptor!");
};
Expand Down Expand Up @@ -362,7 +366,7 @@ impl Fanotify {
let mut events = Vec::new();
let mut offset = 0;

let nread = read(self.fd.as_raw_fd(), &mut buffer)?;
let nread = read(&self.fd, &mut buffer)?;

while (nread - offset) >= metadata_size {
let metadata = unsafe {
Expand Down
2 changes: 1 addition & 1 deletion src/sys/inotify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ impl Inotify {
let mut events = Vec::new();
let mut offset = 0;

let nread = read(self.fd.as_raw_fd(), &mut buffer)?;
let nread = read(&self.fd, &mut buffer)?;

while (nread - offset) >= header_size {
let event = unsafe {
Expand Down
2 changes: 1 addition & 1 deletion src/sys/timerfd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ impl TimerFd {
///
/// Note: If the alarm is unset, then you will wait forever.
pub fn wait(&self) -> Result<()> {
while let Err(e) = read(self.fd.as_fd().as_raw_fd(), &mut [0u8; 8]) {
while let Err(e) = read(&self.fd, &mut [0u8; 8]) {
if e == Errno::ECANCELED {
break;
}
Expand Down
Loading