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

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

Author: sunfishcode

examples/dup2_to_replace_stdio.rs

@@ -16,15 +16,16 @@ fn main() {
     // that stdin and stdout will be open or safe to use. It's ok here, because
     // we're directly inside `main`, so we know that stdin and stdout haven't
     // been closed and aren't being used for other purposes.
-    let (stdin, stdout) = unsafe { (rustix::io::take_stdin(), rustix::io::take_stdout()) };
+    let (mut stdin, mut stdout) = unsafe { (rustix::io::take_stdin(), rustix::io::take_stdout()) };
 
     // Use `dup2` to copy our new file descriptors over the stdio file descriptors.
     //
-    // These take their second argument as an `&OwnedFd` rather than the usual
-    // `impl AsFd` because they conceptually do a `close` on the original file
-    // descriptor, which one shouldn't be able to do with just a `BorrowedFd`.
-    dup2(&reader, &stdin).unwrap();
-    dup2(&writer, &stdout).unwrap();
+    // These take their second argument as an `&mut OwnedFd` rather than the
+    // usual `impl AsFd` because they conceptually do a `close` on the original
+    // file descriptor, which one shouldn't be able to do with just a
+    // `BorrowedFd`.
+    dup2(&reader, &mut stdin).unwrap();
+    dup2(&writer, &mut stdout).unwrap();
 
     // Then, forget the stdio `OwnedFd`s, because actually dropping them would
     // close them. Here, we want stdin and stdout to remain open for the rest

src/imp/libc/io/syscalls.rs

@@ -375,7 +375,7 @@ pub(crate) fn dup(fd: BorrowedFd<'_>) -> io::Result<OwnedFd> {
 }
 
 #[cfg(not(target_os = "wasi"))]
-pub(crate) fn dup2(fd: BorrowedFd<'_>, new: &OwnedFd) -> io::Result<()> {
+pub(crate) fn dup2(fd: BorrowedFd<'_>, new: &mut OwnedFd) -> io::Result<()> {
     unsafe { ret_discarded_fd(c::dup2(borrowed_fd(fd), borrowed_fd(new.as_fd()))) }
 }
 
@@ -387,7 +387,7 @@ pub(crate) fn dup2(fd: BorrowedFd<'_>, new: &OwnedFd) -> io::Result<()> {
     target_os = "redox",
     target_os = "wasi"
 )))]
