Skip to content

unix: Relax escaping in Debug impl on Command #132484

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+114 −28
Open

unix: Relax escaping in Debug impl on Command #132484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions library/std/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
18 changes: 6 additions & 12 deletions library/std/src/process/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:#?}"),
Expand All @@ -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:#?}"),
Expand All @@ -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:#?}"),
Expand All @@ -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!(
Expand All @@ -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!(
Expand All @@ -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!(
Expand Down
66 changes: 60 additions & 6 deletions library/std/src/sys/pal/unix/process/process_common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get a test for the quoting of ", please?

Also, seconding the premise that we need to quote !.

Since this is UNIX-specific, I think it'd be reasonable to always use single-quotes for quoting, and just handle ' specially.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get a test for the quoting of ", please?

There is a test for that? In library/std/src/sys/pal/unix/process/process_common/tests.rs:

check("a\"", r#""a\"""#);

Or perhaps I'm misunderstanding?

// 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 ")?;
Expand All @@ -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(())
}
Expand Down
35 changes: 35 additions & 0 deletions library/std/src/sys/pal/unix/process/process_common/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,3 +190,38 @@ fn unix_exit_statuses() {
}
}
}

#[test]
fn command_debug_escaping() {
#[track_caller]
fn check(arg: impl AsRef<OsStr>, 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""#);
}
14 changes: 8 additions & 6 deletions tests/run-make/link-args-order/rmake.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"#);
}
4 changes: 2 additions & 2 deletions tests/run-make/link-dedup/rmake.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions tests/run-make/pass-linker-flags-flavor/rmake.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
Loading