Skip to content

add std::os::unix::process::CommandExt::fd #145687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Qelxiros wants to merge 5 commits into
base: main
Choose a base branch
from
Open

add std::os::unix::process::CommandExt::fd #145687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
756b94c
add std::os::unix::process::CommandExt::fd
Qelxiros Aug 20, 2025
b3ff6bd
skip addclose when old_fd == new_fd
Qelxiros Sep 17, 2026
a32b7eb
std: Add a test for `CommandExt::fd` clobbering child status FDs
tgross35 Sep 19, 2026
bf24f0e
formatting
Qelxiros Sep 21, 2026
c5f935e
don't clobber pipe fd
Qelxiros Sep 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions library/std/src/os/unix/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,89 @@ pub impl(self) trait CommandExt {

#[unstable(feature = "process_setsid", issue = "105376")]
fn setsid(&mut self, setsid: bool) -> &mut process::Command;

/// Pass a file descriptor to a child process.
///
/// `old_fd` is an open file descriptor in the parent process. This descriptor will be
/// duplicated in the child process and associated with the file descriptor number `new_fd`.
///
/// Getting this right is tricky. It is recommended to provide further information to the child
/// process by some other mechanism. This could be an argument confirming file descriptors that
/// the child can use, device/inode numbers to allow for sanity checks, or something similar.
///
/// If `old_fd` is an open file descriptor in the child process (e.g. if multiple parent fds are being
/// mapped to the same child one) and closing it would produce one or more errors,
/// those errors will be lost when this function is called. See
/// [`man 2 dup`] for more information.
///
/// [`man 2 dup`]: https://pubs.opengroup.org/onlinepubs/9799919799/functions/dup.html
/// ```
/// #![feature(command_pass_fds)]
///
/// use std::process::{Command, Stdio};
/// use std::os::unix::process::CommandExt;
/// use std::io::{self, Write};
///
/// # fn main() -> io::Result<()> {
/// let (pipe_reader, mut pipe_writer) = io::pipe()?;
///
/// let fd_num = 123;
///
/// let mut cmd = Command::new("cat");
/// cmd.arg(format!("/dev/fd/{fd_num}")).stdout(Stdio::piped()).fd(fd_num, pipe_reader);
///
/// let mut child = cmd.spawn()?;
/// let mut stdout = child.stdout.take().unwrap();
///
/// pipe_writer.write_all(b"Hello, world!")?;
/// drop(pipe_writer);
///
/// child.wait()?;
/// assert_eq!(io::read_to_string(&mut stdout)?, "Hello, world!");
///
/// # Ok(())
/// # }
/// ```
///
/// If this method is called multiple times with the same `new_fd`, all but one file descriptor
/// will be lost.
Comment on lines +265 to +266

@Mark-Simulacrum Mark-Simulacrum Sep 19, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what "file descriptor will be lost" means -- losing an integer is not really a thing, we should say something about closing or not mapping certain fds. I think the semantics we intend is something like:

If new_fd is passed multiple times to this method, or overlaps with inherited fds (e.g., stdin/stderr), then it is not specified which source (old_fd) will end up duplicated into new_fd.

I think what @joboet is getting at in the other thread is that this is inherently in conflict with I/O safety, which states

acting on a file descriptor without proof of ownership can lead to misbehavior and even Undefined Behavior in code that relies on ownership of its file descriptors

Here we are closing arbitrary target fds. My sense is that this API is not really usable as-is. I think the only possible API shape is for the caller to pre-allocate the target FDs for us, e.g., by passing old_fd: OwnedFd, new_fd: OwnedFd; we would then dup2 into the new_fds safely. Or am I missing something?

View changes since the review

@tgross35 tgross35 Sep 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm: I think this may be safe since we're duping in the child so the parent's originals remain untouched. And afaict we're not claiming exclusive ownership of the FDs except for the output.

This would be a problem if e.g. you hand the child a list of "safe" FDs that it can assume from_raw_fd on, without realizing that it contains duplicates because some non-cloexec FD happens to overlap with a value set in .fd. Perhaps it's worth a note that the child is still responsible for ensuring it only opens one of each FD?

@Mark-Simulacrum Mark-Simulacrum Sep 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s a good point that the damage/risk is mostly (only? not sure if there are kernel APIs that act on any close - iirc, there are bad file locking APIs that unlock on any close) in the child.

But it feels like safe rust code being able to close arbitrary fds in the child is at least border line violating the expected semantics from I/O safety. To some extent that is unavoidable with the APIs present in Linux, but it still seems iffy. I think ideally we would allow dup(fd) semantics with some Rust protocol for retrieving them soundly in the child as owned…

@tgross35 tgross35 Sep 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I certainly don’t disagree, sending numbers around like this is a rather low-level API.

Do you think there’s a good resolution here? Maybe the “It is recommended to provide further information to the child by some other mechanism” bit could be made stronger and put in its own section with examples. And/or an unresolved question on the tracking issue.

@Mark-Simulacrum Mark-Simulacrum Sep 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think unresolved question is probably the right thing. In the short term we could make this unsafe, though I’m not sure what the exact safety condition should be (and how the caller can guarantee it), especially given that the fd table is inherently a global property. Maybe the target fd should be mandated to be an OwnedFd as I suggest above? That’s a bit wasteful but does seem like it satisfies the requirement that you’re not clobbering random fds…

@tgross35 tgross35 Sep 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started #t-libs > Safety of sending file descriptors since the others may have thoughts on this.

///
/// ```
/// #![feature(command_pass_fds)]
///
/// use std::process::{Command, Stdio};
/// use std::os::unix::process::CommandExt;
/// use std::io::{self, Write};
///
/// # fn main() -> io::Result<()> {
/// let (pipe_reader1, mut pipe_writer1) = io::pipe()?;
/// let (pipe_reader2, mut pipe_writer2) = io::pipe()?;
///
/// let fd_num = 123;
///
/// let mut cmd = Command::new("cat");
/// cmd.arg(format!("/dev/fd/{fd_num}"))
/// .stdout(Stdio::piped())
/// .fd(fd_num, pipe_reader1)
/// .fd(fd_num, pipe_reader2);
///
/// pipe_writer1.write_all(b"Hello from pipe 1!")?;
/// drop(pipe_writer1);
///
/// pipe_writer2.write_all(b"Hello from pipe 2!")?;
/// drop(pipe_writer2);
///
/// let mut child = cmd.spawn()?;
/// let mut stdout = child.stdout.take().unwrap();
///
/// child.wait()?;
/// assert_eq!(io::read_to_string(&mut stdout)?, "Hello from pipe 2!");
///
/// # Ok(())
/// # }
/// ```
#[unstable(feature = "command_pass_fds", issue = "144989")]
fn fd(&mut self, new_fd: RawFd, old_fd: impl Into<OwnedFd>) -> &mut Self;
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down Expand Up @@ -273,6 +356,11 @@ impl CommandExt for process::Command {
self.as_inner_mut().setsid(setsid);
self
}

fn fd(&mut self, new_fd: RawFd, old_fd: impl Into<OwnedFd>) -> &mut Self {
self.as_inner_mut().fd(old_fd.into(), new_fd);
self
}
Comment thread
tgross35 marked this conversation as resolved.
Resolved
}

