Rollup merge of rust-lang#58059 - RalfJung:before_exec, r=alexcrichton

deprecate before_exec in favor of unsafe pre_exec

Fixes rust-lang#39575

As per the [lang team decision](rust-lang#39575 (comment)):

> The language team agreed that before_exec should be unsafe, and leaves the details of a transition plan to the libs team.

Cc @alexcrichton @rust-lang/libs how would you like to proceed?

Author: Centril

src/libstd/sys/redox/ext/process.rs

@@ -36,7 +36,7 @@ pub trait CommandExt {
     /// will be called and the spawn operation will immediately return with a
     /// failure.
     ///
-    /// # Notes
+    /// # Notes and Safety
     ///
     /// This closure will be run in the context of the child process after a
     /// `fork`. This primarily means that any modifications made to memory on
@@ -45,12 +45,32 @@ pub trait CommandExt {
     /// like `malloc` or acquiring a mutex are not guaranteed to work (due to
     /// other threads perhaps still running when the `fork` was run).
     ///
+    /// 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
+    /// invalid use of these duplicates.
+    ///
     /// 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.
+    #[stable(feature = "process_pre_exec", since = "1.34.0")]
+    unsafe fn pre_exec<F>(&mut self, f: F) -> &mut process::Command
+        where F: FnMut() -> io::Result<()> + Send + Sync + 'static;
+
+    /// Schedules a closure to be run just before the `exec` function is
+    /// invoked.
+    ///
+    /// This method is stable and usable, but it should be unsafe. To fix
+    /// that, it got deprecated in favor of the unsafe [`pre_exec`].
+    ///
+    /// [`pre_exec`]: #tymethod.pre_exec
     #[stable(feature = "process_exec", since = "1.15.0")]
+    #[rustc_deprecated(since = "1.37.0", reason = "should be unsafe, use `pre_exec` instead")]
     fn before_exec<F>(&mut self, f: F) -> &mut process::Command
-        where F: FnMut() -> io::Result<()> + Send + Sync + 'static;
+        where F: FnMut() -> io::Result<()> + Send + Sync + 'static
+    {
+        unsafe { self.pre_exec(f) }
+    }
 
     /// Performs all the required setup by this `Command`, followed by calling
     /// the `execvp` syscall.
@@ -87,10 +107,10 @@ impl CommandExt for process::Command {
         self
     }
 
-    fn before_exec<F>(&mut self, f: F) -> &mut process::Command
+    unsafe fn pre_exec<F>(&mut self, f: F) -> &mut process::Command
         where F: FnMut() -> io::Result<()> + Send + Sync + 'static
     {
-        self.as_inner_mut().before_exec(Box::new(f));
+        self.as_inner_mut().pre_exec(Box::new(f));
         self
     }
 

src/libstd/sys/redox/process.rs

@@ -116,8 +116,10 @@ impl Command {
         self.gid = Some(id);
     }
 
-    pub fn before_exec(&mut self,
-                       f: Box<dyn FnMut() -> io::Result<()> + Send + Sync>) {
+    pub unsafe fn pre_exec(
+        &mut self,
+        f: Box<dyn FnMut() -> io::Result<()> + Send + Sync>,
+    ) {
         self.closures.push(f);
     }
 

src/libstd/sys/unix/ext/process.rs

@@ -36,7 +36,7 @@ pub trait CommandExt {
     /// will be called and the spawn operation will immediately return with a
     /// failure.
     ///
-    /// # Notes
+    /// # Notes and Safety
     ///
     /// This closure will be run in the context of the child process after a
     /// `fork`. This primarily means that any modifications made to memory on
@@ -45,12 +45,32 @@ pub trait CommandExt {
     /// like `malloc` or acquiring a mutex are not guaranteed to work (due to
     /// other threads perhaps still running when the `fork` was run).
     ///
+    /// 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
+    /// invalid use of these duplicates.
+    ///
     /// 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.
+    #[stable(feature = "process_pre_exec", since = "1.34.0")]
+    unsafe fn pre_exec<F>(&mut self, f: F) -> &mut process::Command
+        where F: FnMut() -> io::Result<()> + Send + Sync + 'static;
+
+    /// Schedules a closure to be run just before the `exec` function is
+    /// invoked.
+    ///
+    /// This method is stable and usable, but it should be unsafe. To fix
+    /// that, it got deprecated in favor of the unsafe [`pre_exec`].
+    ///
+    /// [`pre_exec`]: #tymethod.pre_exec
     #[stable(feature = "process_exec", since = "1.15.0")]
+    #[rustc_deprecated(since = "1.37.0", reason = "should be unsafe, use `pre_exec` instead")]
     fn before_exec<F>(&mut self, f: F) -> &mut process::Command
-        where F: FnMut() -> io::Result<()> + Send + Sync + 'static;
+        where F: FnMut() -> io::Result<()> + Send + Sync + 'static
+    {
+        unsafe { self.pre_exec(f) }
+    }
 
     /// Performs all the required setup by this `Command`, followed by calling
     /// the `execvp` syscall.
@@ -97,10 +117,10 @@ impl CommandExt for process::Command {
         self
     }
 
-    fn before_exec<F>(&mut self, f: F) -> &mut process::Command
+    unsafe fn pre_exec<F>(&mut self, f: F) -> &mut process::Command
         where F: FnMut() -> io::Result<()> + Send + Sync + 'static
     {
-        self.as_inner_mut().before_exec(Box::new(f));
+        self.as_inner_mut().pre_exec(Box::new(f));
         self
     }
 

src/libstd/sys/unix/process/process_common.rs

@@ -149,8 +149,10 @@ impl Command {
         &mut self.closures
     }
 
-    pub fn before_exec(&mut self,
-                       f: Box<dyn FnMut() -> io::Result<()> + Send + Sync>) {
+    pub unsafe fn pre_exec(
+        &mut self,
+        f: Box<dyn FnMut() -> io::Result<()> + Send + Sync>,
+    ) {
         self.closures.push(f);
     }
 

src/test/run-pass/command-before-exec.rs

@@ -0,0 +1,115 @@
+#![allow(stable_features)]
+// ignore-windows - this is a unix-specific test
+// ignore-cloudabi no processes
+// ignore-emscripten no processes
+#![feature(process_exec, rustc_private)]
+
+extern crate libc;
+
+use std::env;
+use std::io::Error;
+use std::os::unix::process::CommandExt;
+use std::process::Command;
+use std::sync::atomic::{AtomicUsize, Ordering};
+use std::sync::Arc;
+
+fn main() {
+    if let Some(arg) = env::args().nth(1) {
+        match &arg[..] {
+            "test1" => println!("hello2"),
+            "test2" => assert_eq!(env::var("FOO").unwrap(), "BAR"),
+            "test3" => assert_eq!(env::current_dir().unwrap().to_str().unwrap(), "/"),
+            "empty" => {}
+            _ => panic!("unknown argument: {}", arg),
+        }
+        return;
+    }
+
+    let me = env::current_exe().unwrap();
+
+    let output = unsafe {
+        Command::new(&me)
+            .arg("test1")
+            .pre_exec(|| {
+                println!("hello");
+                Ok(())
+            })
+            .output()
+            .unwrap()
+    };
+    assert!(output.status.success());
+    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")
+            .pre_exec(|| {
+                env::set_current_dir("/").unwrap();
+                Ok(())
+            })
+            .output()
+            .unwrap()
+    };
+    assert!(output.status.success());
+    assert!(output.stderr.is_empty());
+    assert!(output.stdout.is_empty());
+
+    let output = unsafe {
+        Command::new(&me)
+            .arg("bad")
+            .pre_exec(|| Err(Error::from_raw_os_error(102)))
+            .output()
+            .unwrap_err()
+    };
+    assert_eq!(output.raw_os_error(), Some(102));
+
+    let pid = unsafe { libc::getpid() };
+    assert!(pid >= 0);
+    let output = unsafe {
+        Command::new(&me)
+            .arg("empty")
+            .pre_exec(move || {
+                let child = libc::getpid();
+                assert!(child >= 0);
+                assert!(pid != child);
+                Ok(())
+            })
+            .output()
+            .unwrap()
+    };
+    assert!(output.status.success());
+    assert!(output.stderr.is_empty());
+    assert!(output.stdout.is_empty());
+
+    let mem = Arc::new(AtomicUsize::new(0));
+    let mem2 = mem.clone();
+    let output = unsafe {
+        Command::new(&me)
+            .arg("empty")
+            .pre_exec(move || {
+                assert_eq!(mem2.fetch_add(1, Ordering::SeqCst), 0);
+                Ok(())
+            })
+            .output()
+            .unwrap()
+    };
+    assert!(output.status.success());
+    assert!(output.stderr.is_empty());
+    assert!(output.stdout.is_empty());
+    assert_eq!(mem.load(Ordering::SeqCst), 0);
+}