Fix soundness issues in dup2_stdout et al (#1080)

* Fix `dup2_stdout` et al to handle incorrect `AsFd` implementations

Without this change, an incorrect `AsFd` implementation could result to
the stdio file descriptors being closed in safe code, violating I/O safety.
The old `dup2_std*` implementations were thus unsound.

Consider this old version of the code:

```rust
if fd.as_fd().as_raw_fd() != c::STDOUT_FILENO {
    // SAFETY: We pass the returned `OwnedFd` to `forget` so that it isn't
    // dropped.
    let mut target = unsafe { take_stdout() };
    backend::io::syscalls::dup2(fd.as_fd(), &mut target)?;
    forget(target);
}
```

The safety of the `take_stdout()` call relies on two things:

1. In the call to `syscalls::dup2`, `fd` and `target` don't alias (#1069).
2. `forget(target)` is called to not drop `target`.

If one of these conditions is violated, then stdout of the process can get
closed. Violating both of them is possible in entirely safe code, because
`AsFd` is not an unsafe trait. Consider a user defined type that (safely)
implements `AsFd` and

1. Returns `stdin()` on the first `as_fd()` call, but `stdout()` on the
   second time `as_fd()` is called. This would bypass the aliasing check,
   because the `dup2_stdout` implementation calls `as_fd()` twice, and the
   first result is used in the check and second result in the actual `dup2`
   call.
2. Or returns `stdin()` on the first `as_fd()` call, but panics on the
   second call. The panic would happen on the same line as the `dup2` call,
   so `forget(target)` after it isn't called, which results in `target`
   getting dropped which closes stdout. The panic could be catched with
   `catch_unwind` that would let other code observe the missing stdout.
   This violates exception safety.

This patch fixes both of these issues by calling `as_fd()` only once and
storing the `BorrowedFd` into a variable. This ensures that the file
descriptor in the alias check and the `dup2` call are the same, and a panic
in `as_fd()` would happen before the unsafe `take_stdout` call, so there is
no concern of `target` getting dropped because of a panic.

* Fix `dup2_stdout` et al to not close the stdio file descriptor on error

The `dup2_std*` functions use the `?` operator to return errors from the
`dup2` system call. However, this means that the code after it
(`forget(target)`) is not run on errors, so `target` gets dropped closing
the file descriptor which violates I/O safety.

Fix that by using `ManuallyDrop` instead of `forget`.

Author: pekkarr

src/stdio.rs

@@ -17,7 +17,7 @@ use backend::fd::{BorrowedFd, FromRawFd, RawFd};
 use {
     crate::io,
     backend::fd::{AsFd, AsRawFd},
-    core::mem::forget,
+    core::mem::ManuallyDrop,
 };
 
 /// `STDIN_FILENO`—Standard input, borrowed.
@@ -477,45 +477,42 @@ pub const fn raw_stderr() -> RawFd {
 
 /// Utility function to safely `dup2` over stdin (fd 0).
 #[cfg(not(any(windows, target_os = "wasi")))]
-#[allow(clippy::mem_forget)]
 #[inline]
 pub fn dup2_stdin<Fd: AsFd>(fd: Fd) -> io::Result<()> {
-    if fd.as_fd().as_raw_fd() != c::STDIN_FILENO {
-        // SAFETY: We pass the returned `OwnedFd` to `forget` so that it isn't
-        // dropped.
-        let mut target = unsafe { take_stdin() };
-        backend::io::syscalls::dup2(fd.as_fd(), &mut target)?;
-        forget(target);
+    let fd = fd.as_fd();
+    if fd.as_raw_fd() != c::STDIN_FILENO {
+        // SAFETY: We wrap the returned `OwnedFd` to `ManuallyDrop` so that it
+        // isn't dropped.
+        let mut target = ManuallyDrop::new(unsafe { take_stdin() });
+        backend::io::syscalls::dup2(fd, &mut target)?;
     }
     Ok(())
 }
 
 /// Utility function to safely `dup2` over stdout (fd 1).
 #[cfg(not(any(windows, target_os = "wasi")))]
-#[allow(clippy::mem_forget)]
 #[inline]
 pub fn dup2_stdout<Fd: AsFd>(fd: Fd) -> io::Result<()> {
-    if fd.as_fd().as_raw_fd() != c::STDOUT_FILENO {
-        // SAFETY: We pass the returned `OwnedFd` to `forget` so that it isn't
-        // dropped.
-        let mut target = unsafe { take_stdout() };
-        backend::io::syscalls::dup2(fd.as_fd(), &mut target)?;
-        forget(target);
+    let fd = fd.as_fd();
+    if fd.as_raw_fd() != c::STDOUT_FILENO {
+        // SAFETY: We wrap the returned `OwnedFd` to `ManuallyDrop` so that it
+        // isn't dropped.
+        let mut target = ManuallyDrop::new(unsafe { take_stdout() });
+        backend::io::syscalls::dup2(fd, &mut target)?;
     }
     Ok(())
 }
 
 /// Utility function to safely `dup2` over stderr (fd 2).
 #[cfg(not(any(windows, target_os = "wasi")))]
-#[allow(clippy::mem_forget)]
 #[inline]
 pub fn dup2_stderr<Fd: AsFd>(fd: Fd) -> io::Result<()> {
-    if fd.as_fd().as_raw_fd() != c::STDERR_FILENO {
-        // SAFETY: We pass the returned `OwnedFd` to `forget` so that it isn't
-        // dropped.
-        let mut target = unsafe { take_stderr() };
-        backend::io::syscalls::dup2(fd.as_fd(), &mut target)?;
-        forget(target);
+    let fd = fd.as_fd();
+    if fd.as_raw_fd() != c::STDERR_FILENO {
+        // SAFETY: We wrap the returned `OwnedFd` to `ManuallyDrop` so that it
+        // isn't dropped.
+        let mut target = ManuallyDrop::new(unsafe { take_stderr() });
+        backend::io::syscalls::dup2(fd, &mut target)?;
     }
     Ok(())
 }