/// Unix-specific extensions to [`ExitStatus`] and [`ExitStatusError`].
Expand Down
36 changes: 36 additions & 0 deletions library/std/src/sys/process/unix/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ pub struct Command {
create_pidfd: bool,
pgroup: Option<pid_t>,
setsid: bool,
/// A map of parent FDs to child FDs to be inherited during spawn.
fds: Vec<(OwnedFd, RawFd)>,
Comment thread
Qelxiros marked this conversation as resolved.
Resolved
/// For testing purposes: store `Some(true)` if the last spawn used `posix_spawn`, `Some(false)`
/// if it used `exec`, and `None` if it hasn't been spawned yet.
#[cfg(test)]
last_spawn_was_posix_spawn: Option<bool>,
Comment thread
Qelxiros marked this conversation as resolved.
Resolved
}

// passed to do_exec() with configuration of what the child stdio should look
Expand Down Expand Up @@ -187,6 +193,9 @@ impl Command {
create_pidfd: false,
pgroup: None,
setsid: false,
fds: Vec::new(),
#[cfg(test)]
last_spawn_was_posix_spawn: None,
}
}

Expand Down Expand Up @@ -372,6 +381,33 @@ impl Command {
let theirs = ChildPipes { stdin: their_stdin, stdout: their_stdout, stderr: their_stderr };
Ok((ours, theirs))
}

