From 7ae2017325ded1047e04ac7e6d5968e514e2a569 Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Fri, 29 Apr 2022 16:31:26 +0100 Subject: [PATCH] Revert "Rollup merge of #92519 - ChrisDenton:command-maybe-verbatim, r=dtolnay" This reverts commit ba2d5ede70ed7e37d7f13a397b9d554e2386a19c, reversing changes made to 9b701e7eaa08c2b2ef8c6e59b8b33436cb10aa45. --- library/std/src/sys/windows/process.rs | 67 ++++++++------------ library/std/src/sys/windows/process/tests.rs | 7 +- 2 files changed, 30 insertions(+), 44 deletions(-) diff --git a/library/std/src/sys/windows/process.rs b/library/std/src/sys/windows/process.rs index a13585a02224a..fafd1412d4cb3 100644 --- a/library/std/src/sys/windows/process.rs +++ b/library/std/src/sys/windows/process.rs @@ -19,12 +19,12 @@ use crate::path::{Path, PathBuf}; use crate::ptr; use crate::sys::c; use crate::sys::c::NonZeroDWORD; -use crate::sys::cvt; use crate::sys::fs::{File, OpenOptions}; use crate::sys::handle::Handle; use crate::sys::path; use crate::sys::pipe::{self, AnonPipe}; use crate::sys::stdio; +use crate::sys::{cvt, to_u16s}; use crate::sys_common::mutex::StaticMutex; use crate::sys_common::process::{CommandEnv, CommandEnvs}; use crate::sys_common::{AsInner, IntoInner}; @@ -269,13 +269,8 @@ impl Command { None }; let program = resolve_exe(&self.program, || env::var_os("PATH"), child_paths)?; - // Case insensitive "ends_with" of UTF-16 encoded ".bat" or ".cmd" - let is_batch_file = matches!( - program.len().checked_sub(5).and_then(|i| program.get(i..)), - Some([46, 98 | 66, 97 | 65, 116 | 84, 0] | [46, 99 | 67, 109 | 77, 100 | 68, 0]) - ); let mut cmd_str = - make_command_line(&program, &self.args, self.force_quotes_enabled, is_batch_file)?; + make_command_line(program.as_os_str(), &self.args, self.force_quotes_enabled)?; cmd_str.push(0); // add null terminator // stolen from the libuv code. @@ -314,6 +309,7 @@ impl Command { si.hStdOutput = stdout.as_raw_handle(); si.hStdError = stderr.as_raw_handle(); + let program = to_u16s(&program)?; unsafe { cvt(c::CreateProcessW( program.as_ptr(), @@ -370,7 +366,7 @@ fn resolve_exe<'a>( exe_path: &'a OsStr, parent_paths: impl FnOnce() -> Option, child_paths: Option<&OsStr>, -) -> io::Result> { +) -> io::Result { // Early return if there is no filename. if exe_path.is_empty() || path::has_trailing_slash(exe_path) { return Err(io::const_io_error!( @@ -392,19 +388,19 @@ fn resolve_exe<'a>( if has_exe_suffix { // The application name is a path to a `.exe` file. // Let `CreateProcessW` figure out if it exists or not. - return path::maybe_verbatim(Path::new(exe_path)); + return Ok(exe_path.into()); } let mut path = PathBuf::from(exe_path); // Append `.exe` if not already there. path = path::append_suffix(path, EXE_SUFFIX.as_ref()); - if let Some(path) = program_exists(&path) { + if program_exists(&path) { return Ok(path); } else { // It's ok to use `set_extension` here because the intent is to // remove the extension that was just added. path.set_extension(""); - return path::maybe_verbatim(&path); + return Ok(path); } } else { ensure_no_nuls(exe_path)?; @@ -419,7 +415,7 @@ fn resolve_exe<'a>( if !has_extension { path.set_extension(EXE_EXTENSION); } - program_exists(&path) + if program_exists(&path) { Some(path) } else { None } }); if let Some(path) = result { return Ok(path); @@ -435,10 +431,10 @@ fn search_paths( parent_paths: Paths, child_paths: Option<&OsStr>, mut exists: Exists, -) -> Option> +) -> Option where Paths: FnOnce() -> Option, - Exists: FnMut(PathBuf) -> Option>, + Exists: FnMut(PathBuf) -> Option, { // 1. Child paths // This is for consistency with Rust's historic behaviour. @@ -490,18 +486,17 @@ where } /// Check if a file exists without following symlinks. -fn program_exists(path: &Path) -> Option> { +fn program_exists(path: &Path) -> bool { unsafe { - let path = path::maybe_verbatim(path).ok()?; - // Getting attributes using `GetFileAttributesW` does not follow symlinks - // and it will almost always be successful if the link exists. - // There are some exceptions for special system files (e.g. the pagefile) - // but these are not executable. - if c::GetFileAttributesW(path.as_ptr()) == c::INVALID_FILE_ATTRIBUTES { - None - } else { - Some(path) - } + to_u16s(path) + .map(|path| { + // Getting attributes using `GetFileAttributesW` does not follow symlinks + // and it will almost always be successful if the link exists. + // There are some exceptions for special system files (e.g. the pagefile) + // but these are not executable. + c::GetFileAttributesW(path.as_ptr()) != c::INVALID_FILE_ATTRIBUTES + }) + .unwrap_or(false) } } @@ -735,12 +730,7 @@ enum Quote { // Produces a wide string *without terminating null*; returns an error if // `prog` or any of the `args` contain a nul. -fn make_command_line( - prog: &[u16], - args: &[Arg], - force_quotes: bool, - is_batch_file: bool, -) -> io::Result> { +fn make_command_line(prog: &OsStr, args: &[Arg], force_quotes: bool) -> io::Result> { // Encode the command and arguments in a command line string such // that the spawned process may recover them using CommandLineToArgvW. let mut cmd: Vec = Vec::new(); @@ -749,18 +739,17 @@ fn make_command_line( // need to add an extra pair of quotes surrounding the whole command line // so they are properly passed on to the script. // See issue #91991. + let is_batch_file = Path::new(prog) + .extension() + .map(|ext| ext.eq_ignore_ascii_case("cmd") || ext.eq_ignore_ascii_case("bat")) + .unwrap_or(false); if is_batch_file { cmd.push(b'"' as u16); } - // Always quote the program name so CreateProcess to avoid ambiguity when - // the child process parses its arguments. - // Note that quotes aren't escaped here because they can't be used in arg0. - // But that's ok because file paths can't contain quotes. - cmd.push(b'"' as u16); - cmd.extend_from_slice(prog.strip_suffix(&[0]).unwrap_or(prog)); - cmd.push(b'"' as u16); - + // Always quote the program name so CreateProcess doesn't interpret args as + // part of the name if the binary wasn't found first time. + append_arg(&mut cmd, prog, Quote::Always)?; for arg in args { cmd.push(' ' as u16); let (arg, quote) = match arg { diff --git a/library/std/src/sys/windows/process/tests.rs b/library/std/src/sys/windows/process/tests.rs index cd535afb4a338..be3a0f4ed52a9 100644 --- a/library/std/src/sys/windows/process/tests.rs +++ b/library/std/src/sys/windows/process/tests.rs @@ -3,12 +3,11 @@ use super::Arg; use crate::env; use crate::ffi::{OsStr, OsString}; use crate::process::Command; -use crate::sys::to_u16s; #[test] fn test_raw_args() { let command_line = &make_command_line( - &to_u16s("quoted exe").unwrap(), + OsStr::new("quoted exe"), &[ Arg::Regular(OsString::from("quote me")), Arg::Raw(OsString::from("quote me *not*")), @@ -17,7 +16,6 @@ fn test_raw_args() { Arg::Regular(OsString::from("optional-quotes")), ], false, - false, ) .unwrap(); assert_eq!( @@ -30,10 +28,9 @@ fn test_raw_args() { fn test_make_command_line() { fn test_wrapper(prog: &str, args: &[&str], force_quotes: bool) -> String { let command_line = &make_command_line( - &to_u16s(prog).unwrap(), + OsStr::new(prog), &args.iter().map(|a| Arg::Regular(OsString::from(a))).collect::>(), force_quotes, - false, ) .unwrap(); String::from_utf16(command_line).unwrap()