diff --git a/src/pty.rs b/src/pty.rs index 88c9cc14e2..fd8ad98e44 100644 --- a/src/pty.rs +++ b/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!"); + }; } } diff --git a/test/test_pty.rs b/test/test_pty.rs index 53e94724ac..e2cd5a5a78 100644 --- a/test/test_pty.rs +++ b/test/test_pty.rs @@ -1,5 +1,7 @@ +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::*; @@ -7,6 +9,30 @@ 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"))]