-pub(crate) fn dup3(fd: BorrowedFd<'_>, new: &OwnedFd, flags: DupFlags) -> io::Result<()> {
+pub(crate) fn dup3(fd: BorrowedFd<'_>, new: &mut OwnedFd, flags: DupFlags) -> io::Result<()> {
     unsafe {
         ret_discarded_fd(c::dup3(
             borrowed_fd(fd),
@@ -404,14 +404,11 @@ pub(crate) fn dup3(fd: BorrowedFd<'_>, new: &OwnedFd, flags: DupFlags) -> io::Re
     target_os = "ios",
     target_os = "redox"
 ))]
-pub(crate) fn dup3(fd: BorrowedFd<'_>, new: &OwnedFd, _flags: DupFlags) -> io::Result<()> {
+pub(crate) fn dup3(fd: BorrowedFd<'_>, new: &mut OwnedFd, _flags: DupFlags) -> io::Result<()> {
     // Android 5.0 has `dup3`, but libc doesn't have bindings. Emulate it
-    // using `dup2`, including the difference of failing with `EINVAL`
-    // when the file descriptors are equal.
-    use std::os::unix::io::AsRawFd;
-    if fd.as_raw_fd() == new.as_raw_fd() {
-        return Err(io::Errno::INVAL);
-    }
+    // using `dup2`. We don't need to worry about the difference between
+    // `dup2` and `dup3` when the file descriptors are equal because we
+    // have an `&mut OwnedFd` which means `fd` doesn't alias it.
     dup2(fd, new)
 }
 

src/imp/linux_raw/io/syscalls.rs

@@ -394,15 +394,12 @@ pub(crate) fn dup(fd: BorrowedFd<'_>) -> io::Result<OwnedFd> {
 }
 
 #[inline]
-pub(crate) fn dup2(fd: BorrowedFd<'_>, new: &OwnedFd) -> io::Result<()> {
+pub(crate) fn dup2(fd: BorrowedFd<'_>, new: &mut OwnedFd) -> io::Result<()> {
     #[cfg(any(target_arch = "aarch64", target_arch = "riscv64"))]
     {
-        // `dup3` fails if the old and new file descriptors have the same
-        // value, so emulate the `dup2` behavior.
-        use crate::fd::AsRawFd;
-        if fd.as_raw_fd() == new.as_raw_fd() {
-            return Ok(());
-        }
+        // We don't need to worry about the difference between `dup2` and
+        // `dup3` when the file descriptors are equal because we have an
+        // `&mut OwnedFd` which means `fd` doesn't alias it.
         dup3(fd, new, DupFlags::empty())
     }
 
@@ -413,7 +410,7 @@ pub(crate) fn dup2(fd: BorrowedFd<'_>, new: &OwnedFd) -> io::Result<()> {
 }
 
 #[inline]
-pub(crate) fn dup3(fd: BorrowedFd<'_>, new: &OwnedFd, flags: DupFlags) -> io::Result<()> {
+pub(crate) fn dup3(fd: BorrowedFd<'_>, new: &mut OwnedFd, flags: DupFlags) -> io::Result<()> {
     unsafe { ret_discarded_fd(syscall_readonly!(__NR_dup3, fd, new.as_fd(), flags)) }
 }
 

src/io/dup.rs

@@ -31,13 +31,16 @@ pub fn dup<Fd: AsFd>(fd: Fd) -> io::Result<OwnedFd> {
     imp::io::syscalls::dup(fd.as_fd())
 }
 
-/// `dup2(fd, new)`—Creates a new `OwnedFd` instance that shares the
-/// same underlying [file description] as the existing `OwnedFd` instance,
-/// closing `new` and reusing its file descriptor.
+/// `dup2(fd, new)`—Changes the [file description] of a file descriptor.
+///
+/// `dup2` conceptually closes `new` and then sets the file description for
+/// `new` to be the same as the one for `fd`. This is a very unusual operation,
+/// and should only be used on file descriptors where you know how `new` will
+/// be subsequently used.
 ///
 /// This function does not set the `O_CLOEXEC` flag. To do a `dup2` that does
 /// set `O_CLOEXEC`, use [`dup3`] with [`DupFlags::CLOEXEC`] on platforms which
-/// support it.
+/// support it, or [`fcntl_dupfd_cloexec`]
 ///
 /// # References
 ///  - [POSIX]
@@ -46,15 +49,15 @@ pub fn dup<Fd: AsFd>(fd: Fd) -> io::Result<OwnedFd> {
 /// [file description]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_258
 /// [POSIX]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/dup2.html
 /// [Linux]: https://man7.org/linux/man-pages/man2/dup2.2.html
+/// [`fcntl_dupfd_cloexec`]: crate::fs::fcntl_dupfd_cloexec
 #[cfg(not(target_os = "wasi"))]
 #[inline]
-pub fn dup2<Fd: AsFd>(fd: Fd, new: &OwnedFd) -> io::Result<()> {
+pub fn dup2<Fd: AsFd>(fd: Fd, new: &mut OwnedFd) -> io::Result<()> {
     imp::io::syscalls::dup2(fd.as_fd(), new)
 }
 
-/// `dup3(fd, new, flags)`—Creates a new `OwnedFd` instance that shares the
-/// same underlying [file description] as the existing `OwnedFd` instance,
-/// closing `new` and reusing its file descriptor, with flags.
+/// `dup3(fd, new, flags)`—Changes the [file description] of a file
+/// descriptor, with flags.
 ///
 /// `dup3` is the same as [`dup2`] but adds an additional flags operand,
 /// and it fails in the case that `fd` and `new` have the same file descriptor
@@ -70,6 +73,6 @@ pub fn dup2<Fd: AsFd>(fd: Fd, new: &OwnedFd) -> io::Result<()> {
 /// [Linux]: https://man7.org/linux/man-pages/man2/dup2.2.html
 #[cfg(not(target_os = "wasi"))]
 #[inline]
-pub fn dup3<Fd: AsFd>(fd: Fd, new: &OwnedFd, flags: DupFlags) -> io::Result<()> {
+pub fn dup3<Fd: AsFd>(fd: Fd, new: &mut OwnedFd, flags: DupFlags) -> io::Result<()> {
     imp::io::syscalls::dup3(fd.as_fd(), new, flags)
 }

tests/io/dup3.rs

@@ -7,9 +7,6 @@
 #[cfg(not(windows))]
 #[cfg(not(target_os = "wasi"))]
 mod dup2_to_replace_stdio;
-#[cfg(not(windows))]
-#[cfg(not(target_os = "wasi"))]
-mod dup3;
 #[cfg(not(feature = "rustc-dep-of-std"))] // TODO
 #[cfg(not(windows))]
 #[cfg(feature = "net")]