Merge #1921

1921: feat: I/O safety for 'sys/termios' & 'pty' r=asomers a=SteveLauC

#### What this PR does:
1. Adds I/O safety for modules `sys/termios` and `pty`

------


#### Known Problems:

1. [Double free issue on `PtyMaster`](https://github.com/nix-rust/nix/issues/659)
                                            
   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](https://github.com/nix-rust/nix/issues/659#issuecomment-315544022))

   ```rust
   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.

   ```shell
   $ 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 #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`:

      ```rust
      /// 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`:

      ```rust
      /// 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.


Co-authored-by: Steve Lau <stevelauc@outlook.com>

5 files changed, 41 insertions, 133 deletions

diff --git a/test/sys/test_termios.rs b/test/sys/test_termios.rs
index aaf00084..cf55b10c 100644
--- a/test/sys/test_termios.rs
+++ b/test/sys/test_termios.rs
@@ -1,17 +1,17 @@
-use std::os::unix::prelude::*;
+use std::os::unix::io::{AsFd, AsRawFd};
 use tempfile::tempfile;
 
 use nix::errno::Errno;
 use nix::fcntl;
 use nix::pty::openpty;
 use nix::sys::termios::{self, tcgetattr, LocalFlags, OutputFlags};
-use nix::unistd::{close, read, write};
+use nix::unistd::{read, write};
 
-/// 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();
     }
 }
 
@@ -22,25 +22,14 @@ fn test_tcgetattr_pty() {
     let _m = crate::PTSNAME_MTX.lock();
 
     let pty = openpty(None, None).expect("openpty failed");
-    termios::tcgetattr(pty.slave).unwrap();
-    close(pty.master).expect("closing the master failed");
-    close(pty.slave).expect("closing the slave failed");
+    termios::tcgetattr(&pty.slave).unwrap();
 }
 
 // Test tcgetattr on something that isn't a terminal
 #[test]
 fn test_tcgetattr_enotty() {
     let file = tempfile().unwrap();
-    assert_eq!(
-        termios::tcgetattr(file.as_raw_fd()).err(),
-        Some(Errno::ENOTTY)
-    );
-}
-
-// Test tcgetattr on an invalid file descriptor
-#[test]
-fn test_tcgetattr_ebadf() {
-    assert_eq!(termios::tcgetattr(-1).err(), Some(Errno::EBADF));
+    assert_eq!(termios::tcgetattr(&file).err(), Some(Errno::ENOTTY));
 }
 
 // Test modifying output flags
@@ -52,12 +41,7 @@ fn test_output_flags() {
     // Open one pty to get attributes for the second one
     let mut termios = {
         let pty = openpty(None, None).expect("openpty failed");
-        assert!(pty.master > 0);
-        assert!(pty.slave > 0);
-        let termios = tcgetattr(pty.slave).expect("tcgetattr failed");
-        close(pty.master).unwrap();
-        close(pty.slave).unwrap();
-        termios
+        tcgetattr(&pty.slave).expect("tcgetattr failed")
     };
 
     // Make sure postprocessing '\r' isn't specified by default or this test is useless.
@@ -73,19 +57,15 @@ fn test_output_flags() {
 
     // Open a pty
     let pty = openpty(None, &termios).unwrap();
-    assert!(pty.master > 0);
-    assert!(pty.slave > 0);
 
     // Write into the master
     let string = "foofoofoo\r";
-    write_all(pty.master, string.as_bytes());
+    write_all(&pty.master, string.as_bytes());
 
     // Read from the slave verifying that the output has been properly transformed
     let mut buf = [0u8; 10];
-    crate::read_exact(pty.slave, &mut buf);
+    crate::read_exact(&pty.slave, &mut buf);
     let transformed_string = "foofoofoo\n";
-    close(pty.master).unwrap();
-    close(pty.slave).unwrap();
     assert_eq!(&buf, transformed_string.as_bytes());
 }
 
@@ -98,12 +78,7 @@ fn test_local_flags() {
     // Open one pty to get attributes for the second one
     let mut termios = {
         let pty = openpty(None, None).unwrap();
-        assert!(pty.master > 0);
-        assert!(pty.slave > 0);
-        let termios = tcgetattr(pty.slave).unwrap();
-        close(pty.master).unwrap();
-        close(pty.slave).unwrap();
-        termios
+        tcgetattr(&pty.slave).unwrap()
     };
 
     // Make sure echo is specified by default or this test is useless.
@@ -114,23 +89,19 @@ fn test_local_flags() {
 
     // Open a new pty with our modified termios settings
     let pty = openpty(None, &termios).unwrap();
-    assert!(pty.master > 0);
-    assert!(pty.slave > 0);
 
     // Set the master is in nonblocking mode or reading will never return.
-    let flags = fcntl::fcntl(pty.master, fcntl::F_GETFL).unwrap();
+    let flags = fcntl::fcntl(pty.master.as_raw_fd(), fcntl::F_GETFL).unwrap();
     let new_flags =
         fcntl::OFlag::from_bits_truncate(flags) | fcntl::OFlag::O_NONBLOCK;
-    fcntl::fcntl(pty.master, fcntl::F_SETFL(new_flags)).unwrap();
+    fcntl::fcntl(pty.master.as_raw_fd(), fcntl::F_SETFL(new_flags)).unwrap();
 
     // Write into the master
     let string = "foofoofoo\r";
-    write_all(pty.master, string.as_bytes());
+    write_all(&pty.master, string.as_bytes());
 
     // Try to read from the master, which should not have anything as echoing was disabled.
     let mut buf = [0u8; 10];
-    let read = read(pty.master, &mut buf).unwrap_err();
-    close(pty.master).unwrap();
-    close(pty.slave).unwrap();
+    let read = read(pty.master.as_raw_fd(), &mut buf).unwrap_err();
     assert_eq!(read, Errno::EAGAIN);
 }
diff --git a/test/test.rs b/test/test.rs
index 6b42aad9..b139ec3a 100644
--- a/test/test.rs
+++ b/test/test.rs
@@ -66,16 +66,16 @@ mod test_unistd;
 
 use nix::unistd::{chdir, getcwd, read};
 use parking_lot::{Mutex, RwLock, RwLockWriteGuard};
-use std::os::unix::io::RawFd;
+use std::os::unix::io::{AsFd, AsRawFd};
 use std::path::PathBuf;
 
-/// 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();
     }
 }
 
diff --git a/test/test_pty.rs b/test/test_pty.rs
index 5c27e2d6..e1286e55 100644
--- a/test/test_pty.rs
+++ b/test/test_pty.rs
@@ -2,28 +2,13 @@ use std::fs::File;
 use std::io::{Read, Write};
 use std::os::unix::prelude::*;
 use std::path::Path;
-use tempfile::tempfile;
 
 use libc::{_exit, STDOUT_FILENO};
 use nix::fcntl::{open, OFlag};
 use nix::pty::*;
 use nix::sys::stat;
 use nix::sys::termios::*;
-use nix::unistd::{close, pause, write};
-
-/// Regression test for Issue #659
-/// This is the correct way to explicitly close a `PtyMaster`
-#[test]
-fn test_explicit_close() {
-    let mut f = {
-        let m = posix_openpt(OFlag::O_RDWR).unwrap();
-        close(m.into_raw_fd()).unwrap();
-        tempfile().unwrap()
-    };
-    // This should work.  But if there's been a double close, then it will
-    // return EBADF
-    f.write_all(b"whatever").unwrap();
-}
+use nix::unistd::{pause, write};
 
 /// Test equivalence of `ptsname` and `ptsname_r`
 #[test]
@@ -50,7 +35,6 @@ fn test_ptsname_copy() {
 
     // Open a new PTTY master
     let master_fd = posix_openpt(OFlag::O_RDWR).unwrap();
-    assert!(master_fd.as_raw_fd() > 0);
 
     // Get the name of the slave
     let slave_name1 = unsafe { ptsname(&master_fd) }.unwrap();
@@ -67,7 +51,6 @@ fn test_ptsname_copy() {
 fn test_ptsname_r_copy() {
     // Open a new PTTY master
     let master_fd = posix_openpt(OFlag::O_RDWR).unwrap();
-    assert!(master_fd.as_raw_fd() > 0);
 
     // Get the name of the slave
     let slave_name1 = ptsname_r(&master_fd).unwrap();
@@ -84,11 +67,9 @@ fn test_ptsname_unique() {
 
     // Open a new PTTY master
     let master1_fd = posix_openpt(OFlag::O_RDWR).unwrap();
-    assert!(master1_fd.as_raw_fd() > 0);
 
     // Open a second PTTY master
     let master2_fd = posix_openpt(OFlag::O_RDWR).unwrap();
-    assert!(master2_fd.as_raw_fd() > 0);
 
     // Get the name of the slave
     let slave_name1 = unsafe { ptsname(&master1_fd) }.unwrap();
@@ -147,16 +128,14 @@ fn open_ptty_pair() -> (PtyMaster, File) {
 ///
 /// This uses a common `open_ptty_pair` because much of these functions aren't useful by
 /// themselves. So for this test we perform the basic act of getting a file handle for a
-/// master/slave PTTY pair, then just sanity-check the raw values.
+/// master/slave PTTY pair.
 #[test]
 fn test_open_ptty_pair() {
-    let (master, slave) = open_ptty_pair();
-    assert!(master.as_raw_fd() > 0);
-    assert!(slave.as_raw_fd() > 0);
+    let (_, _) = open_ptty_pair();
 }
 
 /// Put the terminal in raw mode.
-fn make_raw(fd: RawFd) {
+fn make_raw<Fd: AsFd>(fd: &Fd) {
     let mut termios = tcgetattr(fd).unwrap();
     cfmakeraw(&mut termios);
     tcsetattr(fd, SetArg::TCSANOW, &termios).unwrap();
@@ -166,7 +145,7 @@ fn make_raw(fd: RawFd) {
 #[test]
 fn test_read_ptty_pair() {
     let (mut master, mut slave) = open_ptty_pair();
-    make_raw(slave.as_raw_fd());
+    make_raw(&slave);
 
     let mut buf = [0u8; 5];
     slave.write_all(b"hello").unwrap();
@@ -183,7 +162,7 @@ fn test_read_ptty_pair() {
 #[test]
 fn test_write_ptty_pair() {
     let (mut master, mut slave) = open_ptty_pair();
-    make_raw(slave.as_raw_fd());
+    make_raw(&slave);
 
     let mut buf = [0u8; 5];
     master.write_all(b"adios").unwrap();
@@ -202,33 +181,28 @@ fn test_openpty() {
     let _m = crate::PTSNAME_MTX.lock();
 
     let pty = openpty(None, None).unwrap();
-    assert!(pty.master > 0);
-    assert!(pty.slave > 0);
 
     // Writing to one should be readable on the other one
     let string = "foofoofoo\n";
     let mut buf = [0u8; 10];
-    write(pty.master, string.as_bytes()).unwrap();
-    crate::read_exact(pty.slave, &mut buf);
+    write(pty.master.as_raw_fd(), string.as_bytes()).unwrap();
+    crate::read_exact(&pty.slave, &mut buf);
 
     assert_eq!(&buf, string.as_bytes());
 
     // Read the echo as well
     let echoed_string = "foofoofoo\r\n";
     let mut buf = [0u8; 11];
-    crate::read_exact(pty.master, &mut buf);
+    crate::read_exact(&pty.master, &mut buf);
     assert_eq!(&buf, echoed_string.as_bytes());
 
     let string2 = "barbarbarbar\n";
     let echoed_string2 = "barbarbarbar\r\n";
     let mut buf = [0u8; 14];
-    write(pty.slave, string2.as_bytes()).unwrap();
-    crate::read_exact(pty.master, &mut buf);
+    write(pty.slave.as_raw_fd(), string2.as_bytes()).unwrap();
+    crate::read_exact(&pty.master, &mut buf);
 
     assert_eq!(&buf, echoed_string2.as_bytes());
-
-    close(pty.master).unwrap();
-    close(pty.slave).unwrap();
 }
 
 #[test]
@@ -239,44 +213,34 @@ fn test_openpty_with_termios() {
     // Open one pty to get attributes for the second one
     let mut termios = {
         let pty = openpty(None, None).unwrap();
-        assert!(pty.master > 0);
-        assert!(pty.slave > 0);
-        let termios = tcgetattr(pty.slave).unwrap();
-        close(pty.master).unwrap();
-        close(pty.slave).unwrap();
-        termios
+        tcgetattr(&pty.slave).unwrap()
     };
     // Make sure newlines are not transformed so the data is preserved when sent.
     termios.output_flags.remove(OutputFlags::ONLCR);
 
     let pty = openpty(None, &termios).unwrap();
     // Must be valid file descriptors
-    assert!(pty.master > 0);
-    assert!(pty.slave > 0);
 
     // Writing to one should be readable on the other one
     let string = "foofoofoo\n";
     let mut buf = [0u8; 10];
-    write(pty.master, string.as_bytes()).unwrap();
-    crate::read_exact(pty.slave, &mut buf);
+    write(pty.master.as_raw_fd(), string.as_bytes()).unwrap();
+    crate::read_exact(&pty.slave, &mut buf);
 
     assert_eq!(&buf, string.as_bytes());
 
     // read the echo as well
     let echoed_string = "foofoofoo\n";
-    crate::read_exact(pty.master, &mut buf);
+    crate::read_exact(&pty.master, &mut buf);
     assert_eq!(&buf, echoed_string.as_bytes());
 
     let string2 = "barbarbarbar\n";
     let echoed_string2 = "barbarbarbar\n";
     let mut buf = [0u8; 13];
-    write(pty.slave, string2.as_bytes()).unwrap();
-    crate::read_exact(pty.master, &mut buf);
+    write(pty.slave.as_raw_fd(), string2.as_bytes()).unwrap();
+    crate::read_exact(&pty.master, &mut buf);
 
     assert_eq!(&buf, echoed_string2.as_bytes());
-
-    close(pty.master).unwrap();
-    close(pty.slave).unwrap();
 }
 
 #[test]
@@ -303,11 +267,10 @@ fn test_forkpty() {
         Parent { child } => {
             let mut buf = [0u8; 10];
             assert!(child.as_raw() > 0);
-            crate::read_exact(pty.master, &mut buf);
+            crate::read_exact(&pty.master, &mut buf);
             kill(child, SIGTERM).unwrap();
             wait().unwrap(); // keep other tests using generic wait from getting our child
             assert_eq!(&buf, echoed_string.as_bytes());
-            close(pty.master).unwrap();
         }
     }
 }
diff --git a/test/test_ptymaster_drop.rs b/test/test_ptymaster_drop.rs
deleted file mode 100644
index ffbaa569..00000000
--- a/test/test_ptymaster_drop.rs
+++ /dev/null
@@ -1,20 +0,0 @@
-#[cfg(not(any(target_os = "redox", target_os = "fuchsia")))]
-mod t {
-    use nix::fcntl::OFlag;
-    use nix::pty::*;
-    use nix::unistd::close;
-    use std::os::unix::io::AsRawFd;
-
-    /// Regression test for Issue #659
-    ///
-    /// `PtyMaster` should panic rather than double close the file descriptor
-    /// This must run in its own test process because it deliberately creates a
-    /// race condition.
-    #[test]
-    #[should_panic(expected = "Closing an invalid file descriptor!")]
-    fn test_double_close() {
-        let m = posix_openpt(OFlag::O_RDWR).unwrap();
-        close(m.as_raw_fd()).unwrap();
-        drop(m); // should panic here
-    }
-}
diff --git a/test/test_unistd.rs b/test/test_unistd.rs
index 6619262e..1d50c5fa 100644
--- a/test/test_unistd.rs
+++ b/test/test_unistd.rs
@@ -555,16 +555,13 @@ fn test_lseek() {
     const CONTENTS: &[u8] = b"abcdef123456";
     let mut tmp = tempfile().unwrap();
     tmp.write_all(CONTENTS).unwrap();
-    let tmpfd = tmp.into_raw_fd();
 
     let offset: off_t = 5;
-    lseek(tmpfd, offset, Whence::SeekSet).unwrap();
+    lseek(tmp.as_raw_fd(), offset, Whence::SeekSet).unwrap();
 
     let mut buf = [0u8; 7];
-    crate::read_exact(tmpfd, &mut buf);
+    crate::read_exact(&tmp, &mut buf);
     assert_eq!(b"f123456", &buf);
-
-    close(tmpfd).unwrap();
 }
 
 #[cfg(any(target_os = "linux", target_os = "android"))]
@@ -573,15 +570,12 @@ fn test_lseek64() {
     const CONTENTS: &[u8] = b"abcdef123456";
     let mut tmp = tempfile().unwrap();
     tmp.write_all(CONTENTS).unwrap();
-    let tmpfd = tmp.into_raw_fd();
 
-    lseek64(tmpfd, 5, Whence::SeekSet).unwrap();
+    lseek64(tmp.as_raw_fd(), 5, Whence::SeekSet).unwrap();
 
     let mut buf = [0u8; 7];
-    crate::read_exact(tmpfd, &mut buf);
+    crate::read_exact(&tmp, &mut buf);
     assert_eq!(b"f123456", &buf);
-
-    close(tmpfd).unwrap();
 }
 
 cfg_if! {