From 145e6a94d6bd9aa360fa7d43e52a2e9aad1752ef Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 10 Feb 2023 16:59:04 +0100 Subject: [PATCH 1/6] Add suspicious_command_arg_space lint. --- clippy_lints/src/methods/mod.rs | 30 ++++++++++++++ .../methods/suspicious_command_arg_space.rs | 39 +++++++++++++++++++ clippy_utils/src/paths.rs | 1 + 3 files changed, 70 insertions(+) create mode 100644 clippy_lints/src/methods/suspicious_command_arg_space.rs diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index f1e8be7f2..c97f4661c 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -80,6 +80,7 @@ mod skip_while_next; mod stable_sort_primitive; mod str_splitn; mod string_extend_chars; +mod suspicious_command_arg_space; mod suspicious_map; mod suspicious_splitn; mod suspicious_to_owned; @@ -3162,6 +3163,32 @@ declare_clippy_lint! { "collecting an iterator when collect is not needed" } +declare_clippy_lint! { + /// ### What it does + /// + /// Checks for `Command::arg()` invocations that look like they + /// should be multiple arguments instead, such as `arg("-t ext2")`. + /// + /// ### Why is this bad? + /// + /// Arguments are not split by space. An argument like `arg("-t ext2")` + /// will be passed as a single argument to the command, + /// which is likely not what was intended. + /// + /// ### Example + /// ```rust + /// std::process::Command::new("echo").arg("-n hello").spawn().unwrap(); + /// ``` + /// Use instead: + /// ```rust + /// std::process::Command::new("echo").args(["-n", "hello"]).spawn().unwrap(); + /// ``` + #[clippy::version = "1.67.0"] + pub SUSPICIOUS_COMMAND_ARG_SPACE, + suspicious, + "single command line argument that looks like it should be multiple arguments" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -3496,6 +3523,9 @@ impl Methods { unnecessary_lazy_eval::check(cx, expr, recv, arg, "and"); } }, + ("arg", [arg]) => { + suspicious_command_arg_space::check(cx, recv, arg, span); + } ("as_deref" | "as_deref_mut", []) => { needless_option_as_deref::check(cx, expr, recv, name); }, diff --git a/clippy_lints/src/methods/suspicious_command_arg_space.rs b/clippy_lints/src/methods/suspicious_command_arg_space.rs new file mode 100644 index 000000000..73632c5a3 --- /dev/null +++ b/clippy_lints/src/methods/suspicious_command_arg_space.rs @@ -0,0 +1,39 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::paths; +use clippy_utils::ty::match_type; +use rustc_ast as ast; +use rustc_errors::{Applicability, Diagnostic}; +use rustc_hir as hir; +use rustc_lint::LateContext; +use rustc_span::Span; + +use super::SUSPICIOUS_COMMAND_ARG_SPACE; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, recv: &'tcx hir::Expr<'_>, arg: &'tcx hir::Expr<'_>, span: Span) { + let ty = cx.typeck_results().expr_ty(recv).peel_refs(); + + if match_type(cx, ty, &paths::STD_PROCESS_COMMAND) + && let hir::ExprKind::Lit(lit) = &arg.kind + && let ast::LitKind::Str(s, _) = &lit.node + && let Some((arg1, arg2)) = s.as_str().split_once(' ') + && arg1.starts_with('-') + && arg1.chars().all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') + { + span_lint_and_then( + cx, + SUSPICIOUS_COMMAND_ARG_SPACE, + arg.span, + "single argument that looks like it should be multiple arguments", + |diag: &mut Diagnostic| { + diag.multipart_suggestion_verbose( + "consider splitting the argument", + vec![ + (span, "args".to_string()), + (arg.span, format!("[{arg1:?}, {arg2:?}]")), + ], + Applicability::MaybeIncorrect, + ); + } + ); + } +} diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 95eebab75..4aae0f728 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -115,6 +115,7 @@ pub const STD_FS_CREATE_DIR: [&str; 3] = ["std", "fs", "create_dir"]; pub const STD_IO_SEEK: [&str; 3] = ["std", "io", "Seek"]; pub const STD_IO_SEEK_FROM_CURRENT: [&str; 4] = ["std", "io", "SeekFrom", "Current"]; pub const STD_IO_SEEKFROM_START: [&str; 4] = ["std", "io", "SeekFrom", "Start"]; +pub const STD_PROCESS_COMMAND: [&str; 3] = ["std", "process", "Command"]; pub const STRING_AS_MUT_STR: [&str; 4] = ["alloc", "string", "String", "as_mut_str"]; pub const STRING_AS_STR: [&str; 4] = ["alloc", "string", "String", "as_str"]; pub const STRING_NEW: [&str; 4] = ["alloc", "string", "String", "new"]; From 5fefe8b31738b4adf8329222afa50a264ef97d6a Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 10 Feb 2023 17:13:30 +0100 Subject: [PATCH 2/6] Add test. --- tests/ui/suspicious_command_arg_space.rs | 5 ++++ tests/ui/suspicious_command_arg_space.stderr | 25 ++++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 tests/ui/suspicious_command_arg_space.rs create mode 100644 tests/ui/suspicious_command_arg_space.stderr diff --git a/tests/ui/suspicious_command_arg_space.rs b/tests/ui/suspicious_command_arg_space.rs new file mode 100644 index 000000000..74b230f43 --- /dev/null +++ b/tests/ui/suspicious_command_arg_space.rs @@ -0,0 +1,5 @@ +fn main() { + std::process::Command::new("echo").arg("hello world").spawn().unwrap(); + std::process::Command::new("echo").arg("-n hello").spawn().unwrap(); + std::process::Command::new("cat").arg("--number file").spawn().unwrap(); +} diff --git a/tests/ui/suspicious_command_arg_space.stderr b/tests/ui/suspicious_command_arg_space.stderr new file mode 100644 index 000000000..9bc0ca93a --- /dev/null +++ b/tests/ui/suspicious_command_arg_space.stderr @@ -0,0 +1,25 @@ +error: single argument that looks like it should be multiple arguments + --> $DIR/suspicious_command_arg_space.rs:3:44 + | +LL | std::process::Command::new("echo").arg("-n hello").spawn().unwrap(); + | ^^^^^^^^^^ + | + = note: `-D clippy::suspicious-command-arg-space` implied by `-D warnings` +help: consider splitting the argument + | +LL | std::process::Command::new("echo").args(["-n", "hello"]).spawn().unwrap(); + | ~~~~ ~~~~~~~~~~~~~~~ + +error: single argument that looks like it should be multiple arguments + --> $DIR/suspicious_command_arg_space.rs:4:43 + | +LL | std::process::Command::new("cat").arg("--number file").spawn().unwrap(); + | ^^^^^^^^^^^^^^^ + | +help: consider splitting the argument + | +LL | std::process::Command::new("cat").args(["--number", "file"]).spawn().unwrap(); + | ~~~~ ~~~~~~~~~~~~~~~~~~~~ + +error: aborting due to 2 previous errors + From 984c47b9f4e0341bf4d3ed29007f4a51bf8a146d Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 10 Feb 2023 18:59:37 +0100 Subject: [PATCH 3/6] Clarify description of suspicious_command_arg_space. Co-authored-by: Manish Goregaokar --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c97f4661c..7e34d08ea 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3171,7 +3171,7 @@ declare_clippy_lint! { /// /// ### Why is this bad? /// - /// Arguments are not split by space. An argument like `arg("-t ext2")` + /// `Command::arg()` does not split arguments by space. An argument like `arg("-t ext2")` /// will be passed as a single argument to the command, /// which is likely not what was intended. /// From 8f56767c94a9ccadcd5cb6c495fe7f646e5221a7 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 10 Feb 2023 19:03:20 +0100 Subject: [PATCH 4/6] Update lints. --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 659e8aebc..30727beb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4764,6 +4764,7 @@ Released 2018-09-13 [`suboptimal_flops`]: https://rust-lang.github.io/rust-clippy/master/index.html#suboptimal_flops [`suspicious_arithmetic_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_arithmetic_impl [`suspicious_assignment_formatting`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_assignment_formatting +[`suspicious_command_arg_space`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_command_arg_space [`suspicious_else_formatting`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_else_formatting [`suspicious_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_map [`suspicious_op_assign_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_op_assign_impl diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 457a25826..c1feabf05 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -378,6 +378,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::methods::SKIP_WHILE_NEXT_INFO, crate::methods::STABLE_SORT_PRIMITIVE_INFO, crate::methods::STRING_EXTEND_CHARS_INFO, + crate::methods::SUSPICIOUS_COMMAND_ARG_SPACE_INFO, crate::methods::SUSPICIOUS_MAP_INFO, crate::methods::SUSPICIOUS_SPLITN_INFO, crate::methods::SUSPICIOUS_TO_OWNED_INFO, From 805a0ae2df4d85ec88497e32adcd1bb24a8729d1 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 10 Feb 2023 22:35:23 +0100 Subject: [PATCH 5/6] Add more test cases for suspicious_command_arg_space. --- tests/ui/suspicious_command_arg_space.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/ui/suspicious_command_arg_space.rs b/tests/ui/suspicious_command_arg_space.rs index 74b230f43..bdc6113a2 100644 --- a/tests/ui/suspicious_command_arg_space.rs +++ b/tests/ui/suspicious_command_arg_space.rs @@ -1,5 +1,10 @@ fn main() { - std::process::Command::new("echo").arg("hello world").spawn().unwrap(); + // Things it should warn about: std::process::Command::new("echo").arg("-n hello").spawn().unwrap(); std::process::Command::new("cat").arg("--number file").spawn().unwrap(); + + // Things it should not warn about: + std::process::Command::new("echo").arg("hello world").spawn().unwrap(); + std::process::Command::new("a").arg("--fmt=%a %b %c").spawn().unwrap(); + std::process::Command::new("b").arg("-ldflags=-s -w").spawn().unwrap(); } From 1f77866991568557e69e2135369ab2a512052a6b Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 12 Feb 2023 22:00:13 +0100 Subject: [PATCH 6/6] Add SUSPICIOUS_COMMAND_ARG_SPACE to lint pass. --- clippy_lints/src/methods/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7e34d08ea..d8c018f75 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3316,6 +3316,7 @@ impl_lint_pass!(Methods => [ SEEK_FROM_CURRENT, SEEK_TO_START_INSTEAD_OF_REWIND, NEEDLESS_COLLECT, + SUSPICIOUS_COMMAND_ARG_SPACE, ]); /// Extracts a method call name, args, and `Span` of the method name.