From 3a0803d5405b6022073369c9a508773a331bcfca 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 | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/src/cmdext.rs b/src/cmdext.rs
index a76f6ca..28fe7e7 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,25 @@ impl CapStdExtCommandExt for std::process::Command {
         self
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::sync::Arc;
+
+    #[test]
+    fn test_take_fdn() -> anyhow::Result<()> {
+        // Pass srcfd == destfd and srcfd != destfd
+        for i in 0..1 {
+            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_raw_fd() + i;
+            let st = std::process::Command::new("ls")
+                .arg(format!("/proc/self/fd/{n}"))
+                .take_fd_n(tempd_fd, n)
+                .status()?;
+            assert!(st.success());
+        }
+        Ok(())
+    }
+}