Auto merge of #142035 - ChrisDenton:parent-path, r=<try>

Add `Command::resolve_in_parent_path`

This is part of #141976 (ACP: rust-lang/libs-team#591). libs-api could not come to a consensus on what the final API should look like so they've agreed to experiment with a few different approaches.

This PR just implements the `resolve_in_parent_path` method which forces `Command` to ignore `PATH` set on the child for the purposes of resolving the executable. There was no consensus on what should happen if, for example, `chroot` is set (which can change the meaning of paths) so for now I've just done the easy thing. Figuring out the correct behaviour here will need to be done before this is considered for stabilisation but hopefully having it on nightly will nudge people in to giving their opinions.
try-job: `aarch64-apple`
try-job: `x86_64-msvc-*`
try-job: `dist-various-*`

Author: rust-bors[bot]

library/std/src/process.rs

@@ -1195,6 +1195,22 @@ impl Command {
     pub fn get_current_dir(&self) -> Option<&Path> {
         self.inner.get_current_dir()
     }
+
+    /// Resolve the program given in [`new`](Self::new) using the parent's PATH.
+    ///
+    /// Usually when a `env` is used to set `PATH` on a child process,
+    /// that new `PATH` will be used to discover the process.
+    /// With `resolve_in_parent_path` to `true`, the parent's `PATH` will be used instead.
+    ///
+    /// # Platform-specific behavior
+    ///
+    /// On Unix the process is executed after fork and after setting up the rest of the new environment.
+    /// So if `chroot`, `uid`, `gid` or `groups` are used then the way `PATH` is interpreted may be changed.
+    #[unstable(feature = "command_resolve", issue = "none")]
+    pub fn resolve_in_parent_path(&mut self, use_parent: bool) -> &mut Self {
+        self.inner.resolve_in_parent_path(use_parent);
+        self
+    }
 }
 
 #[stable(feature = "rust1", since = "1.0.0")]

library/std/src/process/tests.rs

@@ -2,10 +2,10 @@ use super::{Command, Output, Stdio};
 use crate::io::prelude::*;
 use crate::io::{BorrowedBuf, ErrorKind};
 use crate::mem::MaybeUninit;
-use crate::str;
+use crate::{fs, str};
 
 fn known_command() -> Command {
-    if cfg!(windows) { Command::new("help") } else { Command::new("echo") }
+    if cfg!(windows) { Command::new("help.exe") } else { Command::new("echo") }
 }
 
 #[cfg(target_os = "android")]
@@ -603,3 +603,42 @@ fn terminate_exited_process() {
     assert!(p.kill().is_ok());
     assert!(p.kill().is_ok());
 }
+
+#[test]
+fn resolve_in_parent_path() {
+    let tempdir = crate::test_helpers::tmpdir();
+    let mut cmd = if cfg!(windows) {
+        // On Windows std prepends the system paths when searching for executables
+        // but not if PATH is set on the command. Therefore adding a broken exe
+        // using PATH will cause spawn to fail if it's invoked.
+        let mut cmd = known_command();
+        fs::write(tempdir.join(cmd.get_program().to_str().unwrap()), "").unwrap();
+        cmd.env("PATH", tempdir.path());
+        cmd
+    } else {
+        let mut cmd = known_command();
+        cmd.env("PATH", "");
+        cmd
+    };
+
+    assert!(cmd.spawn().is_err());
+    cmd.resolve_in_parent_path(true);
+    assert!(cmd.status().unwrap().success());
+    cmd.resolve_in_parent_path(false);
+    assert!(cmd.spawn().is_err());
+
+    // If the platform is a unix and has posix_spawn then that will be used in the test above.
+    // So we also test the fork/exec path by setting a pre_exec function.
+    #[cfg(unix)]
+    {
+        use crate::os::unix::process::CommandExt;
+        unsafe {
+            cmd.pre_exec(|| Ok(()));
+        }
+        assert!(cmd.spawn().is_err());
+        cmd.resolve_in_parent_path(true);
+        assert!(cmd.status().unwrap().success());
+        cmd.resolve_in_parent_path(false);
+        assert!(cmd.spawn().is_err());
+    }
+}

library/std/src/sys/process/unix/common.rs

@@ -98,6 +98,7 @@ pub struct Command {
     #[cfg(target_os = "linux")]
     create_pidfd: bool,
     pgroup: Option<pid_t>,
+    resolve_in_parent_path: bool,
 }
 
 // passed back to std::process with the pipes connected to the child, if any
