From 21d4080183f250d0e086a233302aa1a8205a0c13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fe=CC=81lix=20Saparelli?= Date: Wed, 22 Aug 2018 00:13:45 +1200 Subject: [PATCH 1/2] Wrap whitespace in arguments Fixes #82 Fixes #87 --- src/cli.rs | 6 ++-- src/process.rs | 80 +++++++++++++++++++++++++++++++++++--------------- src/run.rs | 5 ++-- 3 files changed, 60 insertions(+), 31 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 918cf3e..941db11 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -5,7 +5,7 @@ use clap::{App, Arg, Error}; #[derive(Debug)] pub struct Args { - pub cmd: String, + pub cmd: Vec, pub paths: Vec, pub filters: Vec, pub ignores: Vec, @@ -120,9 +120,7 @@ pub fn get_args() -> Args { .arg(Arg::with_name("once").short("1").hidden(true)) .get_matches(); - let cmd = values_t!(args.values_of("command"), String) - .unwrap() - .join(" "); + let cmd: Vec = values_t!(args.values_of("command"), String).unwrap(); let paths = values_t!(args.values_of("path"), String).unwrap_or(vec![String::from(".")]); // Treat --kill as --signal SIGKILL (for compatibility with older syntax) diff --git a/src/process.rs b/src/process.rs index 14abdb4..3096f3e 100644 --- a/src/process.rs +++ b/src/process.rs @@ -2,12 +2,48 @@ use std::path::PathBuf; use std::collections::{HashMap, HashSet}; use pathop::PathOp; -pub fn spawn(cmd: &str, updated_paths: Vec, no_shell: bool) -> Process { +pub fn spawn(cmd: &Vec, updated_paths: Vec, no_shell: bool) -> Process { self::imp::Process::new(cmd, updated_paths, no_shell).expect("unable to spawn process") } pub use self::imp::Process; +fn has_whitespace(s: &String) -> bool { + s.contains(|ch| match ch { + ' ' => true, + '\t' => true, + _ => false + }) +} + +#[cfg(target_family = "unix")] +fn wrap_in_quotes(s: &String) -> String { + format!("'{}'", if s.contains('\'') { + s.replace('\'', "'\"'\"'") + } else { + s.clone() + }) +} + +#[cfg(target_family = "windows")] +fn wrap_in_quotes(s: &String) -> String { + format!("\"{}\"", if s.contains('"') { + s.replace('"', "\"\"") + } else { + s.clone() + }) +} + +fn wrap_commands(cmd: &Vec) -> Vec { + cmd.iter().map(|fragment| { + if has_whitespace(fragment) { + wrap_in_quotes(fragment) + } else { + fragment.clone() + } + }).collect() +} + #[cfg(target_family = "unix")] mod imp { use nix::{self, Error}; @@ -15,6 +51,7 @@ mod imp { use std::io::{self, Result}; use std::process::Command; use std::sync::*; + use super::wrap_commands; use signal::Signal; use pathop::PathOp; @@ -35,7 +72,7 @@ mod imp { #[allow(unknown_lints)] #[allow(mutex_atomic)] impl Process { - pub fn new(cmd: &str, updated_paths: Vec, no_shell: bool) -> Result { + pub fn new(cmd: &Vec, updated_paths: Vec, no_shell: bool) -> Result { use nix::unistd::*; use std::os::unix::process::CommandExt; @@ -45,13 +82,13 @@ mod imp { // but is a little less performant and can cause trouble when using custom signals // (e.g. --signal SIGHUP) let mut command = if no_shell { - let mut split = cmd.split_whitespace(); - let mut command = Command::new(split.next().unwrap()); - command.args(split); + let (head, tail) = cmd.split_first().unwrap(); + let mut command = Command::new(head); + command.args(tail); command } else { let mut command = Command::new("sh"); - command.arg("-c").arg(cmd); + command.arg("-c").arg(wrap_commands(cmd).join(" ")); command }; @@ -134,6 +171,7 @@ mod imp { use std::mem; use std::process::Command; use std::ptr; + use super::wrap_commands; use kernel32::*; use winapi::*; use signal::Signal; @@ -151,7 +189,7 @@ mod imp { } impl Process { - pub fn new(cmd: &str, updated_paths: Vec, no_shell: bool) -> Result { + pub fn new(cmd: &Vec, updated_paths: Vec, no_shell: bool) -> Result { use std::os::windows::io::IntoRawHandle; use std::os::windows::process::CommandExt; @@ -194,23 +232,17 @@ mod imp { panic!("failed to set job info: {}", last_err()); } + let mut command; + if no_shell { + let (arg0, args) = cmd.split_first().unwrap(); + command = Command::new(arg0); + command.args(args); + } else { + command = Command::new("cmd.exe"); + command.arg("/C"); + command.arg(wrap_commands(cmd).join(" ")); + } - let mut iter_args = cmd.split_whitespace(); - let arg0 = match no_shell { - true => iter_args.next().unwrap(), - false => "cmd.exe", - }; - - // TODO: There might be a better way of doing this with &str. - // I've had to fall back to String, as I wasn't able to join(" ") a Vec<&str> - // into a &str - let args: Vec = match no_shell { - true => iter_args.map(str::to_string).collect(), - false => vec!["/C".to_string(), iter_args.collect::>().join(" ")], - }; - - let mut command = Command::new(arg0); - command.args(args); command.creation_flags(CREATE_SUSPENDED); debug!("Assembled command {:?}", command); @@ -397,7 +429,7 @@ mod tests { #[test] fn test_start() { - let _ = spawn("echo hi", vec![], true); + let _ = spawn(&vec!["echo".into(), "hi".into()], vec![], true); } #[test] diff --git a/src/run.rs b/src/run.rs index 18790a0..bb8ee5b 100644 --- a/src/run.rs +++ b/src/run.rs @@ -30,7 +30,7 @@ fn init_logger(debug: bool) { .init(); } -#[cfg(target_os="linux")] +#[cfg(target_os = "linux")] fn should_switch_to_poll(e: &Error) -> bool { use nix::libc; @@ -40,9 +40,8 @@ fn should_switch_to_poll(e: &Error) -> bool { } } -#[cfg(not(target_os="linux"))] +#[cfg(not(target_os = "linux"))] fn should_switch_to_poll(_: &Error) -> bool { - // not known conditions to switch false } From 7375db5ce9f2713a21cd1a1734fa830b51012f50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fe=CC=81lix=20Saparelli?= Date: Wed, 22 Aug 2018 08:17:36 +1200 Subject: [PATCH 2/2] =?UTF-8?q?Also=20wrap=20when=20there=E2=80=99s=20quot?= =?UTF-8?q?es=20but=20no=20whitespace?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/process.rs | 100 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 96 insertions(+), 4 deletions(-) diff --git a/src/process.rs b/src/process.rs index 3096f3e..36a9978 100644 --- a/src/process.rs +++ b/src/process.rs @@ -8,10 +8,9 @@ pub fn spawn(cmd: &Vec, updated_paths: Vec, no_shell: bool) -> P pub use self::imp::Process; -fn has_whitespace(s: &String) -> bool { +fn needs_wrapping(s: &String) -> bool { s.contains(|ch| match ch { - ' ' => true, - '\t' => true, + ' ' | '\t' | '\'' | '"' => true, _ => false }) } @@ -36,7 +35,7 @@ fn wrap_in_quotes(s: &String) -> String { fn wrap_commands(cmd: &Vec) -> Vec { cmd.iter().map(|fragment| { - if has_whitespace(fragment) { + if needs_wrapping(fragment) { wrap_in_quotes(fragment) } else { fragment.clone() @@ -426,12 +425,53 @@ mod tests { use super::spawn; use super::get_longest_common_path; use super::collect_path_env_vars; + use super::wrap_commands; #[test] fn test_start() { let _ = spawn(&vec!["echo".into(), "hi".into()], vec![], true); } + #[test] + fn wrap_commands_that_have_whitespace() { + assert_eq!( + wrap_commands(&vec!["echo".into(), "hello world".into()]), + vec!["echo".into(), "'hello world'".into()] as Vec + ); + } + + #[test] + fn wrap_commands_that_have_long_whitespace() { + assert_eq!( + wrap_commands(&vec!["echo".into(), "hello world".into()]), + vec!["echo".into(), "'hello world'".into()] as Vec + ); + } + + #[test] + fn wrap_commands_that_have_single_quotes() { + assert_eq!( + wrap_commands(&vec!["echo".into(), "hello ' world".into()]), + vec!["echo".into(), "'hello '\"'\"' world'".into()] as Vec + ); + assert_eq!( + wrap_commands(&vec!["echo".into(), "hello'world".into()]), + vec!["echo".into(), "'hello'\"'\"'world'".into()] as Vec + ); + } + + #[test] + fn wrap_commands_that_have_double_quotes() { + assert_eq!( + wrap_commands(&vec!["echo".into(), "hello \" world".into()]), + vec!["echo".into(), "'hello \" world'".into()] as Vec + ); + assert_eq!( + wrap_commands(&vec!["echo".into(), "hello\"world".into()]), + vec!["echo".into(), "'hello\"world'".into()] as Vec + ); + } + #[test] fn longest_common_path_should_return_correct_value() { let single_path = vec![PathBuf::from("/tmp/random/")]; @@ -477,3 +517,55 @@ mod tests { } } +#[cfg(test)] +#[cfg(target_family = "windows")] +mod tests { + use super::spawn; + use super::wrap_commands; + + #[test] + fn test_start() { + let _ = spawn(&vec!["echo".into(), "hi".into()], vec![], true); + } + + #[test] + fn wrap_commands_that_have_whitespace() { + assert_eq!( + wrap_commands(&vec!["echo".into(), "hello world".into()]), + vec!["echo".into(), "\"hello world\"".into()] as Vec + ); + } + + #[test] + fn wrap_commands_that_have_long_whitespace() { + assert_eq!( + wrap_commands(&vec!["echo".into(), "hello world".into()]), + vec!["echo".into(), "\"hello world\"".into()] as Vec + ); + } + + #[test] + fn wrap_commands_that_have_single_quotes() { + assert_eq!( + wrap_commands(&vec!["echo".into(), "hello ' world".into()]), + vec!["echo".into(), "\"hello ' world\"".into()] as Vec + ); + assert_eq!( + wrap_commands(&vec!["echo".into(), "hello'world".into()]), + vec!["echo".into(), "\"hello'world\"".into()] as Vec + ); + } + + #[test] + fn wrap_commands_that_have_double_quotes() { + assert_eq!( + wrap_commands(&vec!["echo".into(), "hello \" world".into()]), + vec!["echo".into(), "\"hello \"\" world\"".into()] as Vec + ); + assert_eq!( + wrap_commands(&vec!["echo".into(), "hello\"world".into()]), + vec!["echo".into(), "\"hello\"\"world\"".into()] as Vec + ); + } +} +