Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
101 changes: 101 additions & 0 deletions library/std/src/os/unix/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,98 @@ pub trait CommandExt: Sealed {

#[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 fd will be duplicated in the
/// child process and associated with the fd 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`](https://www.man7.org/linux/man-pages/man2/dup.2.html#NOTES) for more information.
///
/// ```
/// #![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.
///
/// ```
/// #![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;

/// Check if the last `spawn` of this command used `posix_spawn`.
///
/// Returns `None` if the `Command` hasn't been spawned yet.
/// Returns `Some(true)` if the last spawn used [`posix_spawn`].
/// Returns `Some(false)` otherwise.
///
/// [`posix_spawn`]: https://www.man7.org/linux/man-pages/man3/posix_spawn.3.html
#[unstable(feature = "command_pass_fds", issue = "144989")]
fn last_spawn_was_posix_spawn(&self) -> Option<bool>;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem mentioned in the ACP or tracking issue, and seems like an odd place to put this. It's also not clear to me why this is on Command vs on Child?

I'm not convinced we want to make this a stable API -- posix_spawn feels like a low level detail rather than something callers should care about. If we do want this exposed we should explain what callers are supposed to do with the information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for testing purposes (see #145687 (comment)). Would it be better to mark it perma-unstable? (I'm not sure exactly how to do that)

I don't think it can be private because tests are treated as separate crates.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be possible to make it private. Integration tests in library/std/tests are treated as separate crates, but these are in src/ so can use internal API.

}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down Expand Up @@ -268,6 +360,15 @@ 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
}

fn last_spawn_was_posix_spawn(&self) -> Option<bool> {
self.as_inner().get_last_spawn_was_posix_spawn()
}
}

/// Unix-specific extensions to [`process::ExitStatus`] and
Expand Down
28 changes: 28 additions & 0 deletions library/std/src/sys/process/unix/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ 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)>,
// 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.
last_spawn_was_posix_spawn: Option<bool>,
}

// passed to do_exec() with configuration of what the child stdio should look
Expand Down Expand Up @@ -183,6 +188,8 @@ impl Command {
create_pidfd: false,
pgroup: None,
setsid: false,
fds: Vec::new(),
last_spawn_was_posix_spawn: None,
}
}

Expand Down Expand Up @@ -360,6 +367,27 @@ 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();
}

pub fn last_spawn_was_posix_spawn(&mut self, val: bool) {
self.last_spawn_was_posix_spawn = Some(val);
}

pub fn get_last_spawn_was_posix_spawn(&self) -> Option<bool> {
self.last_spawn_was_posix_spawn
}
}

