From 5e788f2fa0532cff429b2574a61e348e0bc52e0a Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 15 Sep 2020 23:35:08 -0400 Subject: [PATCH 01/21] 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 | 109 ++++++++++++++++-- src/test/ui/command/command-create-pidfd.rs | 27 +++++ 6 files changed, 184 insertions(+), 8 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 f179a524336fc..f7bb63df0c1ff 100644 --- a/library/std/src/os/linux/mod.rs +++ b/library/std/src/os/linux/mod.rs @@ -3,4 +3,5 @@ #![stable(feature = "raw_ext", since = "1.1.0")] 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 f9cfd11e90650..940029adfcf69 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -165,7 +165,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 b9dcc4e4b9e38..38a5915e62b97 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 47aaca82af946..ad7567965cb40 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -6,6 +6,7 @@ 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 = "vxworks")] use libc::RTP_ID as pid_t; @@ -48,7 +49,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, result) = unsafe { (sys::os::env_read_lock(), cvt(libc::fork())?) }; + let env_lock = sys::os::env_read_lock(); + let (result, pidfd) = self.do_fork()?; let pid = unsafe { match result { @@ -76,12 +78,12 @@ impl Command { } n => { drop(env_lock); - n + n as pid_t } } }; - let mut p = Process { pid, status: None }; + let mut p = Process { pid, status: None, pidfd }; drop(output); let mut bytes = [0; 8]; @@ -114,6 +116,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(); @@ -265,8 +346,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, @@ -281,8 +360,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, @@ -430,6 +507,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 { @@ -546,6 +629,18 @@ impl fmt::Display for ExitStatus { } } +#[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 3426bf746e95e5e396f62dcec8433f5708124935 Mon Sep 17 00:00:00 2001 From: Josh Triplett Date: Sat, 17 Oct 2020 20:10:58 -0700 Subject: [PATCH 02/21] 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 ad7567965cb40..5af9f8769b5c4 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -124,7 +124,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 704d6c5de3bdc7127112c07f1da01a15aaef23e4 Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Sat, 6 Feb 2021 14:15:49 +0100 Subject: [PATCH 03/21] Add PidFd type and seal traits Improve docs --- library/std/src/os/linux/process.rs | 146 +++++++++++--- .../src/sys/unix/process/process_common.rs | 8 +- .../std/src/sys/unix/process/process_unix.rs | 178 ++++++++++-------- src/test/ui/command/command-create-pidfd.rs | 4 +- 4 files changed, 233 insertions(+), 103 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 38a5915e62b97..9f6e120134a95 100644 --- a/library/std/src/sys/unix/process/process_common.rs +++ b/library/std/src/sys/unix/process/process_common.rs @@ -79,7 +79,7 @@ pub struct Command { stdin: Option, stdout: Option, stderr: Option, - pub(crate) make_pidfd: bool, + pub(crate) create_pidfd: bool, } // Create a new type for argv, so that we can make it `Send` and `Sync` @@ -142,7 +142,7 @@ impl Command { stdin: None, stdout: None, stderr: None, - make_pidfd: false, + create_pidfd: false, } } @@ -178,9 +178,9 @@ 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; + self.create_pidfd = val; } 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 5af9f8769b5c4..a95c595dc39a9 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -3,16 +3,20 @@ use crate::fmt; use crate::io::{self, Error, ErrorKind}; use crate::mem; use crate::ptr; +use crate::sync::atomic::{AtomicBool, Ordering}; use crate::sys; use crate::sys::cvt; use crate::sys::process::process_common::*; -use crate::sync::atomic::{AtomicBool, Ordering}; +use crate::sys_common::FromInner; + +#[cfg(target_os = "linux")] +use crate::os::linux::process::PidFd; #[cfg(target_os = "vxworks")] use libc::RTP_ID as pid_t; #[cfg(not(target_os = "vxworks"))] -use libc::{c_int, gid_t, pid_t, uid_t}; +use libc::{c_int, c_long, gid_t, pid_t, uid_t}; //////////////////////////////////////////////////////////////////////////////// // Command @@ -78,12 +82,12 @@ impl Command { } n => { drop(env_lock); - n as pid_t + n } } }; - let mut p = Process { pid, status: None, pidfd }; + let mut p = Process::new(pid, pidfd); drop(output); let mut bytes = [0; 8]; @@ -118,83 +122,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> { + fn do_fork(&mut self) -> Result<(pid_t, 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; + let mut pidfd: pid_t = -1; // On Linux, attempt to use the `clone3` syscall, which - // supports more argument (in particular, the ability to create a pidfd). + // 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()` - 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, - } + #[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 - } + syscall! { + fn clone3(cl_args: *mut clone_args, len: libc::size_t) -> c_long + } - if HAS_CLONE3.load(Ordering::Relaxed) { - let mut flags = 0; - if self.make_pidfd { - flags |= CLONE_PIDFD; - } + if HAS_CLONE3.load(Ordering::Relaxed) { + let mut flags = 0; + if self.create_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) - } - } + 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(unsafe { 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)) + // or we do not have permission to call it + cvt(unsafe { libc::fork() }).map(|res| (res, pidfd)) } - pub fn exec(&mut self, default: Stdio) -> io::Error { let envp = self.capture_env(); @@ -346,6 +352,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, @@ -360,6 +368,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, @@ -374,6 +384,7 @@ impl Command { || (self.env_saw_path() && !self.program_is_path()) || !self.get_closures().is_empty() || self.get_groups().is_some() + || self.create_pidfd { return Ok(None); } @@ -418,7 +429,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); @@ -508,14 +519,25 @@ 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 { + 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 } @@ -632,12 +654,18 @@ impl fmt::Display for ExitStatus { #[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..2e08eb1be540a 100644 --- a/src/test/ui/command/command-create-pidfd.rs +++ b/src/test/ui/command/command-create-pidfd.rs @@ -6,8 +6,8 @@ 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 766cc259cc84e6036014944df352cbda2297f993 Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Wed, 10 Mar 2021 11:21:01 +0100 Subject: [PATCH 04/21] 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 a95c595dc39a9..c6f8c446a0bd8 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -652,7 +652,7 @@ impl fmt::Display for ExitStatus { } #[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 d850f6b3746aee0505e21288cbef209050d118d0 Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Mon, 15 Mar 2021 21:17:23 +0100 Subject: [PATCH 05/21] Update libc dependency to 0.2.89 --- Cargo.lock | 4 ++-- library/std/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 203d8acb5b470..779ab7cd401a1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1889,9 +1889,9 @@ checksum = "830d08ce1d1d941e6b30645f1a0eb5643013d835ce3779a5fc208261dbe10f55" [[package]] name = "libc" -version = "0.2.88" +version = "0.2.89" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "03b07a082330a35e43f63177cc01689da34fbffa0105e1246cf0311472cac73a" +checksum = "538c092e5586f4cdd7dd8078c4a79220e3e168880218124dcbce860f0ea938c6" dependencies = [ "rustc-std-workspace-core", ] diff --git a/library/std/Cargo.toml b/library/std/Cargo.toml index f0f5558fd1608..f84c5edef0fd0 100644 --- a/library/std/Cargo.toml +++ b/library/std/Cargo.toml @@ -16,7 +16,7 @@ cfg-if = { version = "0.1.8", features = ['rustc-dep-of-std'] } panic_unwind = { path = "../panic_unwind", optional = true } panic_abort = { path = "../panic_abort" } core = { path = "../core" } -libc = { version = "0.2.88", default-features = false, features = ['rustc-dep-of-std'] } +libc = { version = "0.2.89", default-features = false, features = ['rustc-dep-of-std'] } compiler_builtins = { version = "0.1.39" } profiler_builtins = { path = "../profiler_builtins", optional = true } unwind = { path = "../unwind" } From 5ac8a31fbdf590cba0411ba26a74c7d6740f05ac Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Tue, 16 Mar 2021 10:03:27 +0100 Subject: [PATCH 06/21] Fix test header and imports --- library/std/src/sys/unix/process/process_unix.rs | 8 ++++---- src/test/ui/command/command-create-pidfd.rs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index c6f8c446a0bd8..e5b00973e23a8 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -3,11 +3,9 @@ use crate::fmt; use crate::io::{self, Error, ErrorKind}; use crate::mem; use crate::ptr; -use crate::sync::atomic::{AtomicBool, Ordering}; use crate::sys; use crate::sys::cvt; use crate::sys::process::process_common::*; -use crate::sys_common::FromInner; #[cfg(target_os = "linux")] use crate::os::linux::process::PidFd; @@ -16,7 +14,7 @@ use crate::os::linux::process::PidFd; use libc::RTP_ID as pid_t; #[cfg(not(target_os = "vxworks"))] -use libc::{c_int, c_long, gid_t, pid_t, uid_t}; +use libc::{c_int, gid_t, pid_t, uid_t}; //////////////////////////////////////////////////////////////////////////////// // Command @@ -132,6 +130,7 @@ impl Command { // If this fails, we will fall through this block to a call to `fork()` #[cfg(target_os = "linux")] { + use crate::sync::atomic::{AtomicBool, Ordering}; static HAS_CLONE3: AtomicBool = AtomicBool::new(true); const CLONE_PIDFD: u64 = 0x00001000; @@ -152,7 +151,7 @@ impl Command { } syscall! { - fn clone3(cl_args: *mut clone_args, len: libc::size_t) -> c_long + fn clone3(cl_args: *mut clone_args, len: libc::size_t) -> libc::c_long } if HAS_CLONE3.load(Ordering::Relaxed) { @@ -529,6 +528,7 @@ pub struct Process { 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 } } diff --git a/src/test/ui/command/command-create-pidfd.rs b/src/test/ui/command/command-create-pidfd.rs index 2e08eb1be540a..db728be09dfbc 100644 --- a/src/test/ui/command/command-create-pidfd.rs +++ b/src/test/ui/command/command-create-pidfd.rs @@ -1,5 +1,5 @@ // 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}; From a266bd820143d2daa112e2cb3f3205e82108037e Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Tue, 16 Mar 2021 11:27:24 +0100 Subject: [PATCH 07/21] Split do_fork into two --- .../std/src/sys/unix/process/process_unix.rs | 142 +++++++++--------- 1 file changed, 73 insertions(+), 69 deletions(-) diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index e5b00973e23a8..24618bdcb582c 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -118,84 +118,88 @@ 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. + // 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"))] + fn do_fork(&mut self) -> Result<(pid_t, pid_t), io::Error> { + cvt(unsafe { libc::fork() }).map(|res| (res, -1)) + } + + // 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")] 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, + } + + syscall! { + fn clone3(cl_args: *mut clone_args, len: libc::size_t) -> libc::c_long + } + // If we fail to create a pidfd for any reason, this will - // stay as -1, which indicates an error + // stay as -1, which indicates an error. let mut pidfd: pid_t = -1; - // On Linux, 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()` - #[cfg(target_os = "linux")] - { - 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, - } - - syscall! { - fn clone3(cl_args: *mut clone_args, len: libc::size_t) -> libc::c_long + // 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.create_pidfd { + flags |= CLONE_PIDFD; } - if HAS_CLONE3.load(Ordering::Relaxed) { - let mut flags = 0; - if self.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(unsafe { 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), - }, - } + 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(unsafe { 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 + // If we get here, the 'clone3' syscall does not exist // or we do not have permission to call it cvt(unsafe { libc::fork() }).map(|res| (res, pidfd)) } From cfb2d726098e6c0e3c8639894313ac39ff40a811 Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Tue, 16 Mar 2021 17:30:14 +0100 Subject: [PATCH 08/21] Make do_fork unsafe --- library/std/src/sys/unix/process/process_unix.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index 24618bdcb582c..47d8e0cd771d0 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -52,7 +52,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 (result, pidfd) = self.do_fork()?; + let (result, pidfd) = unsafe { self.do_fork()? }; let pid = unsafe { match result { @@ -121,14 +121,14 @@ impl Command { // 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"))] - fn do_fork(&mut self) -> Result<(pid_t, pid_t), io::Error> { - cvt(unsafe { libc::fork() }).map(|res| (res, -1)) + unsafe fn do_fork(&mut self) -> Result<(pid_t, pid_t), io::Error> { + cvt(libc::fork()).map(|res| (res, -1)) } // 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")] - fn do_fork(&mut self) -> Result<(pid_t, pid_t), io::Error> { + 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); @@ -183,7 +183,7 @@ impl Command { 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) }); + 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() { @@ -201,7 +201,7 @@ impl Command { // If we get here, the 'clone3' syscall does not exist // or we do not have permission to call it - cvt(unsafe { libc::fork() }).map(|res| (res, pidfd)) + cvt(libc::fork()).map(|res| (res, pidfd)) } pub fn exec(&mut self, default: Stdio) -> io::Error { From 79348f49792cb4831a57ca817792ab72308d9325 Mon Sep 17 00:00:00 2001 From: Yuki Okushi Date: Thu, 18 Mar 2021 05:25:47 +0900 Subject: [PATCH 09/21] `cargo update -p parking_lot -p parking_lot_core` Changelog: https://github.com/Amanieu/parking_lot/blob/master/CHANGELOG.md#parking_lot_core-083-2021-02-12 The full log: ``` Removing cloudabi v0.1.0 Updating parking_lot v0.11.0 -> v0.11.1 Updating parking_lot_core v0.8.0 -> v0.8.3 ``` --- Cargo.lock | 22 ++++++---------------- src/tools/tidy/src/deps.rs | 2 -- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 203d8acb5b470..757cd29ac80a3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -613,15 +613,6 @@ dependencies = [ "unicode-normalization", ] -[[package]] -name = "cloudabi" -version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4344512281c643ae7638bbabc3af17a11307803ec8f0fcad9fae512a8bf36467" -dependencies = [ - "bitflags", -] - [[package]] name = "cmake" version = "0.1.44" @@ -2492,9 +2483,9 @@ dependencies = [ [[package]] name = "parking_lot" -version = "0.11.0" +version = "0.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a4893845fa2ca272e647da5d0e46660a314ead9c2fdd9a883aabc32e481a8733" +checksum = "6d7744ac029df22dca6284efe4e898991d28e3085c706c972bcd7da4a27a15eb" dependencies = [ "instant", "lock_api", @@ -2503,15 +2494,14 @@ dependencies = [ [[package]] name = "parking_lot_core" -version = "0.8.0" +version = "0.8.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c361aa727dd08437f2f1447be8b59a33b0edd15e0fcee698f935613d9efbca9b" +checksum = "fa7a782938e745763fe6907fc6ba86946d72f49fe7e21de074e08128a99fb018" dependencies = [ - "cfg-if 0.1.10", - "cloudabi", + "cfg-if 1.0.0", "instant", "libc", - "redox_syscall 0.1.57", + "redox_syscall 0.2.5", "smallvec 1.6.1", "winapi 0.3.9", ] diff --git a/src/tools/tidy/src/deps.rs b/src/tools/tidy/src/deps.rs index 06dc16f767660..5ec2fb6ca7901 100644 --- a/src/tools/tidy/src/deps.rs +++ b/src/tools/tidy/src/deps.rs @@ -32,7 +32,6 @@ const EXCEPTIONS: &[(&str, &str)] = &[ ("fuchsia-zircon", "BSD-3-Clause"), // rustdoc, rustc, cargo (jobserver & tempdir) ("colored", "MPL-2.0"), // rustfmt ("ordslice", "Apache-2.0"), // rls - ("cloudabi", "BSD-2-Clause"), // (rls -> crossbeam-channel 0.2 -> rand 0.5) ("ryu", "Apache-2.0 OR BSL-1.0"), // rls/cargo/... (because of serde) ("bytesize", "Apache-2.0"), // cargo ("im-rc", "MPL-2.0+"), // cargo @@ -76,7 +75,6 @@ const PERMITTED_DEPENDENCIES: &[&str] = &[ "cfg-if", "chalk-derive", "chalk-ir", - "cloudabi", "cmake", "compiler_builtins", "cpuid-bool", From 2b0e27e2423bfa4e878390afaafd47e72ae0c30b Mon Sep 17 00:00:00 2001 From: Yuki Okushi Date: Thu, 18 Mar 2021 05:34:05 +0900 Subject: [PATCH 10/21] `cargo update -p bytecount` Commit range: https://github.com/llogiq/bytecount/compare/b0f5fba8069b993b251a95473910853eff0b888b...8dcd43753cde591fc10dab6e54be3e2fc6e938ce The full log: ``` Updating bytecount v0.6.0 -> v0.6.2 Adding libm v0.1.4 Removing packed_simd v0.3.3 Adding packed_simd_2 v0.3.4 ``` --- Cargo.lock | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 757cd29ac80a3..4a4335ba498fd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -252,11 +252,11 @@ checksum = "e3b5ca7a04898ad4bcd41c90c5285445ff5b791899bb1b0abdd2a2aa791211d7" [[package]] name = "bytecount" -version = "0.6.0" +version = "0.6.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b0017894339f586ccb943b01b9555de56770c11cda818e7e3d8bd93f4ed7f46e" +checksum = "72feb31ffc86498dacdbd0fcebb56138e7177a8cc5cea4516031d15ae85a742e" dependencies = [ - "packed_simd", + "packed_simd_2", ] [[package]] @@ -1901,6 +1901,12 @@ dependencies = [ "pkg-config", ] +[[package]] +name = "libm" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7fc7aa29613bd6a620df431842069224d8bc9011086b1db4c0e0cd47fa03ec9a" + [[package]] name = "libnghttp2-sys" version = "0.1.4+1.41.0" @@ -2435,12 +2441,13 @@ dependencies = [ ] [[package]] -name = "packed_simd" -version = "0.3.3" +name = "packed_simd_2" +version = "0.3.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a85ea9fc0d4ac0deb6fe7911d38786b32fc11119afd9e9d38b84ff691ce64220" +checksum = "3278e0492f961fd4ae70909f56b2723a7e8d01a228427294e19cdfdebda89a17" dependencies = [ "cfg-if 0.1.10", + "libm", ] [[package]] From 4238a8f7da3c1bd8990368e07277d9da528194ca Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Fri, 19 Mar 2021 01:18:27 +0100 Subject: [PATCH 11/21] Add target attribute to create_pidfd field in Command --- .../src/sys/unix/process/process_common.rs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/library/std/src/sys/unix/process/process_common.rs b/library/std/src/sys/unix/process/process_common.rs index 9f6e120134a95..3c65b26861543 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, + #[cfg(target_os = "linux")] pub(crate) create_pidfd: bool, } @@ -125,6 +126,28 @@ 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); + 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, + } + } + + #[cfg(target_os = "linux")] pub fn new(program: &OsStr) -> Command { let mut saw_nul = false; let program = os2c(program, &mut saw_nul); @@ -179,6 +202,7 @@ impl Command { self.groups = Some(Box::from(groups)); } + #[cfg(target_os = "linux")] pub fn create_pidfd(&mut self, val: bool) { self.create_pidfd = val; } From 58c1352330171019b729e86c08cacb8d25c14bd2 Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Fri, 19 Mar 2021 02:11:38 +0100 Subject: [PATCH 12/21] Add method to get create_pidfd value --- library/std/src/sys/unix/process/process_common.rs | 13 ++++++++++++- library/std/src/sys/unix/process/process_unix.rs | 4 ++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/library/std/src/sys/unix/process/process_common.rs b/library/std/src/sys/unix/process/process_common.rs index 3c65b26861543..578fb9b2f069f 100644 --- a/library/std/src/sys/unix/process/process_common.rs +++ b/library/std/src/sys/unix/process/process_common.rs @@ -80,7 +80,7 @@ pub struct Command { stdout: Option, stderr: Option, #[cfg(target_os = "linux")] - pub(crate) create_pidfd: bool, + create_pidfd: bool, } // Create a new type for argv, so that we can make it `Send` and `Sync` @@ -207,6 +207,17 @@ impl Command { 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 { 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 47d8e0cd771d0..a7fdfea074217 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -162,7 +162,7 @@ impl Command { // we will fall through this block to a call to `fork()` if HAS_CLONE3.load(Ordering::Relaxed) { let mut flags = 0; - if self.create_pidfd { + if self.get_create_pidfd() { flags |= CLONE_PIDFD; } @@ -387,7 +387,7 @@ impl Command { || (self.env_saw_path() && !self.program_is_path()) || !self.get_closures().is_empty() || self.get_groups().is_some() - || self.create_pidfd + || self.get_create_pidfd() { return Ok(None); } From addf680cbd9167eccbf8518039e53a10b3b68dcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Sat, 20 Mar 2021 00:00:00 +0000 Subject: [PATCH 13/21] Use a single codegen unit to reduce non-determinism in srcloc.rs test When building with multiple codegen units the test case can fail with only a subset of all errors. Use a single codegen unit as a workaround. --- src/test/ui/asm/srcloc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/ui/asm/srcloc.rs b/src/test/ui/asm/srcloc.rs index 1477e3dd5665c..7262064e7bbee 100644 --- a/src/test/ui/asm/srcloc.rs +++ b/src/test/ui/asm/srcloc.rs @@ -1,7 +1,7 @@ // no-system-llvm // only-x86_64 // build-fail - +// compile-flags: -Ccodegen-units=1 #![feature(asm)] // Checks that inline asm errors are mapped to the correct line in the source code. From 3d64f8d475d701a36971cb416e49b80449988f6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Mon, 8 Mar 2021 00:00:00 +0000 Subject: [PATCH 14/21] Join test thread to make assertion effective in sym.rs test case --- src/test/ui/asm/sym.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/ui/asm/sym.rs b/src/test/ui/asm/sym.rs index 9931697e4129c..d5c03a7d567ba 100644 --- a/src/test/ui/asm/sym.rs +++ b/src/test/ui/asm/sym.rs @@ -76,5 +76,5 @@ fn main() { std::thread::spawn(|| { assert_eq!(static_addr!(S1), &S1 as *const u32); assert_eq!(static_tls_addr!(S2), &S2 as *const u32); - }); + }).join().unwrap(); } From ae8ef70a499907c929f5d7ad6539cd1187da336b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20Sinan=20A=C4=9Facan?= Date: Sun, 21 Mar 2021 14:27:22 +0300 Subject: [PATCH 15/21] Simplify and fix byte skipping in format! string parser Fixes '\\' handling in format strings. Fixes #83340 --- compiler/rustc_parse_format/src/lib.rs | 23 +++++++++-------------- src/test/ui/macros/issue-83340.rs | 8 ++++++++ src/test/ui/macros/issue-83340.stderr | 8 ++++++++ 3 files changed, 25 insertions(+), 14 deletions(-) create mode 100644 src/test/ui/macros/issue-83340.rs create mode 100644 src/test/ui/macros/issue-83340.stderr diff --git a/compiler/rustc_parse_format/src/lib.rs b/compiler/rustc_parse_format/src/lib.rs index 92d974690b514..e20fc28e794cc 100644 --- a/compiler/rustc_parse_format/src/lib.rs +++ b/compiler/rustc_parse_format/src/lib.rs @@ -735,25 +735,24 @@ fn find_skips_from_snippet( }; fn find_skips(snippet: &str, is_raw: bool) -> Vec { - let mut eat_ws = false; let mut s = snippet.char_indices().peekable(); let mut skips = vec![]; while let Some((pos, c)) = s.next() { match (c, s.peek()) { // skip whitespace and empty lines ending in '\\' ('\\', Some((next_pos, '\n'))) if !is_raw => { - eat_ws = true; skips.push(pos); skips.push(*next_pos); let _ = s.next(); - } - ('\\', Some((next_pos, '\n' | 'n' | 't'))) if eat_ws => { - skips.push(pos); - skips.push(*next_pos); - let _ = s.next(); - } - (' ' | '\n' | '\t', _) if eat_ws => { - skips.push(pos); + + while let Some((pos, c)) = s.peek() { + if matches!(c, ' ' | '\n' | '\t') { + skips.push(*pos); + let _ = s.next(); + } else { + break; + } + } } ('\\', Some((next_pos, 'n' | 't' | 'r' | '0' | '\\' | '\'' | '\"'))) => { skips.push(*next_pos); @@ -804,10 +803,6 @@ fn find_skips_from_snippet( } } } - _ if eat_ws => { - // `take_while(|c| c.is_whitespace())` - eat_ws = false; - } _ => {} } } diff --git a/src/test/ui/macros/issue-83340.rs b/src/test/ui/macros/issue-83340.rs new file mode 100644 index 0000000000000..d26200295cdcf --- /dev/null +++ b/src/test/ui/macros/issue-83340.rs @@ -0,0 +1,8 @@ +// check-fail + +fn main() { + println!( + "\ +\n {} │", //~ ERROR: 1 positional argument in format string, but no arguments were given + ); +} diff --git a/src/test/ui/macros/issue-83340.stderr b/src/test/ui/macros/issue-83340.stderr new file mode 100644 index 0000000000000..1935de02b57a9 --- /dev/null +++ b/src/test/ui/macros/issue-83340.stderr @@ -0,0 +1,8 @@ +error: 1 positional argument in format string, but no arguments were given + --> $DIR/issue-83340.rs:6:4 + | +LL | \n {} │", + | ^^ + +error: aborting due to previous error + From 6c45ebe8faf6e0e3b12a855429b6b732c04179d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20Sinan=20A=C4=9Facan?= Date: Sun, 21 Mar 2021 17:29:21 +0300 Subject: [PATCH 16/21] format macro argument parsing fix When the character next to `{}` is "shifted" (when mapping a byte index in the format string to span) we should avoid shifting the span end index, so first map the index of `}` to span, then bump the span, instead of first mapping the next byte index to a span (which causes bumping the end span too much). Regression test added. Fixes #83344 --- compiler/rustc_parse_format/src/lib.rs | 10 ++++++---- src/test/ui/macros/issue-83344.rs | 6 ++++++ src/test/ui/macros/issue-83344.stderr | 8 ++++++++ 3 files changed, 20 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/macros/issue-83344.rs create mode 100644 src/test/ui/macros/issue-83344.stderr diff --git a/compiler/rustc_parse_format/src/lib.rs b/compiler/rustc_parse_format/src/lib.rs index 92d974690b514..d49ddbd64c5eb 100644 --- a/compiler/rustc_parse_format/src/lib.rs +++ b/compiler/rustc_parse_format/src/lib.rs @@ -213,11 +213,13 @@ impl<'a> Iterator for Parser<'a> { Some(String(self.string(pos + 1))) } else { let arg = self.argument(); - if let Some(end) = self.must_consume('}') { - let start = self.to_span_index(pos); - let end = self.to_span_index(end + 1); + if let Some(rbrace_byte_idx) = self.must_consume('}') { + let lbrace_inner_offset = self.to_span_index(pos); + let rbrace_inner_offset = self.to_span_index(rbrace_byte_idx); if self.is_literal { - self.arg_places.push(start.to(end)); + self.arg_places.push( + lbrace_inner_offset.to(InnerOffset(rbrace_inner_offset.0 + 1)), + ); } } Some(NextArgument(arg)) diff --git a/src/test/ui/macros/issue-83344.rs b/src/test/ui/macros/issue-83344.rs new file mode 100644 index 0000000000000..8670425ca653b --- /dev/null +++ b/src/test/ui/macros/issue-83344.rs @@ -0,0 +1,6 @@ +// chekc-fail + +fn main() { + println!("{}\ +"); //~^ ERROR: 1 positional argument in format string, but no arguments were given +} diff --git a/src/test/ui/macros/issue-83344.stderr b/src/test/ui/macros/issue-83344.stderr new file mode 100644 index 0000000000000..1ef70f87a1fb4 --- /dev/null +++ b/src/test/ui/macros/issue-83344.stderr @@ -0,0 +1,8 @@ +error: 1 positional argument in format string, but no arguments were given + --> $DIR/issue-83344.rs:4:15 + | +LL | println!("{}\ + | ^^ + +error: aborting due to previous error + From 18748c9121f5910075ca4317a78ef11adb639263 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Mon, 22 Mar 2021 15:14:24 -0400 Subject: [PATCH 17/21] Make # format easier to discover --- library/alloc/src/fmt.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/library/alloc/src/fmt.rs b/library/alloc/src/fmt.rs index f9424b1d74744..8453f694e1e47 100644 --- a/library/alloc/src/fmt.rs +++ b/library/alloc/src/fmt.rs @@ -19,6 +19,10 @@ //! format!("{value}", value=4); // => "4" //! format!("{} {}", 1, 2); // => "1 2" //! format!("{:04}", 42); // => "0042" with leading zeros +//! format!("{:#?}", (100, 200)); // => "( +//! // 100, +//! // 200, +//! // )" //! ``` //! //! From these, you can see that the first argument is a format string. It is @@ -163,7 +167,7 @@ //! * `-` - Currently not used //! * `#` - This flag indicates that the "alternate" form of printing should //! be used. The alternate forms are: -//! * `#?` - pretty-print the [`Debug`] formatting +//! * `#?` - pretty-print the [`Debug`] formatting (newline and indent) //! * `#x` - precedes the argument with a `0x` //! * `#X` - precedes the argument with a `0x` //! * `#b` - precedes the argument with a `0b` From 93737dc634f42d07441db2acf26a88ae2e888d9f Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Mon, 22 Mar 2021 17:09:11 -0400 Subject: [PATCH 18/21] Update library/alloc/src/fmt.rs --- library/alloc/src/fmt.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/alloc/src/fmt.rs b/library/alloc/src/fmt.rs index 8453f694e1e47..e765fa9d14cc5 100644 --- a/library/alloc/src/fmt.rs +++ b/library/alloc/src/fmt.rs @@ -167,7 +167,7 @@ //! * `-` - Currently not used //! * `#` - This flag indicates that the "alternate" form of printing should //! be used. The alternate forms are: -//! * `#?` - pretty-print the [`Debug`] formatting (newline and indent) +//! * `#?` - pretty-print the [`Debug`] formatting (adds linebreaks and indentation) //! * `#x` - precedes the argument with a `0x` //! * `#X` - precedes the argument with a `0x` //! * `#b` - precedes the argument with a `0b` From b3ae90b4c20ae28c5dc1c141997869f9fd3148a6 Mon Sep 17 00:00:00 2001 From: Camelid Date: Tue, 23 Mar 2021 17:20:12 -0700 Subject: [PATCH 19/21] Tell GitHub to highlight `config.toml.example` as TOML This should be a nice small quality of life improvement when looking at `config.toml.example` on GitHub or looking at diffs of it in PRs. --- .gitattributes | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitattributes b/.gitattributes index d29c15fe712f3..51a670b5fbefd 100644 --- a/.gitattributes +++ b/.gitattributes @@ -9,6 +9,7 @@ src/etc/installer/gfx/* binary src/vendor/** -text Cargo.lock linguist-generated=false +config.toml.example linguist-language=TOML # Older git versions try to fix line endings on images and fonts, this prevents it. *.png binary From 3d8ce0aa55c182e569ad7015a0af752ef92e2572 Mon Sep 17 00:00:00 2001 From: Camelid Date: Tue, 9 Mar 2021 22:11:53 -0800 Subject: [PATCH 20/21] rustdoc: Use diagnostics for error when including sources This error probably almost never happens, but we should still use the diagnostic infrastructure. My guess is that the error was added back before rustdoc used the rustc diagnostic infrastructure (it was all `println!` and `eprintln!` back then!) and since it likely rarely occurs and this code doesn't change that much, no one thought to transition it to using diagnostics. Note that the old error was actually a warning (it didn't stop the rest of doc building). It seems very unlikely that this would fail without the rest of the doc build failing, so it makes more sense for it to be a hard error. The error looks like this: error: failed to render source code for `src/test/rustdoc/smart-punct.rs`: "bar": foo --> src/test/rustdoc/smart-punct.rs:3:1 | 3 | / #![crate_name = "foo"] 4 | | 5 | | //! This is the "start" of the 'document'! How'd you know that "it's" ... 6 | | //! ... | 22 | | //! I say "don't smart-punct me -- please!" 23 | | //! ``` | |_______^ I wasn't sure how to trigger the error, so to create that message I temporarily made rustdoc always emit it. That's also why it says "bar" and "foo" instead of a real error message. Note that the span of the diagnostic starts at line 3 because line 1 of that file is a (non-doc) comment and line 2 is a blank line. --- src/librustdoc/html/sources.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/librustdoc/html/sources.rs b/src/librustdoc/html/sources.rs index 1b6a82fed1170..001c8b090448b 100644 --- a/src/librustdoc/html/sources.rs +++ b/src/librustdoc/html/sources.rs @@ -54,12 +54,10 @@ impl DocFolder for SourceCollector<'_, '_> { self.scx.include_sources = match self.emit_source(&filename) { Ok(()) => true, Err(e) => { - println!( - "warning: source code was requested to be rendered, \ - but processing `{}` had an error: {}", - filename, e + self.scx.tcx.sess.span_err( + item.span.inner(), + &format!("failed to render source code for `{}`: {}", filename, e), ); - println!(" skipping rendering of source code"); false } }; From 8bc4de744fd243657026c3810a20d564c0c1ca9a Mon Sep 17 00:00:00 2001 From: Dominik Stolz Date: Thu, 25 Mar 2021 22:46:37 +0100 Subject: [PATCH 21/21] Remove broken test 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 | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/test/ui/command/command-pre-exec.rs b/src/test/ui/command/command-pre-exec.rs index 819ed0b2ddef7..392abc957838b 100644 --- a/src/test/ui/command/command-pre-exec.rs +++ b/src/test/ui/command/command-pre-exec.rs @@ -66,24 +66,6 @@ fn main() { }; assert_eq!(output.raw_os_error(), Some(102)); - let pid = unsafe { libc::getpid() }; - assert!(pid >= 0); - let output = unsafe { - Command::new(&me) - .arg("empty") - .pre_exec(move || { - let child = libc::getpid(); - assert!(child >= 0); - assert!(pid != child); - Ok(()) - }) - .output() - .unwrap() - }; - assert!(output.status.success()); - assert!(output.stderr.is_empty()); - assert!(output.stdout.is_empty()); - let mem = Arc::new(AtomicUsize::new(0)); let mem2 = mem.clone(); let output = unsafe {