From 694be09b7be057aa342e6cfe61815032942e5f95 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 15 Sep 2020 23:35:08 -0400 Subject: [PATCH 1/7] Add Linux-specific pidfd process extensions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Background: Over the last year, pidfd support was added to the Linux kernel. This allows interacting with other processes. In particular, this allows waiting on a child process with a timeout in a race-free way, bypassing all of the awful signal-handler tricks that are usually required. Pidfds can be obtained for a child process (as well as any other process) via the `pidfd_open` syscall. Unfortunately, this requires several conditions to hold in order to be race-free (i.e. the pid is not reused). Per `man pidfd_open`: ``` · the disposition of SIGCHLD has not been explicitly set to SIG_IGN (see sigaction(2)); · the SA_NOCLDWAIT flag was not specified while establishing a han‐ dler for SIGCHLD or while setting the disposition of that signal to SIG_DFL (see sigaction(2)); and · the zombie process was not reaped elsewhere in the program (e.g., either by an asynchronously executed signal handler or by wait(2) or similar in another thread). If any of these conditions does not hold, then the child process (along with a PID file descriptor that refers to it) should instead be created using clone(2) with the CLONE_PIDFD flag. ``` Sadly, these conditions are impossible to guarantee once any libraries are used. For example, C code runnng in a different thread could call `wait()`, which is impossible to detect from Rust code trying to open a pidfd. While pid reuse issues should (hopefully) be rare in practice, we can do better. By passing the `CLONE_PIDFD` flag to `clone()` or `clone3()`, we can obtain a pidfd for the child process in a guaranteed race-free manner. This PR: This PR adds Linux-specific process extension methods to allow obtaining pidfds for processes spawned via the standard `Command` API. Other than being made available to user code, the standard library does not make use of these pidfds in any way. In particular, the implementation of `Child::wait` is completely unchanged. Two Linux-specific helper methods are added: `CommandExt::create_pidfd` and `ChildExt::pidfd`. These methods are intended to serve as a building block for libraries to build higher-level abstractions - in particular, waiting on a process with a timeout. I've included a basic test, which verifies that pidfds are created iff the `create_pidfd` method is used. This test is somewhat special - it should always succeed on systems with the `clone3` system call available, and always fail on systems without `clone3` available. I'm not sure how to best ensure this programatically. This PR relies on the newer `clone3` system call to pass the `CLONE_FD`, rather than the older `clone` system call. `clone3` was added to Linux in the same release as pidfds, so this shouldn't unnecessarily limit the kernel versions that this code supports. Unresolved questions: * What should the name of the feature gate be for these newly added methods? * Should the `pidfd` method distinguish between an error occurring and `create_pidfd` not being called? --- library/std/src/os/linux/mod.rs | 1 + library/std/src/os/linux/process.rs | 47 ++++++++ library/std/src/process.rs | 2 +- .../src/sys/unix/process/process_common.rs | 6 + .../std/src/sys/unix/process/process_unix.rs | 110 +++++++++++++++++- src/test/ui/command/command-create-pidfd.rs | 27 +++++ 6 files changed, 186 insertions(+), 7 deletions(-) create mode 100644 library/std/src/os/linux/process.rs create mode 100644 src/test/ui/command/command-create-pidfd.rs diff --git a/library/std/src/os/linux/mod.rs b/library/std/src/os/linux/mod.rs index 94438defc2270..8e7776f6646b5 100644 --- a/library/std/src/os/linux/mod.rs +++ b/library/std/src/os/linux/mod.rs @@ -4,4 +4,5 @@ #![doc(cfg(target_os = "linux"))] pub mod fs; +pub mod process; pub mod raw; diff --git a/library/std/src/os/linux/process.rs b/library/std/src/os/linux/process.rs new file mode 100644 index 0000000000000..661d3cef7a03a --- /dev/null +++ b/library/std/src/os/linux/process.rs @@ -0,0 +1,47 @@ +//! Linux-specific extensions to primitives in the `std::process` module. + +#![unstable(feature = "linux_pidfd", issue = "none")] + +use crate::process; +use crate::sys_common::AsInnerMut; +use crate::io::Result; + +/// Os-specific extensions to [`process::Child`] +/// +/// [`process::Child`]: crate::process::Child +pub trait ChildExt { + /// Obtains the pidfd created for this child process, if available. + /// + /// A pidfd will only ever be available if `create_pidfd(true)` was called + /// when the corresponding `Command` was created. + /// + /// Even if `create_pidfd(true)` is called, a pidfd may not be available + /// due to an older version of Linux being in use, or if + /// some other error occured. + /// + /// See `man pidfd_open` for more details about pidfds. + fn pidfd(&self) -> Result; +} + +/// Os-specific extensions to [`process::Command`] +/// +/// [`process::Command`]: crate::process::Command +pub trait CommandExt { + /// Sets whether or this `Command` will attempt to create a pidfd + /// for the child. If this method is never called, a pidfd will + /// not be crated. + /// + /// The pidfd can be retrieved from the child via [`ChildExt::pidfd`] + /// + /// A pidfd will only be created if it is possible to do so + /// in a guaranteed race-free manner (e.g. if the `clone3` system call is + /// supported). Otherwise, [`ChildExit::pidfd`] will return an error. + fn create_pidfd(&mut self, val: bool) -> &mut process::Command; +} + +impl CommandExt for process::Command { + fn create_pidfd(&mut self, val: bool) -> &mut process::Command { + self.as_inner_mut().create_pidfd(val); + self + } +} diff --git a/library/std/src/process.rs b/library/std/src/process.rs index 11a0432ce27a1..99c3369425b06 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -166,7 +166,7 @@ use crate::sys_common::{AsInner, AsInnerMut, FromInner, IntoInner}; /// [`wait`]: Child::wait #[stable(feature = "process", since = "1.0.0")] pub struct Child { - handle: imp::Process, + pub(crate) handle: imp::Process, /// The handle for writing to the child's standard input (stdin), if it has /// been captured. To avoid partially moving diff --git a/library/std/src/sys/unix/process/process_common.rs b/library/std/src/sys/unix/process/process_common.rs index c5bdd1bda4a7a..f7a7a9968b8fd 100644 --- a/library/std/src/sys/unix/process/process_common.rs +++ b/library/std/src/sys/unix/process/process_common.rs @@ -79,6 +79,7 @@ pub struct Command { stdin: Option, stdout: Option, stderr: Option, + pub(crate) make_pidfd: bool, } // Create a new type for argv, so that we can make it `Send` and `Sync` @@ -141,6 +142,7 @@ impl Command { stdin: None, stdout: None, stderr: None, + make_pidfd: false, } } @@ -176,6 +178,10 @@ impl Command { pub fn groups(&mut self, groups: &[gid_t]) { self.groups = Some(Box::from(groups)); } + + pub fn create_pidfd(&mut self, val: bool) { + self.make_pidfd = val; + } pub fn saw_nul(&self) -> bool { self.saw_nul diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index c888dd0d87d8e..c1605faceb6ad 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -8,6 +8,10 @@ use crate::ptr; use crate::sys; use crate::sys::cvt; use crate::sys::process::process_common::*; +use crate::sync::atomic::{AtomicBool, Ordering}; + +#[cfg(target_os = "linux")] +use crate::sys::weak::syscall; #[cfg(any( target_os = "macos", @@ -61,7 +65,8 @@ impl Command { // a lock any more because the parent won't do anything and the child is // in its own process. Thus the parent drops the lock guard while the child // forgets it to avoid unlocking it on a new thread, which would be invalid. - let (env_lock, pid) = unsafe { (sys::os::env_read_lock(), cvt(libc::fork())?) }; + let env_lock = sys::os::env_read_lock(); + let (pid, pidfd) = self.do_fork()?; if pid == 0 { crate::panic::always_abort(); @@ -90,7 +95,7 @@ impl Command { drop(env_lock); drop(output); - let mut p = Process { pid, status: None }; + let mut p = Process { pid, status: None, pidfd }; let mut bytes = [0; 8]; // loop to handle EINTR @@ -122,6 +127,85 @@ impl Command { } } + // Attempts to fork the process. If successful, returns + // Ok((0, -1)) in the child, and Ok((child_pid, child_pidfd)) in the parent. + fn do_fork(&mut self) -> Result<(libc::c_long, libc::pid_t), io::Error> { + // If we fail to create a pidfd for any reason, this will + // stay as -1, which indicates an error + let mut pidfd: libc::pid_t = -1; + + // On Linux, attempt to use the `clone3` syscall, which + // supports more argument (in prarticular, the ability to create a pidfd). + // If this fails, we will fall through this block to a call to `fork()` + cfg_if::cfg_if! { + if #[cfg(target_os = "linux")] { + static HAS_CLONE3: AtomicBool = AtomicBool::new(true); + + const CLONE_PIDFD: u64 = 0x00001000; + + #[repr(C)] + struct clone_args { + flags: u64, + pidfd: u64, + child_tid: u64, + parent_tid: u64, + exit_signal: u64, + stack: u64, + stack_size: u64, + tls: u64, + set_tid: u64, + set_tid_size: u64, + cgroup: u64, + } + + syscall! { + fn clone3(cl_args: *mut clone_args, len: libc::size_t) -> libc::c_long + } + + if HAS_CLONE3.load(Ordering::Relaxed) { + let mut flags = 0; + if self.make_pidfd { + flags |= CLONE_PIDFD; + } + + let mut args = clone_args { + flags, + pidfd: &mut pidfd as *mut libc::pid_t as u64, + child_tid: 0, + parent_tid: 0, + exit_signal: libc::SIGCHLD as u64, + stack: 0, + stack_size: 0, + tls: 0, + set_tid: 0, + set_tid_size: 0, + cgroup: 0 + }; + + let args_ptr = &mut args as *mut clone_args; + let args_size = crate::mem::size_of::(); + + let res = cvt(unsafe { clone3(args_ptr, args_size) }); + match res { + Ok(n) => return Ok((n, pidfd)), + Err(e) => match e.raw_os_error() { + // Multiple threads can race to execute this store, + // but that's fine - that just means that multiple threads + // will have tried and failed to execute the same syscall, + // with no other side effects. + Some(libc::ENOSYS) => HAS_CLONE3.store(false, Ordering::Relaxed), + _ => return Err(e) + } + } + } + } + } + // If we get here, we are either not on Linux, + // or we are on Linux and the 'clone3' syscall does not exist + cvt(unsafe { libc::fork() }.into()).map(|res| (res, pidfd)) + } + + pub fn exec(&mut self, default: Stdio) -> io::Error { let envp = self.capture_env(); @@ -276,8 +360,6 @@ impl Command { #[cfg(not(any( target_os = "macos", target_os = "freebsd", - all(target_os = "linux", target_env = "gnu"), - all(target_os = "linux", target_env = "musl"), )))] fn posix_spawn( &mut self, @@ -292,8 +374,6 @@ impl Command { #[cfg(any( target_os = "macos", target_os = "freebsd", - all(target_os = "linux", target_env = "gnu"), - all(target_os = "linux", target_env = "musl"), ))] fn posix_spawn( &mut self, @@ -441,6 +521,12 @@ impl Command { pub struct Process { pid: pid_t, status: Option, + // On Linux, stores the pidfd created for this child. + // This is -1 if the user did not request pidfd creation, + // or if the pidfd could not be created for some reason + // (e.g. the `clone3` syscall was not available). + #[cfg(target_os = "linux")] + pidfd: libc::c_int, } impl Process { @@ -580,6 +666,18 @@ impl ExitStatusError { } } +#[cfg(target_os = "linux")] +#[unstable(feature = "linux_pidfd", issue = "none")] +impl crate::os::linux::process::ChildExt for crate::process::Child { + fn pidfd(&self) -> crate::io::Result { + if self.handle.pidfd > 0 { + Ok(self.handle.pidfd) + } else { + Err(crate::io::Error::from(crate::io::ErrorKind::Other)) + } + } +} + #[cfg(test)] #[path = "process_unix/tests.rs"] mod tests; diff --git a/src/test/ui/command/command-create-pidfd.rs b/src/test/ui/command/command-create-pidfd.rs new file mode 100644 index 0000000000000..248ae3457d715 --- /dev/null +++ b/src/test/ui/command/command-create-pidfd.rs @@ -0,0 +1,27 @@ +// run-pass +// linux-only - pidfds are a linux-specific concept + +#![feature(linux_pidfd)] +use std::os::linux::process::{CommandExt, ChildExt}; +use std::process::Command; + +fn main() { + // We don't assert the precise value, since the standard libarary + // may be opened other file descriptors before our code ran. + let _ = Command::new("echo") + .create_pidfd(true) + .spawn() + .unwrap() + .pidfd().expect("failed to obtain pidfd"); + + let _ = Command::new("echo") + .create_pidfd(false) + .spawn() + .unwrap() + .pidfd().expect_err("pidfd should not have been created when create_pid(false) is set"); + + let _ = Command::new("echo") + .spawn() + .unwrap() + .pidfd().expect_err("pidfd should not have been created"); +} From ef03de2e6a4c28543941a228e0c42bcf2dc61df6 Mon Sep 17 00:00:00 2001 From: Josh Triplett Date: Sat, 17 Oct 2020 20:10:58 -0700 Subject: [PATCH 2/7] Typo fix Co-authored-by: bjorn3 --- library/std/src/sys/unix/process/process_unix.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index c1605faceb6ad..674c694e4fc73 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -135,7 +135,7 @@ impl Command { let mut pidfd: libc::pid_t = -1; // On Linux, attempt to use the `clone3` syscall, which - // supports more argument (in prarticular, the ability to create a pidfd). + // supports more argument (in particular, the ability to create a pidfd). // If this fails, we will fall through this block to a call to `fork()` cfg_if::cfg_if! { if #[cfg(target_os = "linux")] { From 619fd96868b2cc83b7f205ff11ff897aebb5ef93 Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Sat, 6 Feb 2021 14:15:49 +0100 Subject: [PATCH 3/7] Add PidFd type and seal traits Improve docs Split do_fork into two Make do_fork unsafe Add target attribute to create_pidfd field in Command Add method to get create_pidfd value --- library/std/src/os/linux/process.rs | 146 +++++++++++-- .../src/sys/unix/process/process_common.rs | 43 +++- .../std/src/sys/unix/process/process_unix.rs | 200 ++++++++++-------- src/test/ui/command/command-create-pidfd.rs | 6 +- 4 files changed, 282 insertions(+), 113 deletions(-) diff --git a/library/std/src/os/linux/process.rs b/library/std/src/os/linux/process.rs index 661d3cef7a03a..435c0227c7eca 100644 --- a/library/std/src/os/linux/process.rs +++ b/library/std/src/os/linux/process.rs @@ -2,40 +2,142 @@ #![unstable(feature = "linux_pidfd", issue = "none")] -use crate::process; -use crate::sys_common::AsInnerMut; use crate::io::Result; +use crate::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd}; +use crate::process; +use crate::sys::fd::FileDesc; +use crate::sys_common::{AsInner, AsInnerMut, FromInner, IntoInner}; -/// Os-specific extensions to [`process::Child`] +/// This type represents a file descriptor that refers to a process. +/// +/// A `PidFd` can be obtained by setting the corresponding option on [`Command`] +/// with [`create_pidfd`]. Subsequently, the created pidfd can be retrieved +/// from the [`Child`] by calling [`pidfd`] or [`take_pidfd`]. +/// +/// Example: +/// ```no_run +/// #![feature(linux_pidfd)] +/// use std::os::linux::process::{CommandExt, ChildExt}; +/// use std::process::Command; +/// +/// let mut child = Command::new("echo") +/// .create_pidfd(true) +/// .spawn() +/// .expect("Failed to spawn child"); /// -/// [`process::Child`]: crate::process::Child -pub trait ChildExt { - /// Obtains the pidfd created for this child process, if available. +/// let pidfd = child +/// .take_pidfd() +/// .expect("Failed to retrieve pidfd"); +/// +/// // The file descriptor will be closed when `pidfd` is dropped. +/// ``` +/// Refer to the man page of `pidfd_open(2)` for further details. +/// +/// [`Command`]: process::Command +/// [`create_pidfd`]: CommandExt::create_pidfd +/// [`Child`]: process::Child +/// [`pidfd`]: fn@ChildExt::pidfd +/// [`take_pidfd`]: ChildExt::take_pidfd +#[derive(Debug)] +pub struct PidFd { + inner: FileDesc, +} + +impl AsInner for PidFd { + fn as_inner(&self) -> &FileDesc { + &self.inner + } +} + +impl FromInner for PidFd { + fn from_inner(inner: FileDesc) -> PidFd { + PidFd { inner } + } +} + +impl IntoInner for PidFd { + fn into_inner(self) -> FileDesc { + self.inner + } +} + +impl AsRawFd for PidFd { + fn as_raw_fd(&self) -> RawFd { + self.as_inner().raw() + } +} + +impl FromRawFd for PidFd { + unsafe fn from_raw_fd(fd: RawFd) -> Self { + Self::from_inner(FileDesc::new(fd)) + } +} + +impl IntoRawFd for PidFd { + fn into_raw_fd(self) -> RawFd { + self.into_inner().into_raw() + } +} + +mod private_child_ext { + pub trait Sealed {} + impl Sealed for crate::process::Child {} +} + +/// Os-specific extensions for [`Child`] +/// +/// [`Child`]: process::Child +pub trait ChildExt: private_child_ext::Sealed { + /// Obtains a reference to the [`PidFd`] created for this [`Child`], if available. + /// + /// A pidfd will only be available if its creation was requested with + /// [`create_pidfd`] when the corresponding [`Command`] was created. /// - /// A pidfd will only ever be available if `create_pidfd(true)` was called - /// when the corresponding `Command` was created. + /// Even if requested, a pidfd may not be available due to an older + /// version of Linux being in use, or if some other error occurred. + /// + /// [`Command`]: process::Command + /// [`create_pidfd`]: CommandExt::create_pidfd + /// [`Child`]: process::Child + fn pidfd(&self) -> Result<&PidFd>; + + /// Takes ownership of the [`PidFd`] created for this [`Child`], if available. /// - /// Even if `create_pidfd(true)` is called, a pidfd may not be available - /// due to an older version of Linux being in use, or if - /// some other error occured. + /// A pidfd will only be available if its creation was requested with + /// [`create_pidfd`] when the corresponding [`Command`] was created. /// - /// See `man pidfd_open` for more details about pidfds. - fn pidfd(&self) -> Result; + /// Even if requested, a pidfd may not be available due to an older + /// version of Linux being in use, or if some other error occurred. + /// + /// [`Command`]: process::Command + /// [`create_pidfd`]: CommandExt::create_pidfd + /// [`Child`]: process::Child + fn take_pidfd(&mut self) -> Result; +} + +mod private_command_ext { + pub trait Sealed {} + impl Sealed for crate::process::Command {} } -/// Os-specific extensions to [`process::Command`] +/// Os-specific extensions for [`Command`] /// -/// [`process::Command`]: crate::process::Command -pub trait CommandExt { - /// Sets whether or this `Command` will attempt to create a pidfd - /// for the child. If this method is never called, a pidfd will - /// not be crated. +/// [`Command`]: process::Command +pub trait CommandExt: private_command_ext::Sealed { + /// Sets whether a [`PidFd`](struct@PidFd) should be created for the [`Child`] + /// spawned by this [`Command`]. + /// By default, no pidfd will be created. /// - /// The pidfd can be retrieved from the child via [`ChildExt::pidfd`] + /// The pidfd can be retrieved from the child with [`pidfd`] or [`take_pidfd`]. /// /// A pidfd will only be created if it is possible to do so - /// in a guaranteed race-free manner (e.g. if the `clone3` system call is - /// supported). Otherwise, [`ChildExit::pidfd`] will return an error. + /// in a guaranteed race-free manner (e.g. if the `clone3` system call + /// is supported). Otherwise, [`pidfd`] will return an error. + /// + /// [`Command`]: process::Command + /// [`Child`]: process::Child + /// [`pidfd`]: fn@ChildExt::pidfd + /// [`take_pidfd`]: ChildExt::take_pidfd fn create_pidfd(&mut self, val: bool) -> &mut process::Command; } diff --git a/library/std/src/sys/unix/process/process_common.rs b/library/std/src/sys/unix/process/process_common.rs index f7a7a9968b8fd..80cae5a3d7980 100644 --- a/library/std/src/sys/unix/process/process_common.rs +++ b/library/std/src/sys/unix/process/process_common.rs @@ -79,7 +79,8 @@ pub struct Command { stdin: Option, stdout: Option, stderr: Option, - pub(crate) make_pidfd: bool, + #[cfg(target_os = "linux")] + create_pidfd: bool, } // Create a new type for argv, so that we can make it `Send` and `Sync` @@ -125,6 +126,7 @@ pub enum Stdio { } impl Command { + #[cfg(not(target_os = "linux"))] pub fn new(program: &OsStr) -> Command { let mut saw_nul = false; let program = os2c(program, &mut saw_nul); @@ -142,7 +144,28 @@ impl Command { stdin: None, stdout: None, stderr: None, - make_pidfd: false, + } + } + + #[cfg(target_os = "linux")] + pub fn new(program: &OsStr) -> Command { + let mut saw_nul = false; + let program = os2c(program, &mut saw_nul); + Command { + argv: Argv(vec![program.as_ptr(), ptr::null()]), + args: vec![program.clone()], + program, + env: Default::default(), + cwd: None, + uid: None, + gid: None, + saw_nul, + closures: Vec::new(), + groups: None, + stdin: None, + stdout: None, + stderr: None, + create_pidfd: false, } } @@ -178,9 +201,21 @@ impl Command { pub fn groups(&mut self, groups: &[gid_t]) { self.groups = Some(Box::from(groups)); } - + + #[cfg(target_os = "linux")] pub fn create_pidfd(&mut self, val: bool) { - self.make_pidfd = val; + self.create_pidfd = val; + } + + #[cfg(not(target_os = "linux"))] + #[allow(dead_code)] + pub fn get_create_pidfd(&self) -> bool { + false + } + + #[cfg(target_os = "linux")] + pub fn get_create_pidfd(&self) -> bool { + self.create_pidfd } pub fn saw_nul(&self) -> bool { diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index 674c694e4fc73..f7112a73efac3 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -8,7 +8,9 @@ use crate::ptr; use crate::sys; use crate::sys::cvt; use crate::sys::process::process_common::*; -use crate::sync::atomic::{AtomicBool, Ordering}; + +#[cfg(target_os = "linux")] +use crate::os::linux::process::PidFd; #[cfg(target_os = "linux")] use crate::sys::weak::syscall; @@ -66,7 +68,7 @@ impl Command { // in its own process. Thus the parent drops the lock guard while the child // forgets it to avoid unlocking it on a new thread, which would be invalid. let env_lock = sys::os::env_read_lock(); - let (pid, pidfd) = self.do_fork()?; + let (pid, pidfd) = unsafe { self.do_fork()? }; if pid == 0 { crate::panic::always_abort(); @@ -95,7 +97,7 @@ impl Command { drop(env_lock); drop(output); - let mut p = Process { pid, status: None, pidfd }; + let mut p = Process::new(pid, pidfd); let mut bytes = [0; 8]; // loop to handle EINTR @@ -127,84 +129,91 @@ impl Command { } } - // Attempts to fork the process. If successful, returns - // Ok((0, -1)) in the child, and Ok((child_pid, child_pidfd)) in the parent. - fn do_fork(&mut self) -> Result<(libc::c_long, libc::pid_t), io::Error> { - // If we fail to create a pidfd for any reason, this will - // stay as -1, which indicates an error - let mut pidfd: libc::pid_t = -1; - - // On Linux, attempt to use the `clone3` syscall, which - // supports more argument (in particular, the ability to create a pidfd). - // If this fails, we will fall through this block to a call to `fork()` - cfg_if::cfg_if! { - if #[cfg(target_os = "linux")] { - static HAS_CLONE3: AtomicBool = AtomicBool::new(true); - - const CLONE_PIDFD: u64 = 0x00001000; - - #[repr(C)] - struct clone_args { - flags: u64, - pidfd: u64, - child_tid: u64, - parent_tid: u64, - exit_signal: u64, - stack: u64, - stack_size: u64, - tls: u64, - set_tid: u64, - set_tid_size: u64, - cgroup: u64, - } + // Attempts to fork the process. If successful, returns Ok((0, -1)) + // in the child, and Ok((child_pid, -1)) in the parent. + #[cfg(not(target_os = "linux"))] + unsafe fn do_fork(&mut self) -> Result<(pid_t, pid_t), io::Error> { + cvt(libc::fork()).map(|res| (res, -1)) + } - syscall! { - fn clone3(cl_args: *mut clone_args, len: libc::size_t) -> libc::c_long - } + // Attempts to fork the process. If successful, returns Ok((0, -1)) + // in the child, and Ok((child_pid, child_pidfd)) in the parent. + #[cfg(target_os = "linux")] + unsafe fn do_fork(&mut self) -> Result<(pid_t, pid_t), io::Error> { + use crate::sync::atomic::{AtomicBool, Ordering}; + + static HAS_CLONE3: AtomicBool = AtomicBool::new(true); + const CLONE_PIDFD: u64 = 0x00001000; + + #[repr(C)] + struct clone_args { + flags: u64, + pidfd: u64, + child_tid: u64, + parent_tid: u64, + exit_signal: u64, + stack: u64, + stack_size: u64, + tls: u64, + set_tid: u64, + set_tid_size: u64, + cgroup: u64, + } - if HAS_CLONE3.load(Ordering::Relaxed) { - let mut flags = 0; - if self.make_pidfd { - flags |= CLONE_PIDFD; - } + syscall! { + fn clone3(cl_args: *mut clone_args, len: libc::size_t) -> libc::c_long + } - let mut args = clone_args { - flags, - pidfd: &mut pidfd as *mut libc::pid_t as u64, - child_tid: 0, - parent_tid: 0, - exit_signal: libc::SIGCHLD as u64, - stack: 0, - stack_size: 0, - tls: 0, - set_tid: 0, - set_tid_size: 0, - cgroup: 0 - }; - - let args_ptr = &mut args as *mut clone_args; - let args_size = crate::mem::size_of::(); - - let res = cvt(unsafe { clone3(args_ptr, args_size) }); - match res { - Ok(n) => return Ok((n, pidfd)), - Err(e) => match e.raw_os_error() { - // Multiple threads can race to execute this store, - // but that's fine - that just means that multiple threads - // will have tried and failed to execute the same syscall, - // with no other side effects. - Some(libc::ENOSYS) => HAS_CLONE3.store(false, Ordering::Relaxed), - _ => return Err(e) - } - } - } + // If we fail to create a pidfd for any reason, this will + // stay as -1, which indicates an error. + let mut pidfd: pid_t = -1; + + // Attempt to use the `clone3` syscall, which supports more arguments + // (in particular, the ability to create a pidfd). If this fails, + // we will fall through this block to a call to `fork()` + if HAS_CLONE3.load(Ordering::Relaxed) { + let mut flags = 0; + if self.get_create_pidfd() { + flags |= CLONE_PIDFD; + } + + let mut args = clone_args { + flags, + pidfd: &mut pidfd as *mut pid_t as u64, + child_tid: 0, + parent_tid: 0, + exit_signal: libc::SIGCHLD as u64, + stack: 0, + stack_size: 0, + tls: 0, + set_tid: 0, + set_tid_size: 0, + cgroup: 0, + }; + + let args_ptr = &mut args as *mut clone_args; + let args_size = crate::mem::size_of::(); + + let res = cvt(clone3(args_ptr, args_size)); + match res { + Ok(n) => return Ok((n as pid_t, pidfd)), + Err(e) => match e.raw_os_error() { + // Multiple threads can race to execute this store, + // but that's fine - that just means that multiple threads + // will have tried and failed to execute the same syscall, + // with no other side effects. + Some(libc::ENOSYS) => HAS_CLONE3.store(false, Ordering::Relaxed), + // Fallback to fork if `EPERM` is returned. (e.g. blocked by seccomp) + Some(libc::EPERM) => {} + _ => return Err(e), + }, } } - // If we get here, we are either not on Linux, - // or we are on Linux and the 'clone3' syscall does not exist - cvt(unsafe { libc::fork() }.into()).map(|res| (res, pidfd)) - } + // If we get here, the 'clone3' syscall does not exist + // or we do not have permission to call it + cvt(libc::fork()).map(|res| (res, pidfd)) + } pub fn exec(&mut self, default: Stdio) -> io::Error { let envp = self.capture_env(); @@ -360,6 +369,8 @@ impl Command { #[cfg(not(any( target_os = "macos", target_os = "freebsd", + all(target_os = "linux", target_env = "gnu"), + all(target_os = "linux", target_env = "musl"), )))] fn posix_spawn( &mut self, @@ -374,6 +385,8 @@ impl Command { #[cfg(any( target_os = "macos", target_os = "freebsd", + all(target_os = "linux", target_env = "gnu"), + all(target_os = "linux", target_env = "musl"), ))] fn posix_spawn( &mut self, @@ -388,6 +401,7 @@ impl Command { || (self.env_saw_path() && !self.program_is_path()) || !self.get_closures().is_empty() || self.get_groups().is_some() + || self.get_create_pidfd() { return Ok(None); } @@ -432,7 +446,7 @@ impl Command { None => None, }; - let mut p = Process { pid: 0, status: None }; + let mut p = Process::new(0, -1); struct PosixSpawnFileActions<'a>(&'a mut MaybeUninit); @@ -522,14 +536,26 @@ pub struct Process { pid: pid_t, status: Option, // On Linux, stores the pidfd created for this child. - // This is -1 if the user did not request pidfd creation, + // This is None if the user did not request pidfd creation, // or if the pidfd could not be created for some reason // (e.g. the `clone3` syscall was not available). #[cfg(target_os = "linux")] - pidfd: libc::c_int, + pidfd: Option, } impl Process { + #[cfg(target_os = "linux")] + fn new(pid: pid_t, pidfd: pid_t) -> Self { + use crate::sys_common::FromInner; + let pidfd = (pidfd >= 0).then(|| PidFd::from_inner(sys::fd::FileDesc::new(pidfd))); + Process { pid, status: None, pidfd } + } + + #[cfg(not(target_os = "linux"))] + fn new(pid: pid_t, _pidfd: pid_t) -> Self { + Process { pid, status: None } + } + pub fn id(&self) -> u32 { self.pid as u32 } @@ -669,12 +695,18 @@ impl ExitStatusError { #[cfg(target_os = "linux")] #[unstable(feature = "linux_pidfd", issue = "none")] impl crate::os::linux::process::ChildExt for crate::process::Child { - fn pidfd(&self) -> crate::io::Result { - if self.handle.pidfd > 0 { - Ok(self.handle.pidfd) - } else { - Err(crate::io::Error::from(crate::io::ErrorKind::Other)) - } + fn pidfd(&self) -> io::Result<&PidFd> { + self.handle + .pidfd + .as_ref() + .ok_or_else(|| Error::new(ErrorKind::Other, "No pidfd was created.")) + } + + fn take_pidfd(&mut self) -> io::Result { + self.handle + .pidfd + .take() + .ok_or_else(|| Error::new(ErrorKind::Other, "No pidfd was created.")) } } diff --git a/src/test/ui/command/command-create-pidfd.rs b/src/test/ui/command/command-create-pidfd.rs index 248ae3457d715..db728be09dfbc 100644 --- a/src/test/ui/command/command-create-pidfd.rs +++ b/src/test/ui/command/command-create-pidfd.rs @@ -1,13 +1,13 @@ // run-pass -// linux-only - pidfds are a linux-specific concept +// only-linux - pidfds are a linux-specific concept #![feature(linux_pidfd)] use std::os::linux::process::{CommandExt, ChildExt}; use std::process::Command; fn main() { - // We don't assert the precise value, since the standard libarary - // may be opened other file descriptors before our code ran. + // We don't assert the precise value, since the standard library + // might have opened other file descriptors before our code runs. let _ = Command::new("echo") .create_pidfd(true) .spawn() From c3321d3eb3bdf8655993c4564ad6569c4c849b59 Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Wed, 10 Mar 2021 11:21:01 +0100 Subject: [PATCH 4/7] Add tracking issue and link to man-page --- library/std/src/os/linux/process.rs | 5 +++-- library/std/src/sys/unix/process/process_unix.rs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/library/std/src/os/linux/process.rs b/library/std/src/os/linux/process.rs index 435c0227c7eca..17b9ae82bce4d 100644 --- a/library/std/src/os/linux/process.rs +++ b/library/std/src/os/linux/process.rs @@ -1,6 +1,6 @@ //! Linux-specific extensions to primitives in the `std::process` module. -#![unstable(feature = "linux_pidfd", issue = "none")] +#![unstable(feature = "linux_pidfd", issue = "82971")] use crate::io::Result; use crate::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd}; @@ -31,13 +31,14 @@ use crate::sys_common::{AsInner, AsInnerMut, FromInner, IntoInner}; /// /// // The file descriptor will be closed when `pidfd` is dropped. /// ``` -/// Refer to the man page of `pidfd_open(2)` for further details. +/// Refer to the man page of [`pidfd_open(2)`] for further details. /// /// [`Command`]: process::Command /// [`create_pidfd`]: CommandExt::create_pidfd /// [`Child`]: process::Child /// [`pidfd`]: fn@ChildExt::pidfd /// [`take_pidfd`]: ChildExt::take_pidfd +/// [`pidfd_open(2)`]: https://man7.org/linux/man-pages/man2/pidfd_open.2.html #[derive(Debug)] pub struct PidFd { inner: FileDesc, diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index f7112a73efac3..4b210d6af1303 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -693,7 +693,7 @@ impl ExitStatusError { } #[cfg(target_os = "linux")] -#[unstable(feature = "linux_pidfd", issue = "none")] +#[unstable(feature = "linux_pidfd", issue = "82971")] impl crate::os::linux::process::ChildExt for crate::process::Child { fn pidfd(&self) -> io::Result<&PidFd> { self.handle From 12fbabd27f700a59d0e7031f0839b220c3514bcb Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Thu, 25 Mar 2021 22:46:37 +0100 Subject: [PATCH 5/7] Do not call getpid wrapper after fork in tests The test calls libc::getpid() in the pre_exec hook and asserts that the returned value is different from the PID of the parent. However, libc::getpid() returns the wrong value. Before version 2.25, glibc caches the PID of the current process with the goal of avoiding additional syscalls. The cached value is only updated when the wrapper functions for fork or clone are called. In PR #81825 we switch to directly using the clone3 syscall. Thus, the cache is not updated and getpid returns the PID of the parent. source: https://man7.org/linux/man-pages/man2/getpid.2.html#NOTES --- src/test/ui/command/command-pre-exec.rs | 25 ++++++++++++++----- .../ui/process/process-panic-after-fork.rs | 17 ++++++++++++- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/test/ui/command/command-pre-exec.rs b/src/test/ui/command/command-pre-exec.rs index 61914e2293070..10a8b19159e02 100644 --- a/src/test/ui/command/command-pre-exec.rs +++ b/src/test/ui/command/command-pre-exec.rs @@ -8,8 +8,6 @@ // ignore-sgx no processes #![feature(process_exec, rustc_private)] -extern crate libc; - use std::env; use std::io::Error; use std::os::unix::process::CommandExt; @@ -17,6 +15,23 @@ use std::process::Command; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; +#[cfg(not(target_os = "linux"))] +fn getpid() -> u32 { + use std::process; + process::id() +} + +/// We need to directly use the getpid syscall instead of using `process::id()` +/// because the libc wrapper might return incorrect values after a process was +/// forked. +#[cfg(target_os = "linux")] +fn getpid() -> u32 { + extern crate libc; + unsafe { + libc::syscall(libc::SYS_getpid) as _ + } +} + fn main() { if let Some(arg) = env::args().nth(1) { match &arg[..] { @@ -68,14 +83,12 @@ fn main() { }; assert_eq!(output.raw_os_error(), Some(102)); - let pid = unsafe { libc::getpid() }; - assert!(pid >= 0); + let pid = getpid(); let output = unsafe { Command::new(&me) .arg("empty") .pre_exec(move || { - let child = libc::getpid(); - assert!(child >= 0); + let child = getpid(); assert!(pid != child); Ok(()) }) diff --git a/src/test/ui/process/process-panic-after-fork.rs b/src/test/ui/process/process-panic-after-fork.rs index 1ccf6bb051c20..ad749371beac0 100644 --- a/src/test/ui/process/process-panic-after-fork.rs +++ b/src/test/ui/process/process-panic-after-fork.rs @@ -23,6 +23,21 @@ use std::sync::atomic::{AtomicU32, Ordering}; use libc::c_int; +#[cfg(not(target_os = "linux"))] +fn getpid() -> u32 { + process::id() +} + +/// We need to directly use the getpid syscall instead of using `process::id()` +/// because the libc wrapper might return incorrect values after a process was +/// forked. +#[cfg(target_os = "linux")] +fn getpid() -> u32 { + unsafe { + libc::syscall(libc::SYS_getpid) as _ + } +} + /// This stunt allocator allows us to spot heap allocations in the child. struct PidChecking { parent: A, @@ -44,7 +59,7 @@ impl PidChecking { fn check(&self) { let require_pid = self.require_pid.load(Ordering::Acquire); if require_pid != 0 { - let actual_pid = process::id(); + let actual_pid = getpid(); if require_pid != actual_pid { unsafe { libc::raise(libc::SIGUSR1); From 2a4d012103999528ea7712bd3bb93a5ed7a6824e Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Thu, 1 Jul 2021 16:50:14 +0200 Subject: [PATCH 6/7] Add dummy FileDesc struct for doc target --- library/std/src/os/linux/process.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/std/src/os/linux/process.rs b/library/std/src/os/linux/process.rs index 17b9ae82bce4d..91547b8f91622 100644 --- a/library/std/src/os/linux/process.rs +++ b/library/std/src/os/linux/process.rs @@ -5,9 +5,13 @@ use crate::io::Result; use crate::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd}; use crate::process; +#[cfg(not(doc))] use crate::sys::fd::FileDesc; use crate::sys_common::{AsInner, AsInnerMut, FromInner, IntoInner}; +#[cfg(doc)] +struct FileDesc; + /// This type represents a file descriptor that refers to a process. /// /// A `PidFd` can be obtained by setting the corresponding option on [`Command`] From 4a832d32f232a68acdabfd29e526d2a4b6366a1c Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Sat, 10 Jul 2021 12:58:30 +0200 Subject: [PATCH 7/7] Check whether clone3 syscall exists in pidfd test --- src/test/ui/command/command-create-pidfd.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/test/ui/command/command-create-pidfd.rs b/src/test/ui/command/command-create-pidfd.rs index db728be09dfbc..93321ac536ab9 100644 --- a/src/test/ui/command/command-create-pidfd.rs +++ b/src/test/ui/command/command-create-pidfd.rs @@ -2,10 +2,28 @@ // only-linux - pidfds are a linux-specific concept #![feature(linux_pidfd)] -use std::os::linux::process::{CommandExt, ChildExt}; +#![feature(rustc_private)] + +extern crate libc; + +use std::io::Error; +use std::os::linux::process::{ChildExt, CommandExt}; use std::process::Command; +fn has_clone3() -> bool { + let res = unsafe { libc::syscall(libc::SYS_clone3, 0, 0) }; + let err = (res == -1) + .then(|| Error::last_os_error()) + .expect("probe syscall should not succeed"); + err.raw_os_error() != Some(libc::ENOSYS) +} + fn main() { + // pidfds require the clone3 syscall + if !has_clone3() { + return; + } + // We don't assert the precise value, since the standard library // might have opened other file descriptors before our code runs. let _ = Command::new("echo")