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

Change dup2's second operand from &OwnedFd to &mut OwnedFd. #332

Merged
merged 1 commit into from
May 26, 2022

Conversation

sunfishcode
Copy link
Member

And similar for dup3.

The idea behind using &OwnedFd is that dup2's second operand isn't like a
normal borrow. It effectively closes the old file descriptor, and creates a
new one with the same index. This could break assumptions of classes that have
an AsFd to allow users to do special I/O operations, but which don't expect
users can close and reopen their file descriptor as some completely unrelated
resource.

However, the existence of things like FilelikeView, as well as the
ManuallyDrop pattern, mean that &OwnedFd doesn't actually prevent
users from using dup2 on a BorrowedFd.

With sunfishcode/io-lifetimes#32 though, &mut OwnedFd would be
sufficient, because it removes the DerefMut implementation.

So change rustix stance to be that dup2 requires &mut OwnedFd.

This means that it's no longer possible to pass the same file descriptor
to both operands of dup2 or dup3 with safe Rust, which means it's not
possible to observe the difference in behavior in that case, so remove
the dup3.rs test.

And similar for `dup3`.

The idea behind using `&OwnedFd` is that `dup2`'s second operand isn't like a
normal borrow. It effectively closes the old file descriptor, and creates a
new one with the same index. This could break assumptions of classes that have
an `AsFd` to allow users to do special I/O operations, but which don't expect
users can close and reopen their file descriptor as some completely unrelated
resource.

However, the existence of things like [`FilelikeView`], as well as the
`ManuallyDrop` pattern, mean that `&OwnedFd` doesn't actually prevent
users from using `dup2` on a `BorrowedFd`.

With sunfishcode/io-lifetimes#32 though, `&mut OwnedFd` would be
sufficient, because it removes the `DerefMut` implementation.

So change `rustix` stance to be that `dup2` requires `&mut OwnedFd`.

This means that it's no longer possible to pass the same file descriptor
to both operands of `dup2` or `dup3` with safe Rust, which means it's not
possible to observe the difference in behavior in that case, so remove
the `dup3.rs` test.

[`FilelikeView`]: https://docs.rs/io-lifetimes/latest/io_lifetimes/views/struct.FilelikeView.html
@sunfishcode
Copy link
Member Author

See sunfishcode/io-lifetimes#32 (comment) for some discussion of this.

@sunfishcode
Copy link
Member Author

I also like how &mut OwnedFd is also what you need if you want to reassign an OwnedFd to an unrelated OwnedFd value, which is conceptually very similar to what dup2 does.

@sunfishcode
Copy link
Member Author

This also aligns with what's written about dup2 in the standard library documentation.

And, this is sufficient to support the redirect-stdio-with-dup2 use case, which is the most common use case for dup2.

@sunfishcode
Copy link
Member Author

Merging, as there don't seem to be any major issues here. And we can always revisit this if the thinking around views or anything else changes.

@sunfishcode sunfishcode merged commit d02a6ba into main May 26, 2022
@sunfishcode sunfishcode deleted the sunfishcode/dup2-mut-ownedfd branch May 26, 2022 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver bump Issues that will require a semver-incompatible fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant