PtyMaster::drop should panic on EBADF

Fixes #659

Author: asomers

src/pty.rs

@@ -45,10 +45,17 @@ impl IntoRawFd for PtyMaster {
 
 impl Drop for PtyMaster {
     fn drop(&mut self) {
-        // Errors when closing are ignored because we don't actually know if the file descriptor
-        // was closed. If we retried, it's possible that descriptor was reallocated in the mean
-        // time and the wrong file descriptor could be closed.
-        let _ = ::unistd::close(self.0);
+        // On drop, we ignore errors like EINTR and EIO because there's no clear
+        // way to handle them, we can't return anything, and (on FreeBSD at
+        // least) the file descriptor is deallocated in these cases.  However,
+        // we must panic on EBADF, because it is always an error to close an
+        // invalid file descriptor.  That frequently indicates a double-close
+        // condition, which can cause confusing errors for future I/O
+        // operations.
+        let e = ::unistd::close(self.0);
+        if e == Err(Error::Sys(Errno::EBADF)) {
+            panic!("Closing an invalid file descriptor!");
+        };
     }
 }
 

test/test_pty.rs

@@ -1,12 +1,38 @@
+use std::io::Write;
 use std::path::Path;
 use std::os::unix::prelude::*;
+use tempfile::tempfile;
 
 use nix::fcntl::{O_RDWR, open};
 use nix::pty::*;
 use nix::sys::stat;
 use nix::sys::termios::*;
 use nix::unistd::{write, close};
 
+/// Regression test for Issue #659
+/// PtyMaster should panic rather than double close the file descriptor
+#[test]
+#[should_panic(expected = "Closing an invalid file descriptor!")]
+fn test_double_close() {
+    let m = posix_openpt(O_RDWR).unwrap();
+    close(m.as_raw_fd()).unwrap();
+    drop(m);            // should panic here
+}
+
+/// 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(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(b"whatever").unwrap();
+}
+
 /// Test equivalence of `ptsname` and `ptsname_r`
 #[test]
 #[cfg(any(target_os = "android", target_os = "linux"))]