diff options
| -rw-r--r-- | Cargo.toml | 4 | ||||
| -rw-r--r-- | src/pty.rs | 15 | ||||
| -rw-r--r-- | src/unistd.rs | 34 | ||||
| -rw-r--r-- | test/test_pty.rs | 16 | ||||
| -rw-r--r-- | test/test_ptymaster_drop.rs | 21 |
5 files changed, 86 insertions, 4 deletions
@@ -48,3 +48,7 @@ test = true name = "test-mount" path = "test/test_mount.rs" harness = false + +[[test]] +name = "test-ptymaster-drop" +path = "test/test_ptymaster_drop.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/src/unistd.rs b/src/unistd.rs index 3209bb98..196232b6 100644 --- a/src/unistd.rs +++ b/src/unistd.rs @@ -670,6 +670,40 @@ pub fn gethostname<'a>(buffer: &'a mut [u8]) -> Result<&'a CStr> { }) } +/// Close a raw file descriptor +/// +/// Be aware that many Rust types implicitly close-on-drop, including +/// `std::fs::File`. Explicitly closing them with this method too can result in +/// a double-close condition, which can cause confusing `EBADF` errors in +/// seemingly unrelated code. Caveat programmer. +/// +/// # Examples +/// +/// ```no_run +/// extern crate tempfile; +/// extern crate nix; +/// +/// use std::os::unix::io::AsRawFd; +/// use nix::unistd::close; +/// +/// fn main() { +/// let mut f = tempfile::tempfile().unwrap(); +/// close(f.as_raw_fd()).unwrap(); // Bad! f will also close on drop! +/// } +/// ``` +/// +/// ```rust +/// extern crate tempfile; +/// extern crate nix; +/// +/// use std::os::unix::io::IntoRawFd; +/// use nix::unistd::close; +/// +/// fn main() { +/// let mut f = tempfile::tempfile().unwrap(); +/// close(f.into_raw_fd()).unwrap(); // Good. into_raw_fd consumes f +/// } +/// ``` pub fn close(fd: RawFd) -> Result<()> { let res = unsafe { libc::close(fd) }; Errno::result(res).map(drop) diff --git a/test/test_pty.rs b/test/test_pty.rs index 53e94724..55316ab4 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,20 @@ use nix::sys::stat; use nix::sys::termios::*; use nix::unistd::{write, close}; +/// 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"))] diff --git a/test/test_ptymaster_drop.rs b/test/test_ptymaster_drop.rs new file mode 100644 index 00000000..664a4dd6 --- /dev/null +++ b/test/test_ptymaster_drop.rs @@ -0,0 +1,21 @@ +extern crate nix; + +use nix::fcntl::O_RDWR; +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!")] +// In Travis on i686-unknown-linux-musl, this test gets SIGABRT. I don't know +// why. It doesn't happen on any other target, and it doesn't happen on my PC. +#[cfg_attr(all(target_env = "musl", target_arch = "x86"), ignore)] +fn test_double_close() { + let m = posix_openpt(O_RDWR).unwrap(); + close(m.as_raw_fd()).unwrap(); + drop(m); // should panic here +} |