Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not attempt to unlock envlock in child process after a fork. #82949

Merged
merged 1 commit into from
Mar 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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