Skip to content

Commit

Permalink
Do not attempt to unlock envlock in child process after a fork.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
the8472 committed Mar 9, 2021
1 parent 3a5d45f commit d854789
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 21 deletions.
11 changes: 10 additions & 1 deletion library/std/src/sys/unix/ext/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
15 changes: 9 additions & 6 deletions library/std/src/sys/unix/process/process_unix.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -74,7 +74,10 @@ impl Command {
rtassert!(output.write(&bytes).is_ok());
libc::_exit(1)
}
n => n,
n => {
drop(env_lock);
n
}
}
};

Expand Down
14 changes: 0 additions & 14 deletions src/test/ui/command/command-pre-exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down

0 comments on commit d854789

Please sign in to comment.