PtyMaster::drop should panic on EBADF #677
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
LGTM. This bug was never in a released version of nix, so we're fine with no CHANGELOG entry. Is it worth adding any doccomments somewhere that elaborate on this case so people understand what could happen? I'm not certain there is, but I wanted to make sure you thought about it. |
|
I have no idea why i686-unknown-linux-musl keeps failing. It passed 10,000 times in a row on my system. QEMU is not to blame; i686 doesn't use it. @Susurrus any ideas? |
|
Please squash these into a single commit. And then it's r+ by me. |
Also, document the double-close risk with unistd::close Fixes nix-rust#659
|
bors r+ susurrus |
Build succeeded |
bors bot
added a commit
that referenced
this pull request
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 <stevelauc@outlook.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #659