From d854789ce191be25f2953c60fd50ce711776d9eb Mon Sep 17 00:00:00 2001 From: The8472 Date: Tue, 9 Mar 2021 21:42:38 +0100 Subject: [PATCH] Do not attempt to unlock envlock in child process after a fork. This is a breaking change for cases where the environment is accessed in a Command::pre_exec closure. Except for single-threaded programs these uses were not correct anyway since they aren't async-signal safe. --- library/std/src/sys/unix/ext/process.rs | 11 ++++++++++- library/std/src/sys/unix/process/process_unix.rs | 15 +++++++++------ src/test/ui/command/command-pre-exec.rs | 14 -------------- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/library/std/src/sys/unix/ext/process.rs b/library/std/src/sys/unix/ext/process.rs index 88a27f27f6628..d059ed44eed1b 100644 --- a/library/std/src/sys/unix/ext/process.rs +++ b/library/std/src/sys/unix/ext/process.rs @@ -62,9 +62,14 @@ pub trait CommandExt: Sealed { /// `fork`. This primarily means that any modifications made to memory on /// behalf of this closure will **not** be visible to the parent process. /// This is often a very constrained environment where normal operations - /// like `malloc` or acquiring a mutex are not guaranteed to work (due to + /// like `malloc`, accessing environment variables through [`std::env`] + /// or acquiring a mutex are not guaranteed to work (due to /// other threads perhaps still running when the `fork` was run). /// + /// For further details refer to the [POSIX fork() specification] + /// and the equivalent documentation for any targeted + /// platform, especially the requirements around *async-signal-safety*. + /// /// This also means that all resources such as file descriptors and /// memory-mapped regions got duplicated. It is your responsibility to make /// sure that the closure does not violate library invariants by making @@ -73,6 +78,10 @@ pub trait CommandExt: Sealed { /// When this closure is run, aspects such as the stdio file descriptors and /// working directory have successfully been changed, so output to these /// locations may not appear where intended. + /// + /// [POSIX fork() specification]: + /// https://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html + /// [`std::env`]: mod@crate::env #[stable(feature = "process_pre_exec", since = "1.34.0")] unsafe fn pre_exec(&mut self, f: F) -> &mut process::Command where diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index 2746f87468dca..ce20c7196f4b7 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -1,6 +1,7 @@ use crate::convert::TryInto; use crate::fmt; use crate::io::{self, Error, ErrorKind}; +use crate::mem; use crate::ptr; use crate::sys; use crate::sys::cvt; @@ -45,15 +46,14 @@ impl Command { // // Note that as soon as we're done with the fork there's no need to hold // a lock any more because the parent won't do anything and the child is - // in its own process. - let result = unsafe { - let _env_lock = sys::os::env_lock(); - cvt(libc::fork())? - }; + // 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_lock(), cvt(libc::fork())?) }; let pid = unsafe { match result { 0 => { + mem::forget(env_lock); drop(input); let Err(err) = self.do_exec(theirs, envp.as_ref()); let errno = err.raw_os_error().unwrap_or(libc::EINVAL) as u32; @@ -74,7 +74,10 @@ impl Command { rtassert!(output.write(&bytes).is_ok()); libc::_exit(1) } - n => n, + n => { + drop(env_lock); + n + } } }; diff --git a/src/test/ui/command/command-pre-exec.rs b/src/test/ui/command/command-pre-exec.rs index 8fc6a220331f0..819ed0b2ddef7 100644 --- a/src/test/ui/command/command-pre-exec.rs +++ b/src/test/ui/command/command-pre-exec.rs @@ -43,20 +43,6 @@ fn main() { assert!(output.stderr.is_empty()); assert_eq!(output.stdout, b"hello\nhello2\n"); - let output = unsafe { - Command::new(&me) - .arg("test2") - .pre_exec(|| { - env::set_var("FOO", "BAR"); - Ok(()) - }) - .output() - .unwrap() - }; - assert!(output.status.success()); - assert!(output.stderr.is_empty()); - assert!(output.stdout.is_empty()); - let output = unsafe { Command::new(&me) .arg("test3")