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

Conversation

SteveLauC
Copy link
Member

@SteveLauC SteveLauC commented Jun 9, 2024

What does this PR do

Add I/O safety to module unistd.

File descriptor duplication

The safe interface for dup2() proposed in this PR looks like this:

pub fn dup2<Fd: AsFd>(oldfd: Fd, newfd: &mut OwnedFd) -> Result<()>

We take newfd by mutable reference so that we can ensure we have exclusive access. This interface stops users from specifying newfd with arbitrary fd values, unsafe fn dup2_raw() comes as a remedy:

pub unsafe fn dup2_raw<Fd1: AsFd, Fd2: IntoRawFd>(
    oldfd: Fd1,
    newfd: Fd2,
) -> Result<OwnedFd>

dup2_raw() is unsafe because when specifying a fd that is open, one has to ensure the returned OwnedFd is the ONLY owner of this fd, or a double close will happen.

image

Using the above dup2() interface for stdin/stdout/stderr redirection is troublesome, one has to construct an OwnedFd from stdin/stdout/stderr, then mem::forget() it. We provide 3 helper functions to make this easier:

pub fn dup2_stdin<Fd: AsFd>(fd: Fd) -> Result<()>
pub fn dup2_stdout<Fd: AsFd>(fd: Fd) -> Result<()>
pub fn dup2_stderr<Fd: AsFd>(fd: Fd) -> Result<()>

Since the only flag that can be used with dup3() is O_CLOEXEC, which does not make much sense to stdin/stdout/stderr, I didn't provide the dup3 versions of these utilities.

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

@SteveLauC SteveLauC mentioned this pull request Jun 9, 2024
29 tasks
))]
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.

@@ -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.

src/unistd.rs Outdated
@@ -1295,19 +1504,21 @@ impl LinkatFlags {
/// # References
/// See also [linkat(2)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/linkat.html)
#[cfg(not(target_os = "redox"))] // RedoxFS does not support symlinks yet
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if hard link is supported by RedoxFS, but linkat() is for hard link rather than symlink

@SteveLauC SteveLauC changed the title refactor: I/O safety for sys/unistd.rs refactor: I/O safety for unistd.rs Jun 10, 2024
@SteveLauC SteveLauC marked this pull request as ready for review June 10, 2024 06:09
@SteveLauC SteveLauC added this pull request to the merge queue Jun 10, 2024
Merged via the queue into nix-rust:master with commit 458013d Jun 10, 2024
35 checks passed
@SteveLauC SteveLauC deleted the refactor/io_safety_unistd branch June 10, 2024 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant