Skip to content

Commit

Permalink
cmdext: Fix bug when srcfd == targetfd
Browse files Browse the repository at this point in the history
See ostreedev/ostree-rs-ext#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 <[email protected]>
  • Loading branch information
cgwalters committed Oct 31, 2024
1 parent bfbb355 commit 5fd59d1
Showing 1 changed file with 44 additions and 1 deletion.
45 changes: 44 additions & 1 deletion src/cmdext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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(())
Expand All @@ -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(())
}
}

0 comments on commit 5fd59d1

Please sign in to comment.