Explicitly pass PATH to the Windows exe resolver

Author: ChrisDenton

Diff for: library/std/src/sys/windows/process.rs

@@ -268,7 +268,7 @@ impl Command {
         } else {
             None
         };
-        let program = resolve_exe(&self.program, child_paths)?;
+        let program = resolve_exe(&self.program, || env::var_os("PATH"), child_paths)?;
         let mut cmd_str =
             make_command_line(program.as_os_str(), &self.args, self.force_quotes_enabled)?;
         cmd_str.push(0); // add null terminator
@@ -362,7 +362,11 @@ impl fmt::Debug for Command {
 // Therefore this functions first assumes `.exe` was intended.
 // It falls back to the plain file name if a full path is given and the extension is omitted
 // or if only a file name is given and it already contains an extension.
-fn resolve_exe<'a>(exe_path: &'a OsStr, child_paths: Option<&OsStr>) -> io::Result<PathBuf> {
+fn resolve_exe<'a>(
+    exe_path: &'a OsStr,
+    parent_paths: impl FnOnce() -> Option<OsString>,
+    child_paths: Option<&OsStr>,
+) -> io::Result<PathBuf> {
     // Early return if there is no filename.
     if exe_path.is_empty() || path::has_trailing_slash(exe_path) {
         return Err(io::Error::new_const(
@@ -406,7 +410,7 @@ fn resolve_exe<'a>(exe_path: &'a OsStr, child_paths: Option<&OsStr>) -> io::Resu
         let has_extension = exe_path.bytes().contains(&b'.');
 
         // Search the directories given by `search_paths`.
-        let result = search_paths(child_paths, |mut path| {
+        let result = search_paths(parent_paths, child_paths, |mut path| {
             path.push(&exe_path);
             if !has_extension {
                 path.set_extension(EXE_EXTENSION);
@@ -423,15 +427,20 @@ fn resolve_exe<'a>(exe_path: &'a OsStr, child_paths: Option<&OsStr>) -> io::Resu
 
 // Calls `f` for every path that should be used to find an executable.
 // Returns once `f` returns the path to an executable or all paths have been searched.
-fn search_paths<F>(child_paths: Option<&OsStr>, mut f: F) -> Option<PathBuf>
+fn search_paths<Paths, Exists>(
+    parent_paths: Paths,
+    child_paths: Option<&OsStr>,
+    mut exists: Exists,
+) -> Option<PathBuf>
 where
-    F: FnMut(PathBuf) -> Option<PathBuf>,
+    Paths: FnOnce() -> Option<OsString>,
+    Exists: FnMut(PathBuf) -> Option<PathBuf>,
 {
     // 1. Child paths
     // This is for consistency with Rust's historic behaviour.
     if let Some(paths) = child_paths {
         for path in env::split_paths(paths).filter(|p| !p.as_os_str().is_empty()) {
-            if let Some(path) = f(path) {
+            if let Some(path) = exists(path) {
                 return Some(path);
             }
         }
@@ -440,7 +449,7 @@ where
     // 2. Application path
     if let Ok(mut app_path) = env::current_exe() {
         app_path.pop();
-        if let Some(path) = f(app_path) {
+        if let Some(path) = exists(app_path) {
             return Some(path);
         }
     }
@@ -450,25 +459,25 @@ where
     unsafe {
         if let Ok(Some(path)) = super::fill_utf16_buf(
             |buf, size| c::GetSystemDirectoryW(buf, size),
-            |buf| f(PathBuf::from(OsString::from_wide(buf))),
+            |buf| exists(PathBuf::from(OsString::from_wide(buf))),
         ) {
             return Some(path);
         }
         #[cfg(not(target_vendor = "uwp"))]
         {
             if let Ok(Some(path)) = super::fill_utf16_buf(
                 |buf, size| c::GetWindowsDirectoryW(buf, size),
-                |buf| f(PathBuf::from(OsString::from_wide(buf))),
+                |buf| exists(PathBuf::from(OsString::from_wide(buf))),
             ) {
                 return Some(path);
             }
         }
     }
 
     // 5. Parent paths
-    if let Some(parent_paths) = env::var_os("PATH") {
+    if let Some(parent_paths) = parent_paths() {
         for path in env::split_paths(&parent_paths).filter(|p| !p.as_os_str().is_empty()) {
-            if let Some(path) = f(path) {
+            if let Some(path) = exists(path) {
                 return Some(path);
             }
         }

Diff for: library/std/src/sys/windows/process/tests.rs

@@ -136,51 +136,46 @@ fn windows_exe_resolver() {
     use super::resolve_exe;
     use crate::io;
 
+    let env_paths = || env::var_os("PATH");
+
     // Test a full path, with and without the `exe` extension.
     let mut current_exe = env::current_exe().unwrap();
-    assert!(resolve_exe(current_exe.as_ref(), None).is_ok());
+    assert!(resolve_exe(current_exe.as_ref(), env_paths, None).is_ok());
     current_exe.set_extension("");
-    assert!(resolve_exe(current_exe.as_ref(), None).is_ok());
+    assert!(resolve_exe(current_exe.as_ref(), env_paths, None).is_ok());
 
     // Test lone file names.
-    assert!(resolve_exe(OsStr::new("cmd"), None).is_ok());
-    assert!(resolve_exe(OsStr::new("cmd.exe"), None).is_ok());
-    assert!(resolve_exe(OsStr::new("cmd.EXE"), None).is_ok());
-    assert!(resolve_exe(OsStr::new("fc"), None).is_ok());
+    assert!(resolve_exe(OsStr::new("cmd"), env_paths, None).is_ok());
+    assert!(resolve_exe(OsStr::new("cmd.exe"), env_paths, None).is_ok());
+    assert!(resolve_exe(OsStr::new("cmd.EXE"), env_paths, None).is_ok());
+    assert!(resolve_exe(OsStr::new("fc"), env_paths, None).is_ok());
 
     // Invalid file names should return InvalidInput.
-    assert_eq!(resolve_exe(OsStr::new(""), None).unwrap_err().kind(), io::ErrorKind::InvalidInput);
     assert_eq!(
-        resolve_exe(OsStr::new("\0"), None).unwrap_err().kind(),
+        resolve_exe(OsStr::new(""), env_paths, None).unwrap_err().kind(),
+        io::ErrorKind::InvalidInput
+    );
+    assert_eq!(
+        resolve_exe(OsStr::new("\0"), env_paths, None).unwrap_err().kind(),
         io::ErrorKind::InvalidInput
     );
     // Trailing slash, therefore there's no file name component.
     assert_eq!(
-        resolve_exe(OsStr::new(r"C:\Path\to\"), None).unwrap_err().kind(),
+        resolve_exe(OsStr::new(r"C:\Path\to\"), env_paths, None).unwrap_err().kind(),
         io::ErrorKind::InvalidInput
     );
 
-    /* FIXME: fix and re-enable these tests before making changes to the resolver.
-
     /*
     Some of the following tests may need to be changed if you are deliberately
     changing the behaviour of `resolve_exe`.
     */
 
-    let paths = env::var_os("PATH").unwrap();
-    env::set_var("PATH", "");
-
-    assert_eq!(resolve_exe(OsStr::new("rustc"), None).unwrap_err().kind(), io::ErrorKind::NotFound);
-
-    let child_paths = Some(paths.as_os_str());
-    assert!(resolve_exe(OsStr::new("rustc"), child_paths).is_ok());
+    let empty_paths = || None;
 
     // The resolver looks in system directories even when `PATH` is empty.
-    assert!(resolve_exe(OsStr::new("cmd.exe"), None).is_ok());
+    assert!(resolve_exe(OsStr::new("cmd.exe"), empty_paths, None).is_ok());
 
     // The application's directory is also searched.
     let current_exe = env::current_exe().unwrap();
-    assert!(resolve_exe(current_exe.file_name().unwrap().as_ref(), None).is_ok());
-
-    */
+    assert!(resolve_exe(current_exe.file_name().unwrap().as_ref(), empty_paths, None).is_ok());
 }