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

feat: I/O safety for 'sys/termios' & 'pty' #1921

Merged
merged 1 commit into from
Dec 8, 2022

Conversation

SteveLauC
Copy link
Member

@SteveLauC SteveLauC commented Dec 7, 2022

What this PR does:

  1. Adds I/O safety for modules sys/termios and pty

Known Problems:

  1. Double free issue on PtyMaster

    I have changed the RawFd in PtyMaster to OwnedFd in this PR, with this
    change, the double-free issue still exists, see this test code snippet
    (From this comment)

    use std::io::prelude::*;
    use std::os::unix::io::AsRawFd;
    
    fn main() {
        let mut f = {
            let m = nix::pty::posix_openpt(nix::fcntl::OFlag::O_RDWR).unwrap(); // get fd 3
            nix::unistd::close(m.as_raw_fd()).unwrap(); // close fd 3
            std::fs::File::create("foo").unwrap()       // get fd 3 again
        }; // m goes out of scope, `drop(OwnedFd)`, fd 3 closed
        f.write("whatever".as_bytes()).unwrap(); // EBADF
    }

    I have tested this code with nix 0.26.1, and I am still getting EBADF, which means the current impl does not prevent this problem either.

    $ cat Cargo.toml | grep nix
    nix = "0.26.1"
    
    $ cargo r -q
    thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 9, kind: Uncategorized, message: "Bad file descriptor" }', src/main.rs:10:36

    If we still wanna the drop of PtyMaster panic when the internal fd is invalid
    as we did in PtyMaster::drop should panic on EBADF #677, then we have to revert the changes to use RawFd and manually impl Drop.

  2. Some trait implementations for some types are removed

    • struct OpenptyResult:

      1. PartialEq
      2. Eq
      3. Hash
      4. Clone
    • struct ForkptyResult:

      1. Clone
    • struct PtyMaster:

      1. PartialEq
      2. Eq
      3. Hash

    In the previous implementation, these trait impls are #[derive()]ed, due to
    the type change to OwnedFd, we can no longer derive them. Should we manually
    implement them?

    I kinda think we should at least impl PartialEq and Eq for OpenptyResult
    and PtyMaster.


Some Clarifications that may help code review

  1. For the basic fd-related syscall like read(2), write(2) and fcntl(2)
    , I am still using the old RawFd interfaces, as they will be covered in
    other PRs.

  2. Two helper functions

    1. write_all() in test/sys/test_termios.rs:

      /// Helper function analogous to `std::io::Write::write_all`, but for `RawFd`s
      fn write_all(f: RawFd, buf: &[u8]) {
      /// Helper function analogous to `std::io::Write::write_all`, but for `Fd`s
      fn write_all<Fd: AsFd>(f: &Fd, buf: &[u8]) {
          let mut len = 0;
          while len < buf.len() {
              len += write(f, &buf[len..]).unwrap();
              len += write(f.as_fd().as_raw_fd(), &buf[len..]).unwrap();
          }
      }
    2. read_exact() in test/test.rs:

      /// Helper function analogous to `std::io::Read::read_exact`, but for `RawFD`s
      fn read_exact(f: RawFd, buf: &mut [u8]) {
      /// Helper function analogous to `std::io::Read::read_exact`, but for `Fd`s
      fn read_exact<Fd: AsFd>(f: &Fd, buf: &mut [u8]) {
          let mut len = 0;
          while len < buf.len() {
              // get_mut would be better than split_at_mut, but it requires nightly
              let (_, remaining) = buf.split_at_mut(len);
              len += read(f, remaining).unwrap();
              len += read(f.as_fd().as_raw_fd(), remaining).unwrap();
          }
      }

    I have added I/O safety for them, but it actually does not matter whether
    they use Fd: AsFd or RawFd. So feel free to ask me to discard these changes
    if you guys don't like it.

@SteveLauC SteveLauC mentioned this pull request Dec 7, 2022
29 tasks
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Soon we will change nix::unistd::close to take an OwnedFd, which will make the double-close situation much harder to trigger. Given that, I'm cool with this PR. We don't need to worry overmuch about what happens in Drop.

I think it makes sense to drop those particular trait implementations. They aren't very useful for file descriptors. PartialEq used to be, but isn't any more now that OwnedFd isn't Clone. So how would you ever get two PtyMaster objects that were equal? Equally weird, one might expect that file descriptors returned by dup2 would be equal to their originals, but they won't be for any likely implementation of PartialEq.

This PR looks pretty good. I just wonder if we should add explicit instructions about Drop to the CHANGELOG.

src/pty.rs Show resolved Hide resolved
@SteveLauC SteveLauC force-pushed the io-safety-sys/termios-and-pty branch from 0fb3a12 to 8f52bc9 Compare December 8, 2022 06:04
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

@bors bors bot merged commit a2356ab into nix-rust:master Dec 8, 2022
@SteveLauC SteveLauC deleted the io-safety-sys/termios-and-pty branch December 8, 2022 23:02
bors bot added a commit that referenced this pull request Dec 9, 2022
1932: refactor: take `AsFd` by value r=asomers a=SteveLauC

#### What this PR does

1. Changes the `fd` type to take `AsFd` by value for the I/O safety PRs that are merged.
    * #1916 
    * #1919 
    * #1921 
    * #1922 

Co-authored-by: Steve Lau <[email protected]>
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.

2 participants