From d83fe8d83c8c8563490dcf93c079b7204bc35a2c Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Sat, 2 Nov 2024 03:52:26 +0100 Subject: [PATCH] unix: Relax escaping in Debug impl on Command The Debug output of Command is very useful for showing to the user the command that was executed when something went wrong. This is done for example by rustc when invoking an external tool like the linker fails. It is also overly verbose, since everything is quoted, which makes it harder to read. Instead, we now first check if we're reasonably sure that an argument is simple enough that using it in the shell wouldn't need quoting, and then output it without quotes if possible. Before and example output could look like this: PATH="/a:/b" "cc" "foo.o" "-target" "arm64-apple-darwin11.0" Now it looks like this: PATH=/a:/b cc foo.o -target arm64-apple-darwin11.0 --- library/std/src/lib.rs | 1 + library/std/src/process/tests.rs | 18 ++--- .../sys/pal/unix/process/process_common.rs | 66 +++++++++++++++++-- .../pal/unix/process/process_common/tests.rs | 35 ++++++++++ tests/run-make/link-args-order/rmake.rs | 14 ++-- tests/run-make/link-dedup/rmake.rs | 4 +- .../pass-linker-flags-flavor/rmake.rs | 4 +- 7 files changed, 114 insertions(+), 28 deletions(-) diff --git a/library/std/src/lib.rs b/library/std/src/lib.rs index ee6fceb024fd7..937a1973c1f49 100644 --- a/library/std/src/lib.rs +++ b/library/std/src/lib.rs @@ -285,6 +285,7 @@ #![feature(cfg_target_thread_local)] #![feature(cfi_encoding)] #![feature(concat_idents)] +#![feature(debug_closure_helpers)] #![feature(decl_macro)] #![feature(deprecated_suggestion)] #![feature(doc_cfg)] diff --git a/library/std/src/process/tests.rs b/library/std/src/process/tests.rs index fb0b495961c36..308384920d92f 100644 --- a/library/std/src/process/tests.rs +++ b/library/std/src/process/tests.rs @@ -552,7 +552,7 @@ fn debug_print() { let mut command = Command::new("some-boring-name"); - assert_eq!(format!("{command:?}"), format!(r#""some-boring-name""#)); + assert_eq!(format!("{command:?}"), format!("some-boring-name")); assert_eq!( format!("{command:#?}"), @@ -568,7 +568,7 @@ fn debug_print() { command.args(&["1", "2", "3"]); - assert_eq!(format!("{command:?}"), format!(r#""some-boring-name" "1" "2" "3""#)); + assert_eq!(format!("{command:?}"), format!("some-boring-name 1 2 3")); assert_eq!( format!("{command:#?}"), @@ -587,10 +587,7 @@ fn debug_print() { crate::os::unix::process::CommandExt::arg0(&mut command, "exciting-name"); - assert_eq!( - format!("{command:?}"), - format!(r#"["some-boring-name"] "exciting-name" "1" "2" "3""#) - ); + assert_eq!(format!("{command:?}"), format!("[some-boring-name] exciting-name 1 2 3")); assert_eq!( format!("{command:#?}"), @@ -609,10 +606,7 @@ fn debug_print() { let mut command_with_env_and_cwd = Command::new("boring-name"); command_with_env_and_cwd.current_dir("/some/path").env("FOO", "bar"); - assert_eq!( - format!("{command_with_env_and_cwd:?}"), - r#"cd "/some/path" && FOO="bar" "boring-name""# - ); + assert_eq!(format!("{command_with_env_and_cwd:?}"), "cd /some/path && FOO=bar boring-name"); assert_eq!( format!("{command_with_env_and_cwd:#?}"), format!( @@ -638,7 +632,7 @@ fn debug_print() { let mut command_with_removed_env = Command::new("boring-name"); command_with_removed_env.env_remove("FOO").env_remove("BAR"); - assert_eq!(format!("{command_with_removed_env:?}"), r#"env -u BAR -u FOO "boring-name""#); + assert_eq!(format!("{command_with_removed_env:?}"), "env -u BAR -u FOO boring-name"); assert_eq!( format!("{command_with_removed_env:#?}"), format!( @@ -660,7 +654,7 @@ fn debug_print() { let mut command_with_cleared_env = Command::new("boring-name"); command_with_cleared_env.env_clear().env("BAR", "val").env_remove("FOO"); - assert_eq!(format!("{command_with_cleared_env:?}"), r#"env -i BAR="val" "boring-name""#); + assert_eq!(format!("{command_with_cleared_env:?}"), "env -i BAR=val boring-name"); assert_eq!( format!("{command_with_cleared_env:#?}"), format!( diff --git a/library/std/src/sys/pal/unix/process/process_common.rs b/library/std/src/sys/pal/unix/process/process_common.rs index 13290fed762ae..b8619b896992d 100644 --- a/library/std/src/sys/pal/unix/process/process_common.rs +++ b/library/std/src/sys/pal/unix/process/process_common.rs @@ -580,8 +580,56 @@ impl fmt::Debug for Command { debug_command.finish() } else { + /// Only escape arguments when necessary, otherwise output it + /// unescaped to make the output easier to read. + /// + /// NOTE: This is best effort only! + fn relaxed_escaping(bytes: &[u8], is_arg: bool) -> impl fmt::Display + use<'_> { + // Don't escape if all the characters are likely + // to not be a problem when copied to the shell. + let can_print_safely = move |&c| { + // See: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_02 + match c { + // The documentation linked above says that `=`: + // > may need to be quoted under certain circumstances + // + // Because they may be parsed as a variable assignment. + // But in argument position to a command, they + // shouldn't need escaping. + b'=' if is_arg => true, + // As per documentation linked above: + // > The application shall quote the following characters + b'|' | b'&' | b';' | b'<' | b'>' | b'(' | b')' | b'$' | b'`' | b'\\' + | b'"' | b'\'' | b' ' | b'\t' | b'\n' => false, + // As per documentation linked above: + // > may need to be quoted under certain circumstances + b'*' | b'?' | b'[' | b'#' | b'~' | b'=' | b'%' => false, + // It'd be weird to quote `[` and not quote `]`. + b']' => false, + // ! does history expansion in Bash, require quoting for that as well. + // + // NOTE: This doesn't currently work properly, we'd need to backslash-escape + // `!` as well (which `escape_ascii` doesn't do). + b'!' => false, + // Assume all other printable characters may be output. + 0x20..0x7e => true, + // Unprintable or not ASCII. + _ => false, + } + }; + fmt::from_fn(move |f| { + if !bytes.is_empty() && bytes.iter().all(can_print_safely) { + // Unwrap is fine, we've checked above that the bytes only contains ascii. + let ascii = crate::str::from_utf8(bytes).unwrap(); + write!(f, "{ascii}") + } else { + write!(f, "\"{}\"", bytes.escape_ascii()) + } + }) + } + if let Some(ref cwd) = self.cwd { - write!(f, "cd {cwd:?} && ")?; + write!(f, "cd {} && ", relaxed_escaping(cwd.to_bytes(), true))?; } if self.env.does_clear() { write!(f, "env -i ")?; @@ -595,23 +643,29 @@ impl fmt::Debug for Command { write!(f, "env ")?; any_removed = true; } - write!(f, "-u {} ", key.to_string_lossy())?; + write!(f, "-u {} ", relaxed_escaping(key.as_encoded_bytes(), false))?; } } } // Altered env vars can just be added in front of the program. for (key, value_opt) in self.get_envs() { if let Some(value) = value_opt { - write!(f, "{}={value:?} ", key.to_string_lossy())?; + write!( + f, + "{}={} ", + relaxed_escaping(key.as_encoded_bytes(), false), + relaxed_escaping(value.as_encoded_bytes(), false) + )?; } } if self.program != self.args[0] { - write!(f, "[{:?}] ", self.program)?; + write!(f, "[{}] ", relaxed_escaping(self.program.to_bytes(), false))?; } - write!(f, "{:?}", self.args[0])?; + + write!(f, "{}", relaxed_escaping(self.args[0].to_bytes(), false))?; for arg in &self.args[1..] { - write!(f, " {:?}", arg)?; + write!(f, " {}", relaxed_escaping(arg.to_bytes(), true))?; } Ok(()) } diff --git a/library/std/src/sys/pal/unix/process/process_common/tests.rs b/library/std/src/sys/pal/unix/process/process_common/tests.rs index e5c8dd6e341e1..86b542bac931d 100644 --- a/library/std/src/sys/pal/unix/process/process_common/tests.rs +++ b/library/std/src/sys/pal/unix/process/process_common/tests.rs @@ -190,3 +190,38 @@ fn unix_exit_statuses() { } } } + +#[test] +fn command_debug_escaping() { + #[track_caller] + fn check(arg: impl AsRef, expected: &str) { + let cmd = Command::new(arg.as_ref()); + assert_eq!(format!("{cmd:?}"), expected); + } + + // Escaped. + check("", r#""""#); + check("é", r#""\xc3\xa9""#); + check("a'", r#""a\'""#); + check("a\"", r#""a\"""#); + check("'\"", r#""\'\"""#); + check(" ", r#"" ""#); + check( + "/Users/The user's name/.cargo/bin/cargo", + r#""/Users/The user\'s name/.cargo/bin/cargo""#, + ); + check("!a", r#""!a""#); + + // Simple enough that we don't escape them. + check("a", "a"); + check("/my/simple/path", "/my/simple/path"); + check("abc-defg_1234", "abc-defg_1234"); + + // Real-world use-case, immediately clear that two of the arguments are passed as one. + let mut cmd = crate::process::Command::new("ld"); + cmd.arg("my/file.o"); + cmd.arg("-macos_version_min"); + cmd.arg("13.0"); + cmd.arg("-arch arm64"); + assert_eq!(format!("{cmd:?}"), r#"ld my/file.o -macos_version_min 13.0 "-arch arm64""#); +} diff --git a/tests/run-make/link-args-order/rmake.rs b/tests/run-make/link-args-order/rmake.rs index b7ef8333267f2..8075dd23e6068 100644 --- a/tests/run-make/link-args-order/rmake.rs +++ b/tests/run-make/link-args-order/rmake.rs @@ -13,17 +13,19 @@ fn main() { .linker_flavor(linker) .link_arg("a") .link_args("b c") - .link_args("d e") - .link_arg("f") + .link_arg("d e") + .link_args("f g") + .link_arg("h") .run_fail() - .assert_stderr_contains(r#""a" "b" "c" "d" "e" "f""#); + .assert_stderr_contains(r#"a b c "d e" f g h"#); rustc() .input("empty.rs") .linker_flavor(linker) .arg("-Zpre-link-arg=a") .arg("-Zpre-link-args=b c") - .arg("-Zpre-link-args=d e") - .arg("-Zpre-link-arg=f") + .arg("-Zpre-link-arg=d e") + .arg("-Zpre-link-args=f g") + .arg("-Zpre-link-arg=h") .run_fail() - .assert_stderr_contains(r#""a" "b" "c" "d" "e" "f""#); + .assert_stderr_contains(r#"a b c "d e" f g h"#); } diff --git a/tests/run-make/link-dedup/rmake.rs b/tests/run-make/link-dedup/rmake.rs index 6075f31095424..0075057b340ca 100644 --- a/tests/run-make/link-dedup/rmake.rs +++ b/tests/run-make/link-dedup/rmake.rs @@ -31,9 +31,9 @@ fn needle_from_libs(libs: &[&str]) -> String { let mut needle = String::new(); for lib in libs { if is_msvc() { - let _ = needle.write_fmt(format_args!(r#""{lib}.lib" "#)); + let _ = needle.write_fmt(format_args!("{lib}.lib ")); } else { - let _ = needle.write_fmt(format_args!(r#""-l{lib}" "#)); + let _ = needle.write_fmt(format_args!("-l{lib} ")); } } needle.pop(); // remove trailing space diff --git a/tests/run-make/pass-linker-flags-flavor/rmake.rs b/tests/run-make/pass-linker-flags-flavor/rmake.rs index e36d68cc88432..6b2ee63b28cb1 100644 --- a/tests/run-make/pass-linker-flags-flavor/rmake.rs +++ b/tests/run-make/pass-linker-flags-flavor/rmake.rs @@ -72,8 +72,8 @@ fn main() { .stdout_utf8(); let no_verbatim = regex::Regex::new("l1.*-Wl,a1.*l2.*-Wl,a2.*d1.*-Wl,a3").unwrap(); - let one_verbatim = regex::Regex::new(r#"l1.*"a1".*l2.*-Wl,a2.*d1.*-Wl,a3"#).unwrap(); - let ld = regex::Regex::new(r#"l1.*"a1".*l2.*"a2".*d1.*"a3""#).unwrap(); + let one_verbatim = regex::Regex::new("l1.*a1.*l2.*-Wl,a2.*d1.*-Wl,a3").unwrap(); + let ld = regex::Regex::new("l1.*a1.*l2.*a2.*d1.*a3").unwrap(); assert!(no_verbatim.is_match(&out_gnu)); assert!(no_verbatim.is_match(&out_att_gnu));