Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential double close in PtyMaster #659

Closed
asomers opened this issue Jul 8, 2017 · 12 comments
Closed

Potential double close in PtyMaster #659

asomers opened this issue Jul 8, 2017 · 12 comments
Assignees
Labels

Comments

@asomers
Copy link
Member

asomers commented Jul 8, 2017

PtyMaster implements Drop to close its RawFd. However, it ignores errors, because the caller may have manually closed the RawFd. This is bad, because it can lead to double closes, like this:

{
    let m = posix_openpt(O_RDWR);		# Creates a pty with file descriptor 3
    close(m.master);				# Close file descriptor 3
    let f = std::fs::File::create("foo");	# Creates a file with file descriptor 3
}						# PtyMaster::Drop closes file descriptor 3
f.write("whatever");				# fails with EBADF

There are three possible solutions:

  1. Always check for errors in PtyMaster::Drop
  2. Don't implement PtyMaster::Drop; make the caller responsible for closing it. If we go this route, we should also add a PtyMaster::close method.
  3. Change PtyMaster to wrap an Option<RawFd> and have the close method set it to None. That way, the drop method will know whether to call close.

I prefer option 1, but I could go with any of these solutions.

@asomers
Copy link
Member Author

asomers commented Jul 8, 2017

Also, the exact same problem is present in SignalFd. For consistency, we should fix both classes the same way.

This was referenced Jul 8, 2017
@Susurrus
Copy link
Contributor

Note that your example doesn't actually work since f goes out of scope when the block drops.

@asomers
Copy link
Member Author

asomers commented Jul 10, 2017

Touche. How about this:

let f = {
    let m = posix_openpt(O_RDWR);		# Creates a pty with file descriptor 3
    close(m.master);				# Close file descriptor 3
    std::fs::File::create("foo");	        # Creates a file with file descriptor 3
}						# PtyMaster::Drop closes file descriptor 3
f.write("whatever");                            # fails with EBADF	

@Susurrus
Copy link
Contributor

👍

For option 1, what do we do when we detect an error? I think all you can do in Drop is panic, is that what we want to do?

@asomers
Copy link
Member Author

asomers commented Jul 10, 2017

The same issue was discussed in libstd at rust-lang/rust#19028 . Panicing during drop is frowned upon because drop runs during stack unwinding. But in this case I think it's the best option. Let's panic in EBADF and ignore other errors like EIO. And once we're satisfied with our solution, we should file another issue for FileDesc at libstd.

@Susurrus
Copy link
Contributor

@asomers Just realized that you assigned this to me. So I guess I'll dig into this :-)

So here's a working version:

extern crate nix;

use std::io::prelude::*;
use std::os::unix::io::IntoRawFd;

fn main() {
    let mut f = {
        let m = nix::pty::posix_openpt(nix::fcntl::O_RDWR).unwrap();
        nix::unistd::close(m.into_raw_fd()).unwrap();
        std::fs::File::create("foo").unwrap()
    };
    f.write("whatever".as_bytes()).unwrap();
}

And here's one that fails at the f.write() call with error code 9 "Bad file descriptor":

extern crate nix;

use std::io::prelude::*;
use std::os::unix::io::AsRawFd;

fn main() {
    let mut f = {
        let m = nix::pty::posix_openpt(nix::fcntl::O_RDWR).unwrap();
        nix::unistd::close(m.as_raw_fd()).unwrap();
        std::fs::File::create("foo").unwrap()
    };
    f.write("whatever".as_bytes()).unwrap();
}

So I actually think this behavior is correct. Or should close have actually failed here?

@asomers
Copy link
Member Author

asomers commented Jul 15, 2017

@Susurrus I just now started working on this myself! Do you want me to self-assign it and take care of it? I think your first example is correct. But I think the second example should've failed when m got dropped.

@Susurrus
Copy link
Contributor

@asomers I just want you to use the assign field correctly! :-P

You seem to understand this issue, so you're welcome to take this.

Also, when writing out this code example I think that unistd::close() should actually take a T: IntoRawFd which improves its ergonomics a bit. Any thoughts on that? Also many other functions should take AsRawFd where appropriate. That'll make working with posix_openpt or any wrapper type we eventually implement much more ergonomic.

@asomers asomers assigned asomers and unassigned Susurrus Jul 15, 2017
@asomers
Copy link
Member Author

asomers commented Jul 15, 2017

Making close take an T: IntoRawFd would certainly prevent some occurrences of this bug. But as long as PtyMaster implements AsRawFd, it can't prevent all of them. Do you think that would confuse any nix consumers who aren't expecting close to consume an object?

Separately, I agree that making most Nix functions accept an T: AsRawFd instead of a RawFd would be a great idea.

@Susurrus
Copy link
Contributor

I think we might want to explicitly address this situation in the docs for close, yes. But also when dealing with this stuff the user needs to be careful in general, and there's only so much hand-holding we can do.

Cool, I'll file a PR for the AsRawFd stuff. It'll be a nice ergonomics win I'd like to get in the next release.

@Susurrus
Copy link
Contributor

Regarding the AsRawFd/IntoRawFd stuff, this doesn't have very good ergonomics because RawFd doesn't implement either of these traits. It was originally merged by @Mic92 in rust-lang/rust#40482 and then quickly reverted in rust-lang/rust#41035 because RawFd is just an alias for i32, so it would have implemented these for that which would've been bad. I'm going to file another ticket with upstream to see what can be done about this.

asomers added a commit to asomers/nix that referenced this issue Jul 15, 2017
@asomers
Copy link
Member Author

asomers commented Jul 15, 2017

Good find. Sounds like a case for a NewType!

@Susurrus Susurrus added the A-bug label Jul 15, 2017
asomers added a commit to asomers/nix that referenced this issue Jul 15, 2017
asomers added a commit to asomers/nix that referenced this issue Jul 16, 2017
Also, document the double-close risk with unistd::close

Fixes nix-rust#659
bors bot added a commit that referenced this issue Jul 16, 2017
677: PtyMaster::drop should panic on EBADF r=asomers

Fixes #659
@bors bors bot closed this as completed in #677 Jul 16, 2017
bors bot added a commit that referenced this issue Dec 8, 2022
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants