From 5fd59d1420f43eeac1a14331556a8c2fc5ea3026 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Thu, 31 Oct 2024 17:18:47 -0400
Subject: [PATCH] cmdext: Fix bug when srcfd == targetfd

See https://github.com/ostreedev/ostree-rs-ext/issues/664

Basically in the case when the source fd number is the same
as the target, `dup2` is a no-op. This is an astoundingly evil
bug because it means we just don't pass the expected
fd to the child process.

Fix this by detecting this situation and just stripping off
`O_CLOEXEC`.

Signed-off-by: Colin Walters <walters@verbum.org>
---
 src/cmdext.rs | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/src/cmdext.rs b/src/cmdext.rs
index a76f6ca..122541f 100644
--- a/src/cmdext.rs
+++ b/src/cmdext.rs
@@ -10,6 +10,8 @@ use cap_std::io_lifetimes;
 use cap_tempfile::cap_std;
 use io_lifetimes::OwnedFd;
 use rustix::fd::{AsFd, FromRawFd, IntoRawFd};
+use rustix::io::FdFlags;
+use std::os::fd::AsRawFd;
 use std::os::unix::process::CommandExt;
 use std::sync::Arc;
 
@@ -30,7 +32,15 @@ impl CapStdExtCommandExt for std::process::Command {
         unsafe {
             self.pre_exec(move || {
                 let mut target = OwnedFd::from_raw_fd(target);
-                rustix::io::dup2(&*fd, &mut target)?;
+                // If the fd is already what we want, then just ensure that
+                // O_CLOEXEC is stripped off.
+                if target.as_raw_fd() == fd.as_raw_fd() {
+                    let fl = rustix::io::fcntl_getfd(&target)?;
+                    rustix::io::fcntl_setfd(&mut target, fl.difference(FdFlags::CLOEXEC))?;
+                } else {
+                    // Otherwise create a dup, which will also default to not setting O_CLOEXEC.
+                    rustix::io::dup2(&*fd, &mut target)?;
+                }
                 // Intentionally leak into the child.
                 let _ = target.into_raw_fd();
                 Ok(())
@@ -49,3 +59,36 @@ impl CapStdExtCommandExt for std::process::Command {
         self
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::sync::Arc;
+
+    #[test]
+    fn test_take_fdn_notequal() -> anyhow::Result<()> {
+        // Pass srcfd != destfd
+        let tempd = cap_tempfile::TempDir::new(cap_std::ambient_authority())?;
+        let tempd_fd = Arc::new(tempd.as_fd().try_clone_to_owned()?);
+        let st = std::process::Command::new("ls")
+            .arg("/proc/self/fd/3")
+            .take_fd_n(tempd_fd, 3)
+            .status()?;
+        assert!(st.success());
+        Ok(())
+    }
+
+    #[test]
+    fn test_take_fdn_equal() -> anyhow::Result<()> {
+        // Pass srcfd == destfd
+        let tempd = cap_tempfile::TempDir::new(cap_std::ambient_authority())?;
+        let tempd_fd = Arc::new(tempd.as_fd().try_clone_to_owned()?);
+        let n = tempd_fd.as_fd().as_raw_fd();
+        let st = std::process::Command::new("ls")
+            .arg(format!("/proc/self/fd/{n}"))
+            .take_fd_n(tempd_fd, n)
+            .status()?;
+        assert!(st.success());
+        Ok(())
+    }
+}