pub fn fd(&mut self, old_fd: OwnedFd, new_fd: RawFd) {
self.fds.push((old_fd, new_fd));
}

pub fn get_fds(&self) -> &[(OwnedFd, RawFd)] {
&self.fds
}

/// Clear the fd vector, closing all descriptors owned by this `Command`.
pub fn close_owned_fds(&mut self) {
self.fds.clear();
}
Comment thread
Qelxiros marked this conversation as resolved.
Resolved

#[cfg_attr(not(test), allow(unused_variables))]
pub(crate) fn last_spawn_was_posix_spawn(&mut self, val: bool) {
#[cfg(test)]
{
self.last_spawn_was_posix_spawn = Some(val);
}
}

// we only use this for testing
#[cfg(test)]
pub(crate) fn get_last_spawn_was_posix_spawn(&self) -> Option<bool> {
self.last_spawn_was_posix_spawn
}
Comment thread
Qelxiros marked this conversation as resolved.
Resolved
}

fn os2c(s: &OsStr, saw_nul: &mut bool) -> CString {
Expand Down
55 changes: 52 additions & 3 deletions library/std/src/sys/process/unix/unix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ use libc::{gid_t, uid_t};
use super::common::*;
use crate::io::{self, Error, ErrorKind};
use crate::num::NonZero;
use crate::os::fd::AsRawFd;
#[cfg(not(any(target_os = "tvos", target_os = "watchos")))]
use crate::os::fd::OwnedFd;
use crate::process::StdioPipes;
use crate::sys::cvt;
#[cfg(target_os = "linux")]
use crate::sys::process::PidFd;
use crate::sys::{FromInner, IntoInner, cvt};
use crate::{fmt, mem, sys};

cfg_select! {
Expand Down Expand Up @@ -71,8 +74,12 @@ impl Command {
let (ours, theirs) = self.setup_io(default, needs_stdin)?;

if let Some(ret) = self.posix_spawn(&theirs, envp.as_ref())? {
self.last_spawn_was_posix_spawn(true);
// Close fds in the parent that have been duplicated in the child
self.close_owned_fds();
Comment thread
Qelxiros marked this conversation as resolved.
Resolved
return Ok((ret, ours));
}
self.last_spawn_was_posix_spawn(false);

#[cfg(target_os = "linux")]
let (input, output) = sys::net::Socket::new_pair(libc::AF_UNIX, libc::SOCK_SEQPACKET)?;
Expand Down Expand Up @@ -101,7 +108,15 @@ impl Command {
if self.get_create_pidfd() {
self.send_pidfd(&output);
}
let Err(err) = unsafe { self.do_exec(theirs, envp.as_ref()) };
#[cfg(target_os = "linux")]
let mut output_fd = output.into_inner().into_inner();
#[cfg(not(target_os = "linux"))]
let mut output_fd = output.into_inner();
let Err(err) = unsafe { self.do_exec(theirs, envp.as_ref(), Some(&mut output_fd)) };
#[cfg(target_os = "linux")]
let output = sys::net::Socket::from_inner(sys::fd::FileDesc::from_inner(output_fd));
#[cfg(not(target_os = "linux"))]
let output = sys::fd::FileDesc::from_inner(output_fd);
let errno = err.raw_os_error().unwrap_or(libc::EINVAL) as u32;
let errno = errno.to_be_bytes();
let bytes = [
Expand All @@ -124,6 +139,9 @@ impl Command {
drop(env_lock);
drop(output);

// Close fds in the parent that have been duplicated in the child
self.close_owned_fds();

#[cfg(target_os = "linux")]
let pidfd = if self.get_create_pidfd() { self.recv_pidfd(&input) } else { -1 };

Expand Down Expand Up @@ -241,7 +259,7 @@ impl Command {
// environment lock before we try to exec.
let _lock = sys::env::env_read_lock();

let Err(e) = self.do_exec(theirs, envp.as_ref());
let Err(e) = self.do_exec(theirs, envp.as_ref(), None);
e
}
}
Expand Down Expand Up @@ -284,6 +302,7 @@ impl Command {
&mut self,
stdio: ChildPipes,
maybe_envp: Option<&CStringArray>,
mut preserve_fd: Option<&mut OwnedFd>,
) -> Result<!, io::Error> {
use crate::sys::{self, cvt_r};

Expand All @@ -297,6 +316,22 @@ impl Command {
cvt_r(|| libc::dup2(fd, libc::STDERR_FILENO))?;
}

for &(ref old_fd, new_fd) in self.get_fds() {
if let Some(ref mut preserve_fd) = preserve_fd
&& new_fd == preserve_fd.as_raw_fd()
{
let mut copy = preserve_fd.try_clone()?;
mem::swap(&mut copy, preserve_fd);
// after the swap, copy holds the same file descriptor as new_fd, so we can let dup2
// close it instead
mem::forget(copy);
}
cvt_r(|| libc::dup2(old_fd.as_raw_fd(), new_fd))?;

@joboet joboet Mar 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is problematic and probably unsound (as it violates I/O safety) if new_fd is accidentally equal to the file descriptor number that is used by the pipe or socket used to return errors. That would lead to spawn inaccurately reporting a success, even if an error occurs (since this half of the pipe will be closed).

@Qelxiros Qelxiros Mar 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to check for that? If not, it seems impossible to avoid.

@Qelxiros Qelxiros Mar 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joboet friendly nudge :)

@joboet joboet Mar 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use dup2 (ideally std's try_clone wrapper) to clone the pipe/socket to a different descriptor and then close the original, effectively moving the descriptor.

@Qelxiros Qelxiros Mar 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I don't understand the issue. If new_fd might collide with the pipe/socket, then it might still collide after I move the pipe/socket, right?

@Mark-Simulacrum Mark-Simulacrum Sep 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joboet can you restate the concern? I don't think I'm quite seeing what you're getting at.

@tgross35 tgross35 Sep 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to forking, we create a socket pair or pipe used to communicate with the child. If those happen to get assigned e.g. fd 4 and the user called cmd.fd(4, ...), it's going to clobber the pipe.

I think Jonas is saying this can be mitigated by doing something like pass preserve_fd: &mut OwnedFd (from the relevant side of the socket/pipe) to do_exec and, prior to each dup2 call for user-specified FDs, check if the destination FD is the same as preserve_fd. If so then try_clone preserve_fd and replace it with the new one, and in the calling function recreate the socket/pipe from it.

You can actually mem::swap preserve_fd then mem::forget the result to save an extra close call, since dup2 is specified to close an open destination fd if it already exists.

Great catch Jonas.

@Mark-Simulacrum Mark-Simulacrum Sep 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, yeah, that sounds like it's basically the same issue I'm referencing here - #145687 (comment) - i.e., we're not really satisfying I/O Safety here. I don't thinkt he current API allows for that though, even if we mitigate it for a small set of known FDs.

@tgross35 tgross35 Sep 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was able to write a test that reliably fails because of this but passes with posix-spawn. @Qelxiros could you cherry pick tgross35@986dba3?

@Mark-Simulacrum Mark-Simulacrum Sep 19, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably mention something about CLOEXEC not being set by us somewhere in the docs.

View changes since the review

@tgross35 tgross35 Sep 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dup3 is reasonably widely supported, should we be using that instead where available?

@tgross35 tgross35 Sep 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope that doesn't make sense since we haven't execed yet.

if old_fd.as_raw_fd() != new_fd {
cvt_r(|| libc::close(old_fd.as_raw_fd()))?;
}
}

#[cfg(not(target_os = "l4re"))]
{
if let Some(_g) = self.get_groups() {
Expand Down Expand Up @@ -462,6 +497,7 @@ impl Command {
use core::sync::atomic::{Atomic, AtomicU8, Ordering};

use crate::mem::MaybeUninit;
use crate::os::fd::AsRawFd;
use crate::pin::{Pin, pin};
use crate::sys::helpers::COpaque;
use crate::sys::{self, cvt_nz, on_broken_pipe_used};
Expand Down Expand Up @@ -734,6 +770,19 @@ impl Command {
libc::STDERR_FILENO,
))?;
}
for &(ref old_fd, new_fd) in self.get_fds() {
cvt_nz(libc::posix_spawn_file_actions_adddup2(
file_actions.0.get(),
old_fd.as_raw_fd(),
new_fd,
))?;
if old_fd.as_raw_fd() != new_fd {
cvt_nz(libc::posix_spawn_file_actions_addclose(
file_actions.0.get(),
old_fd.as_raw_fd(),
))?;
}
}
if let Some((f, cwd)) = addchdir {
cvt_nz(f(file_actions.0.get(), cwd.as_ptr()))?;
}
Expand Down
Loading
Loading