From 85b55ce00df3766db2b617bda4b2457f6d76e542 Mon Sep 17 00:00:00 2001
From: Josh Stone <jistone@redhat.com>
Date: Fri, 15 Oct 2021 12:21:45 -0700
Subject: [PATCH 1/4] Only use `clone3` when needed for pidfd
In #89522 we learned that `clone3` is interacting poorly with Gentoo's
`sandbox` tool. We only need that for the unstable pidfd extensions, so
otherwise avoid that and use a normal `fork`.
---
library/std/src/sys/unix/process/process_unix.rs | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs
index 99013efb495d0..d0af2b6e9db0c 100644
--- a/library/std/src/sys/unix/process/process_unix.rs
+++ b/library/std/src/sys/unix/process/process_unix.rs
@@ -166,6 +166,10 @@ impl Command {
fn clone3(cl_args: *mut clone_args, len: libc::size_t) -> libc::c_long
}
+ // Bypassing libc for `clone3` can make further libc calls unsafe,
+ // so we use it sparingly for now. See #89522 for details.
+ let want_clone3_pidfd = self.get_create_pidfd();
+
// 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;
@@ -173,14 +177,9 @@ impl Command {
// 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;
- }
-
+ if want_clone3_pidfd && HAS_CLONE3.load(Ordering::Relaxed) {
let mut args = clone_args {
- flags,
+ flags: CLONE_PIDFD,
pidfd: &mut pidfd as *mut pid_t as u64,
child_tid: 0,
parent_tid: 0,
From fa2eee7bf2bcf03f64aa40a25f885b0301a9eb4a Mon Sep 17 00:00:00 2001
From: Josh Stone <jistone@redhat.com>
Date: Fri, 15 Oct 2021 14:45:23 -0700
Subject: [PATCH 2/4] Update another comment on fork vs. clone3
---
library/std/src/sys/unix/process/process_unix.rs | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs
index d0af2b6e9db0c..5e4eff75894b6 100644
--- a/library/std/src/sys/unix/process/process_unix.rs
+++ b/library/std/src/sys/unix/process/process_unix.rs
@@ -211,8 +211,8 @@ impl Command {
}
}
- // If we get here, the 'clone3' syscall does not exist
- // or we do not have permission to call it
+ // Generally, we just call `fork`. If we get here after wanting `clone3`,
+ // then the syscall does not exist or we do not have permission to call it.
cvt(libc::fork()).map(|res| (res, pidfd))
}
From 6edaaa6db80a1d35a9ecb48a3a9b32551b91dc5d Mon Sep 17 00:00:00 2001
From: Josh Stone <jistone@fedoraproject.org>
Date: Fri, 15 Oct 2021 15:29:15 -0700
Subject: [PATCH 3/4] Also note tool expectations of fork vs clone3
Co-authored-by: Josh Triplett <josh@joshtriplett.org>
---
library/std/src/sys/unix/process/process_unix.rs | 2 ++
1 file changed, 2 insertions(+)
diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs
index 5e4eff75894b6..326382d9038a8 100644
--- a/library/std/src/sys/unix/process/process_unix.rs
+++ b/library/std/src/sys/unix/process/process_unix.rs
@@ -168,6 +168,8 @@ impl Command {
// Bypassing libc for `clone3` can make further libc calls unsafe,
// so we use it sparingly for now. See #89522 for details.
+ // Some tools (e.g. sandboxing tools) may also expect `fork`
+ // rather than `clone3`.
let want_clone3_pidfd = self.get_create_pidfd();
// If we fail to create a pidfd for any reason, this will
From e96a0a8681998caf78093b65e746bfd967cb87e9 Mon Sep 17 00:00:00 2001
From: Josh Stone <jistone@redhat.com>
Date: Fri, 15 Oct 2021 16:04:52 -0700
Subject: [PATCH 4/4] Revert "Do not call getpid wrapper after fork in tests"
This reverts commit 12fbabd27f700a59d0e7031f0839b220c3514bcb.
It was only needed because of using raw `clone3` instead of `fork`, but
we only do that now when a pidfd is requested.
---
src/test/ui/command/command-pre-exec.rs | 25 +++++--------------
.../ui/process/process-panic-after-fork.rs | 17 +------------
2 files changed, 7 insertions(+), 35 deletions(-)
diff --git a/src/test/ui/command/command-pre-exec.rs b/src/test/ui/command/command-pre-exec.rs
index 10a8b19159e02..61914e2293070 100644
--- a/src/test/ui/command/command-pre-exec.rs
+++ b/src/test/ui/command/command-pre-exec.rs
@@ -8,6 +8,8 @@
// 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;
@@ -15,23 +17,6 @@ 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[..] {
@@ -83,12 +68,14 @@ fn main() {
};
assert_eq!(output.raw_os_error(), Some(102));
- let pid = getpid();
+ let pid = unsafe { libc::getpid() };
+ assert!(pid >= 0);
let output = unsafe {
Command::new(&me)
.arg("empty")
.pre_exec(move || {
- let child = getpid();
+ let child = libc::getpid();
+ assert!(child >= 0);
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 ad749371beac0..1ccf6bb051c20 100644
--- a/src/test/ui/process/process-panic-after-fork.rs
+++ b/src/test/ui/process/process-panic-after-fork.rs
@@ -23,21 +23,6 @@ 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<A> {
parent: A,
@@ -59,7 +44,7 @@ impl<A> PidChecking<A> {
fn check(&self) {
let require_pid = self.require_pid.load(Ordering::Acquire);
if require_pid != 0 {
- let actual_pid = getpid();
+ let actual_pid = process::id();
if require_pid != actual_pid {
unsafe {
libc::raise(libc::SIGUSR1);