Skip to content

Commit

Permalink
Merge #1921
Browse files Browse the repository at this point in the history
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`](#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](#659 (comment)))

   ```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 <[email protected]>
  • Loading branch information
bors[bot] and SteveLauC authored Dec 8, 2022
2 parents 7f18847 + 8f52bc9 commit a2356ab
Show file tree
Hide file tree
Showing 9 changed files with 99 additions and 193 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ This project adheres to [Semantic Versioning](https://semver.org/).
([#1862](https://github.com/nix-rust/nix/pull/1862))
- The epoll interface now uses a type.
([#1882](https://github.com/nix-rust/nix/pull/1882))
- With I/O-safe type applied in `pty::OpenptyResult` and `pty::ForkptyResult`,
users no longer need to manually close the file descriptors in these types.
([#1921](https://github.com/nix-rust/nix/pull/1921))

### Fixed
### Removed
Expand Down
4 changes: 0 additions & 4 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,3 @@ path = "test/test_clearenv.rs"
name = "test-mount"
path = "test/test_mount.rs"
harness = false

[[test]]
name = "test-ptymaster-drop"
path = "test/test_ptymaster_drop.rs"
62 changes: 21 additions & 41 deletions src/pty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,78 +16,58 @@ use crate::{fcntl, unistd, Result};

/// Representation of a master/slave pty pair
///
/// This is returned by `openpty`. Note that this type does *not* implement `Drop`, so the user
/// must manually close the file descriptors.
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
/// This is returned by [`openpty`].
#[derive(Debug)]
pub struct OpenptyResult {
/// The master port in a virtual pty pair
pub master: RawFd,
pub master: OwnedFd,
/// The slave port in a virtual pty pair
pub slave: RawFd,
pub slave: OwnedFd,
}

feature! {
#![feature = "process"]
/// Representation of a master with a forked pty
///
/// This is returned by `forkpty`. Note that this type does *not* implement `Drop`, so the user
/// must manually close the file descriptors.
#[derive(Clone, Copy, Debug)]
/// This is returned by [`forkpty`].
#[derive(Debug)]
pub struct ForkptyResult {
/// The master port in a virtual pty pair
pub master: RawFd,
pub master: OwnedFd,
/// Metadata about forked process
pub fork_result: ForkResult,
}
}

/// Representation of the Master device in a master/slave pty pair
///
/// While this datatype is a thin wrapper around `RawFd`, it enforces that the available PTY
/// functions are given the correct file descriptor. Additionally this type implements `Drop`,
/// so that when it's consumed or goes out of scope, it's automatically cleaned-up.
#[derive(Debug, Eq, Hash, PartialEq)]
pub struct PtyMaster(RawFd);
/// While this datatype is a thin wrapper around `OwnedFd`, it enforces that the available PTY
/// functions are given the correct file descriptor.
#[derive(Debug)]
pub struct PtyMaster(OwnedFd);

impl AsRawFd for PtyMaster {
fn as_raw_fd(&self) -> RawFd {
self.0
self.0.as_raw_fd()
}
}

impl IntoRawFd for PtyMaster {
fn into_raw_fd(self) -> RawFd {
let fd = self.0;
mem::forget(self);
fd
}
}

impl Drop for PtyMaster {
fn drop(&mut self) {
// 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(Errno::EBADF) {
panic!("Closing an invalid file descriptor!");
};
fd.into_raw_fd()
}
}

impl io::Read for PtyMaster {
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
unistd::read(self.0, buf).map_err(io::Error::from)
unistd::read(self.0.as_raw_fd(), buf).map_err(io::Error::from)
}
}

impl io::Write for PtyMaster {
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
unistd::write(self.0, buf).map_err(io::Error::from)
unistd::write(self.0.as_raw_fd(), buf).map_err(io::Error::from)
}
fn flush(&mut self) -> io::Result<()> {
Ok(())
Expand All @@ -96,13 +76,13 @@ impl io::Write for PtyMaster {

impl io::Read for &PtyMaster {
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
unistd::read(self.0, buf).map_err(io::Error::from)
unistd::read(self.0.as_raw_fd(), buf).map_err(io::Error::from)
}
}

impl io::Write for &PtyMaster {
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
unistd::write(self.0, buf).map_err(io::Error::from)
unistd::write(self.0.as_raw_fd(), buf).map_err(io::Error::from)
}
fn flush(&mut self) -> io::Result<()> {
Ok(())
Expand Down Expand Up @@ -164,7 +144,7 @@ pub fn posix_openpt(flags: fcntl::OFlag) -> Result<PtyMaster> {
return Err(Errno::last());
}

Ok(PtyMaster(fd))
Ok(PtyMaster(unsafe { OwnedFd::from_raw_fd(fd) }))
}

/// Get the name of the slave pseudoterminal (see
Expand Down Expand Up @@ -308,8 +288,8 @@ pub fn openpty<

unsafe {
Ok(OpenptyResult {
master: master.assume_init(),
slave: slave.assume_init(),
master: OwnedFd::from_raw_fd(master.assume_init()),
slave: OwnedFd::from_raw_fd(slave.assume_init()),
})
}
}
Expand Down Expand Up @@ -364,7 +344,7 @@ pub unsafe fn forkpty<'a, 'b, T: Into<Option<&'a Winsize>>, U: Into<Option<&'b T
})?;

Ok(ForkptyResult {
master: master.assume_init(),
master: OwnedFd::from_raw_fd(master.assume_init()),
fork_result,
})
}
Expand Down
49 changes: 34 additions & 15 deletions src/sys/termios.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ use libc::{self, c_int, tcflag_t};
use std::cell::{Ref, RefCell};
use std::convert::From;
use std::mem;
use std::os::unix::io::RawFd;
use std::os::unix::io::{AsFd, AsRawFd};

#[cfg(feature = "process")]
use crate::unistd::Pid;
Expand Down Expand Up @@ -1143,10 +1143,12 @@ pub fn cfmakesane(termios: &mut Termios) {
/// `tcgetattr()` returns a `Termios` structure with the current configuration for a port. Modifying
/// this structure *will not* reconfigure the port, instead the modifications should be done to
/// the `Termios` structure and then the port should be reconfigured using `tcsetattr()`.
pub fn tcgetattr(fd: RawFd) -> Result<Termios> {
pub fn tcgetattr<Fd: AsFd>(fd: &Fd) -> Result<Termios> {
let mut termios = mem::MaybeUninit::uninit();

let res = unsafe { libc::tcgetattr(fd, termios.as_mut_ptr()) };
let res = unsafe {
libc::tcgetattr(fd.as_fd().as_raw_fd(), termios.as_mut_ptr())
};

Errno::result(res)?;

Expand All @@ -1159,53 +1161,70 @@ pub fn tcgetattr(fd: RawFd) -> Result<Termios> {
/// `tcsetattr()` reconfigures the given port based on a given `Termios` structure. This change
/// takes affect at a time specified by `actions`. Note that this function may return success if
/// *any* of the parameters were successfully set, not only if all were set successfully.
pub fn tcsetattr(fd: RawFd, actions: SetArg, termios: &Termios) -> Result<()> {
pub fn tcsetattr<Fd: AsFd>(
fd: &Fd,
actions: SetArg,
termios: &Termios,
) -> Result<()> {
let inner_termios = termios.get_libc_termios();
Errno::result(unsafe {
libc::tcsetattr(fd, actions as c_int, &*inner_termios)
libc::tcsetattr(
fd.as_fd().as_raw_fd(),
actions as c_int,
&*inner_termios,
)
})
.map(drop)
}

/// Block until all output data is written (see
/// [tcdrain(3p)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/tcdrain.html)).
pub fn tcdrain(fd: RawFd) -> Result<()> {
Errno::result(unsafe { libc::tcdrain(fd) }).map(drop)
pub fn tcdrain<Fd: AsFd>(fd: &Fd) -> Result<()> {
Errno::result(unsafe { libc::tcdrain(fd.as_fd().as_raw_fd()) }).map(drop)
}

/// Suspend or resume the transmission or reception of data (see
/// [tcflow(3p)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/tcflow.html)).
///
/// `tcflow()` suspends of resumes the transmission or reception of data for the given port
/// depending on the value of `action`.
pub fn tcflow(fd: RawFd, action: FlowArg) -> Result<()> {
Errno::result(unsafe { libc::tcflow(fd, action as c_int) }).map(drop)
pub fn tcflow<Fd: AsFd>(fd: &Fd, action: FlowArg) -> Result<()> {
Errno::result(unsafe {
libc::tcflow(fd.as_fd().as_raw_fd(), action as c_int)
})
.map(drop)
}

/// Discard data in the output or input queue (see
/// [tcflush(3p)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/tcflush.html)).
///
/// `tcflush()` will discard data for a terminal port in the input queue, output queue, or both
/// depending on the value of `action`.
pub fn tcflush(fd: RawFd, action: FlushArg) -> Result<()> {
Errno::result(unsafe { libc::tcflush(fd, action as c_int) }).map(drop)
pub fn tcflush<Fd: AsFd>(fd: &Fd, action: FlushArg) -> Result<()> {
Errno::result(unsafe {
libc::tcflush(fd.as_fd().as_raw_fd(), action as c_int)
})
.map(drop)
}

/// Send a break for a specific duration (see
/// [tcsendbreak(3p)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/tcsendbreak.html)).
///
/// When using asynchronous data transmission `tcsendbreak()` will transmit a continuous stream
/// of zero-valued bits for an implementation-defined duration.
pub fn tcsendbreak(fd: RawFd, duration: c_int) -> Result<()> {
Errno::result(unsafe { libc::tcsendbreak(fd, duration) }).map(drop)
pub fn tcsendbreak<Fd: AsFd>(fd: &Fd, duration: c_int) -> Result<()> {
Errno::result(unsafe {
libc::tcsendbreak(fd.as_fd().as_raw_fd(), duration)
})
.map(drop)
}

feature! {
#![feature = "process"]
/// Get the session controlled by the given terminal (see
/// [tcgetsid(3)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/tcgetsid.html)).
pub fn tcgetsid(fd: RawFd) -> Result<Pid> {
let res = unsafe { libc::tcgetsid(fd) };
pub fn tcgetsid<Fd: AsFd>(fd: &Fd) -> Result<Pid> {
let res = unsafe { libc::tcgetsid(fd.as_fd().as_raw_fd()) };

Errno::result(res).map(Pid::from_raw)
}
Expand Down
59 changes: 15 additions & 44 deletions test/sys/test_termios.rs
Original file line number Diff line number Diff line change
@@ -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();
}
}

Expand All @@ -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
Expand All @@ -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.
Expand All @@ -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());
}

Expand All @@ -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.
Expand All @@ -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);
}
Loading

0 comments on commit a2356ab

Please sign in to comment.