fn os2c(s: &OsStr, saw_nul: &mut bool) -> CString {
Expand Down
25 changes: 25 additions & 0 deletions library/std/src/sys/process/unix/unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use libc::{gid_t, uid_t};
use super::common::*;
use crate::io::{self, Error, ErrorKind};
use crate::num::NonZero;
use crate::os::fd::AsRawFd;
use crate::process::StdioPipes;
use crate::sys::cvt;
#[cfg(target_os = "linux")]
Expand Down Expand Up @@ -71,8 +72,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();
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 @@ -124,6 +129,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 @@ -292,6 +300,11 @@ impl Command {
cvt_r(|| libc::dup2(fd, libc::STDERR_FILENO))?;
}

for &(ref old_fd, new_fd) in self.get_fds() {
cvt_r(|| libc::dup2(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 @@ -455,6 +468,7 @@ impl Command {
use core::sync::atomic::{Atomic, AtomicU8, Ordering};

use crate::mem::MaybeUninit;
use crate::os::fd::AsRawFd;
use crate::sys::{self, cvt_nz, on_broken_pipe_flag_used};

if self.get_gid().is_some()
Expand Down Expand Up @@ -717,6 +731,17 @@ 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.as_mut_ptr(),
old_fd.as_raw_fd(),
new_fd,
))?;
cvt_nz(libc::posix_spawn_file_actions_addclose(
file_actions.0.as_mut_ptr(),
old_fd.as_raw_fd(),
))?;
}
if let Some((f, cwd)) = addchdir {
cvt_nz(f(file_actions.0.as_mut_ptr(), cwd.as_ptr()))?;
}
Expand Down
110 changes: 110 additions & 0 deletions library/std/src/sys/process/unix/unix/tests.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
use crate::fs;
use crate::os::unix::fs::MetadataExt;
use crate::os::unix::process::{CommandExt, ExitStatusExt};
use crate::panic::catch_unwind;
use crate::process::Command;

// Many of the other aspects of this situation, including heap alloc concurrency
// safety etc., are tested in tests/ui/process/process-panic-after-fork.rs

/// Use dev + ino to uniquely identify a file
fn md_file_id(md: &fs::Metadata) -> (u64, u64) {
(md.dev(), md.ino())
}

#[test]
fn exitstatus_display_tests() {
// In practice this is the same on every Unix.
Expand Down Expand Up @@ -74,3 +81,106 @@ fn test_command_fork_no_unwind() {
|| signal == libc::SIGSEGV
);
}

/// For `Command`'s fd-related tests, we want to be sure they work both with exec
/// and with `posix_spawn`. We test both the default which should use `posix_spawn`
/// on supported platforms, and using `pre_exec` to force spawn using `exec`.
mod fd_impls {
use super::{assert_spawn_method, md_file_id};
use crate::fs;
use crate::io::{self, Write};
use crate::os::fd::AsRawFd;
use crate::os::unix::process::CommandExt;
use crate::process::{Command, Stdio};

/// Check setting the child's stdin via `.fd`.
pub fn test_stdin(use_exec: bool) {
let (pipe_reader, mut pipe_writer) = io::pipe().unwrap();

let fd_num = libc::STDIN_FILENO;

let mut cmd = Command::new("cat");
cmd.stdout(Stdio::piped()).fd(fd_num, pipe_reader);

if use_exec {
unsafe {
cmd.pre_exec(|| Ok(()));
}
}

let mut child = cmd.spawn().unwrap();
let mut stdout = child.stdout.take().unwrap();

assert_spawn_method(&cmd, use_exec);

pipe_writer.write_all(b"Hello, world!").unwrap();
drop(pipe_writer);

child.wait().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
child.wait().unwrap();
child.wait().unwrap().exit_ok().unwrap();

Verify the child didn't run into errors, applies a few places

assert_eq!(io::read_to_string(&mut stdout).unwrap(), "Hello, world!");
}

// ensure that the fd is properly closed in the parent, but only after the child is spawned.
pub fn test_close_time(use_exec: bool) {
let (_pipe_reader, pipe_writer) = io::pipe().unwrap();

let fd = pipe_writer.as_raw_fd();
let fd_path = format!("/dev/fd/{fd}");

let mut cmd = Command::new("true");
cmd.fd(123, pipe_writer);

if use_exec {
unsafe {
cmd.pre_exec(|| Ok(()));
}
}

// Get the identifier of the fd (metadata follows symlinks)
let fd_id = md_file_id(&fs::metadata(&fd_path).expect("fd should be open"));

cmd.spawn().unwrap().wait().unwrap();

assert_spawn_method(&cmd, use_exec);

// After the child is spawned, our fd should be closed
match fs::metadata(&fd_path) {
// Ok; fd exists but points to a different file
Ok(md) => assert_ne!(md_file_id(&md), fd_id),
// Ok; fd does not exist
Err(_) => (),
}
}
}

#[test]
fn fd_test_stdin() {
fd_impls::test_stdin(false);
fd_impls::test_stdin(true);
}

#[test]
fn fd_test_close_time() {
fd_impls::test_close_time(false);
fd_impls::test_close_time(true);
}

#[track_caller]
fn assert_spawn_method(cmd: &Command, use_exec: bool) {
let used_posix_spawn = cmd.last_spawn_was_posix_spawn().unwrap();
if use_exec {
assert!(!used_posix_spawn, "posix_spawn used but exec was expected");
} else if cfg!(any(
target_os = "freebsd",
target_os = "illumos",
all(target_os = "linux", target_env = "gnu"),
all(target_os = "linux", target_env = "musl"),
target_os = "nto",
target_vendor = "apple",
target_os = "cygwin",
)) {
assert!(used_posix_spawn, "platform supports posix_spawn but it wasn't used");
} else {
assert!(!used_posix_spawn);
}
}
Loading