@@ -185,6 +186,7 @@ impl Command {
             #[cfg(target_os = "linux")]
             create_pidfd: false,
             pgroup: None,
+            resolve_in_parent_path: false,
         }
     }
 
@@ -220,6 +222,9 @@ impl Command {
             self.cwd(&OsStr::new("/"));
         }
     }
+    pub fn resolve_in_parent_path(&mut self, use_parent: bool) {
+        self.resolve_in_parent_path = use_parent;
+    }
 
     #[cfg(target_os = "linux")]
     pub fn create_pidfd(&mut self, val: bool) {
@@ -298,6 +303,9 @@ impl Command {
     pub fn get_chroot(&self) -> Option<&CStr> {
         self.chroot.as_deref()
     }
+    pub fn get_resolve_in_parent_path(&self) -> bool {
+        self.resolve_in_parent_path
+    }
 
     pub fn get_closures(&mut self) -> &mut Vec<Box<dyn FnMut() -> io::Result<()> + Send + Sync>> {
         &mut self.closures

library/std/src/sys/process/unix/unix.rs

@@ -89,6 +89,7 @@ impl Command {
         // The child calls `mem::forget` to leak the lock, which is crucial because
         // releasing a lock is not async-signal-safe.
         let env_lock = sys::env::env_read_lock();
+
         let pid = unsafe { self.do_fork()? };
 
         if pid == 0 {
@@ -276,7 +277,7 @@ impl Command {
     unsafe fn do_exec(
         &mut self,
         stdio: ChildPipes,
-        maybe_envp: Option<&CStringArray>,
+        mut maybe_envp: Option<&CStringArray>,
     ) -> Result<!, io::Error> {
         use crate::sys::{self, cvt_r};
 
@@ -378,13 +379,17 @@ impl Command {
             callback()?;
         }
 
-        // Although we're performing an exec here we may also return with an
-        // error from this function (without actually exec'ing) in which case we
-        // want to be sure to restore the global environment back to what it
-        // once was, ensuring that our temporary override, when free'd, doesn't
-        // corrupt our process's environment.
         let mut _reset = None;
-        if let Some(envp) = maybe_envp {
+        if let Some(envp) = maybe_envp
+            && (!self.get_resolve_in_parent_path()
+                || !self.env_saw_path()
+                || self.get_program_kind() != ProgramKind::PathLookup)
+        {
+            // Although we're performing an exec here we may also return with an
+            // error from this function (without actually exec'ing) in which case we
+            // want to be sure to restore the global environment back to what it
+            // once was, ensuring that our temporary override, when free'd, doesn't
+            // corrupt our process's environment.
             struct Reset(*const *const libc::c_char);
 
             impl Drop for Reset {
@@ -397,9 +402,69 @@ impl Command {
 
             _reset = Some(Reset(*sys::env::environ()));
             *sys::env::environ() = envp.as_ptr();
+            // We've set the environment, no need to set it again.
+            maybe_envp = None;
         }
 
-        libc::execvp(self.get_program_cstr().as_ptr(), self.get_argv().as_ptr());
+        use crate::ffi::CStr;
+        // execvpe is a gnu extension...
+        #[cfg(all(target_os = "linux", target_env = "gnu"))]
+        unsafe fn exec_with_env(
+            program: &CStr,
+            args: &CStringArray,
+            envp: &CStringArray,
+        ) -> libc::c_int {
+            unsafe { libc::execvpe(program.as_ptr(), args.as_ptr(), envp.as_ptr()) }
+        }
+
+        // ...so if we're not gnu then use our own implementation.
+        #[cfg(not(all(target_os = "linux", target_env = "gnu")))]
+        unsafe fn exec_with_env(
+            program: &CStr,
+            args: &CStringArray,
+            envp: &CStringArray,
+        ) -> libc::c_int {
+            unsafe {
+                let name = program.to_bytes();
+                let mut buffer =
+                    [const { mem::MaybeUninit::<u8>::uninit() }; libc::PATH_MAX as usize];
+                let mut environ = *sys::env::environ();
+                // Search the environment for PATH and, if found,
+                // search the paths for the executable by trying to execve each candidate.
+                while !(*environ).is_null() {
+                    let kv = CStr::from_ptr(*environ);
+                    if let Some(value) = kv.to_bytes().strip_prefix(b"PATH=") {
+                        for path in value.split(|&b| b == b':') {
+                            if buffer.len() - 2 >= path.len().saturating_add(name.len()) {
+                                let buf_ptr = buffer.as_mut_ptr().cast::<u8>();
+                                let mut offset = 0;
+                                if !path.is_empty() {
+                                    buf_ptr.copy_from(path.as_ptr(), path.len());
+                                    offset += path.len();
+                                    if path.last() != Some(&b'/') {
+                                        *buf_ptr.add(path.len()) = b'/';
+                                        offset += 1;
+                                    }
+                                }
+                                buf_ptr.add(offset).copy_from(name.as_ptr(), name.len());
+                                offset += name.len();
+                                *buf_ptr.add(offset) = 0;
+                                libc::execve(buf_ptr.cast(), args.as_ptr(), envp.as_ptr());
+                            }
+                        }
+                        break;
+                    }
+                    environ = environ.add(1);
+                }
+                // If execve is successful then it'll never return, thus we'll never reach this point.
+                -1
+            }
+        }
+        if let Some(envp) = maybe_envp {
+            exec_with_env(self.get_program_cstr(), self.get_argv(), envp);
+        } else {
+            libc::execvp(self.get_program_cstr().as_ptr(), self.get_argv().as_ptr());
+        }
         Err(io::Error::last_os_error())
     }
 
@@ -453,7 +518,9 @@ impl Command {
 
         if self.get_gid().is_some()
             || self.get_uid().is_some()
-            || (self.env_saw_path() && !self.program_is_path())
+            || (!self.get_resolve_in_parent_path()
+                && self.env_saw_path()
+                && !self.program_is_path())
             || !self.get_closures().is_empty()
             || self.get_groups().is_some()
             || self.get_chroot().is_some()
@@ -793,6 +860,7 @@ impl Command {
             // Safety: -1 indicates we don't have a pidfd.
             let mut p = Process::new(0, -1);
 
+            // FIXME: if a list of paths to search is passed then retry for every path.
             let spawn_res = spawn_fn(
                 &mut p.pid,
                 self.get_program_cstr().as_ptr(),

library/std/src/sys/process/unsupported.rs

@@ -21,6 +21,7 @@ pub struct Command {
     stdin: Option<Stdio>,
     stdout: Option<Stdio>,
     stderr: Option<Stdio>,
+    force_parent_path: bool,
 }
 
 // passed back to std::process with the pipes connected to the child, if any
@@ -52,6 +53,7 @@ impl Command {
             stdin: None,
             stdout: None,
             stderr: None,
+            force_parent_path: false,
         }
     }
 
@@ -79,6 +81,10 @@ impl Command {
         self.stderr = Some(stderr);
     }
 
+    pub fn resolve_in_parent_path(&mut self, use_parent: bool) {
+        self.force_parent_path = use_parent;
+    }
+
     pub fn get_program(&self) -> &OsStr {
         &self.program
     }

library/std/src/sys/process/windows.rs

@@ -158,6 +158,7 @@ pub struct Command {
     startupinfo_fullscreen: bool,
     startupinfo_untrusted_source: bool,
     startupinfo_force_feedback: Option<bool>,
+    force_parent_path: bool,
 }
 
 pub enum Stdio {
@@ -192,6 +193,7 @@ impl Command {
             startupinfo_fullscreen: false,
             startupinfo_untrusted_source: false,
             startupinfo_force_feedback: None,
+            force_parent_path: false,
         }
     }
 
@@ -224,6 +226,10 @@ impl Command {
         self.force_quotes_enabled = enabled;
     }
 
+    pub fn resolve_in_parent_path(&mut self, use_parent: bool) {
+        self.force_parent_path = use_parent;
+    }
+
     pub fn raw_arg(&mut self, command_str_to_append: &OsStr) {
         self.args.push(Arg::Raw(command_str_to_append.to_os_string()))
     }
@@ -274,7 +280,10 @@ impl Command {
         let env_saw_path = self.env.have_changed_path();
         let maybe_env = self.env.capture_if_changed();
 
-        let child_paths = if env_saw_path && let Some(env) = maybe_env.as_ref() {
+        let child_paths = if env_saw_path
+            && !self.force_parent_path
+            && let Some(env) = maybe_env.as_ref()
+        {
             env.get(&EnvKey::new("PATH")).map(|s| s.as_os_str())
         } else {
             None