summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--Cargo.toml4
-rw-r--r--src/pty.rs15
-rw-r--r--src/unistd.rs34
-rw-r--r--test/test_pty.rs16
-rw-r--r--test/test_ptymaster_drop.rs21
5 files changed, 86 insertions, 4 deletions
diff --git a/Cargo.toml b/Cargo.toml
index 64a212f6..e768f4a1 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -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"
diff --git a/src/pty.rs b/src/pty.rs
index 88c9cc14..fd8ad98e 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/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
+}