From d854789ce191be25f2953c60fd50ce711776d9eb Mon Sep 17 00:00:00 2001
From: The8472 <git@infinite-source.de>
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 88a27f27f66..d059ed44eed 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<F>(&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 2746f87468d..ce20c7196f4 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 8fc6a220331..819ed0b2dde 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")