From f83c71fb0f2f96a2dd065a8d55f5f81e2aa1d1e0 Mon Sep 17 00:00:00 2001 From: ColumbusLabs <287001685+ColumbusLabs@users.noreply.github.com> Date: Thu, 23 Jul 2026 19:05:18 -0400 Subject: [PATCH 1/2] grep: expand bundled numeric context options --- src/lib.rs | 124 +++++++++++++++++++++++++++++++++++---------- tests/test_grep.rs | 120 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 216 insertions(+), 28 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index fbf25c3..1ac622d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -857,50 +857,118 @@ fn expand_num_shorthand(args: impl Iterator) -> Vec { b"group-separator", ]; - fn consumes_next_arg(arg: &[u8]) -> bool { - if let Some(name) = arg.strip_prefix(b"-") { - LONG_OPTS_WITH_VALUE.contains(&name) - } else { - arg.last() - .is_some_and(|b| SHORT_OPTS_WITH_VALUE.contains(b)) + fn short_option_consumes_next(cluster: &[u8]) -> bool { + cluster + .iter() + .position(|b| SHORT_OPTS_WITH_VALUE.contains(b)) + .is_some_and(|i| i + 1 == cluster.len()) + } + + fn has_context_digit(cluster: &[u8]) -> bool { + for b in cluster { + if SHORT_OPTS_WITH_VALUE.contains(b) { + return false; + } + if b.is_ascii_digit() { + return true; + } } + false } - let mut out: Vec = args.collect(); - let mut i = 1; // argv[0] is the executable name + let mut args = args; + let mut out = Vec::new(); + let Some(program) = args.next() else { + return out; + }; + out.push(program); + + let mut parsing_options = true; + let mut consumes_next = false; - while i < out.len() { - let arg = out[i].as_encoded_bytes(); + for arg in args { + if !parsing_options || consumes_next { + consumes_next = false; + out.push(arg); + continue; + } - // Narrow down to `-flags` (and `--options`) and strip the leading `-` for easier matching. - let Some(arg) = arg.strip_prefix(b"-") else { - // Not a flag, skip. - i += 1; + // Only ASCII short options can contain a numeric shorthand. Leaving any + // other platform-native argument intact also avoids making assumptions + // about the encoding of an OsString. + let Some(arg_str) = arg.to_str() else { + out.push(arg); continue; }; - // No more options after `--`. - if arg == b"-" { - break; + if arg_str == "--" { + parsing_options = false; + out.push(arg); + continue; } - // This flag consumes two args. Skip both. - if consumes_next_arg(arg) { - i += 2; + let Some(option) = arg_str.strip_prefix('-') else { + out.push(arg); + continue; + }; + + if option.is_empty() { + out.push(arg); continue; } - // Translate -NUM to -C NUM. - if !arg.is_empty() && arg.iter().all(u8::is_ascii_digit) { - // SAFETY: We know that all `arg` bytes are valid ASCII (digits). - let digits = unsafe { OsStr::from_encoded_bytes_unchecked(arg) }.to_owned(); - out[i] = OsString::from("-C"); - out.insert(i + 1, digits); - i += 2; + if let Some(long_option) = option.strip_prefix('-') { + consumes_next = LONG_OPTS_WITH_VALUE.contains(&long_option.as_bytes()); + out.push(arg); continue; } - i += 1; + let cluster = option.as_bytes(); + if !cluster.is_ascii() { + out.push(arg); + continue; + } + + // A standalone digit run is one context value (`-12` means `-C 12`). + if cluster.iter().all(u8::is_ascii_digit) { + out.push(OsString::from("-C")); + out.push(OsString::from(option)); + continue; + } + + // Digits after a value-taking option belong to that option, not to the + // context shorthand (for example, `-e123` and `-m2`). + if !has_context_digit(cluster) { + consumes_next = short_option_consumes_next(cluster); + out.push(arg); + continue; + } + + // A digit embedded in a short-option cluster acts as a single `-NUM` + // option at that point in the cluster. Emitting each option separately + // preserves left-to-right override semantics. + let mut i = 0; + while i < cluster.len() { + let option = cluster[i]; + if option.is_ascii_digit() { + out.push(OsString::from("-C")); + out.push(OsString::from(char::from(option).to_string())); + i += 1; + } else if SHORT_OPTS_WITH_VALUE.contains(&option) { + let mut value_option = String::from("-"); + value_option.push_str( + std::str::from_utf8(&cluster[i..]).expect("short option cluster is ASCII"), + ); + out.push(OsString::from(value_option)); + consumes_next = i + 1 == cluster.len(); + break; + } else { + let mut flag = String::from("-"); + flag.push(char::from(option)); + out.push(OsString::from(flag)); + i += 1; + } + } } out diff --git a/tests/test_grep.rs b/tests/test_grep.rs index e250165..90f65d8 100644 --- a/tests/test_grep.rs +++ b/tests/test_grep.rs @@ -931,6 +931,126 @@ fn num_shorthand_is_context() { .stdout_only("a\nb\nMATCH\nc\nd\n"); } +#[test] +fn num_shorthand_in_short_option_clusters() { + let input = "a\nb\nMATCH\nc\nd\n"; + + // A numeric context option can follow another short option in the same argument. + let (_s, mut c) = ucmd(); + c.args(&["-w2", "MATCH"]) + .pipe_in(input) + .succeeds() + .stdout_only(input); + + let (_s, mut c) = ucmd(); + c.args(&["-i2", "match"]) + .pipe_in(input) + .succeeds() + .stdout_only(input); + + // Preserve option order when a later option overrides the bundled context. + let (_s, mut c) = ucmd(); + c.args(&["-w2C1", "MATCH"]) + .pipe_in(input) + .succeeds() + .stdout_only("b\nMATCH\nc\n"); + + // Each embedded digit is a context option, so `-w44` finishes with four + // lines of context rather than treating the suffix as the number 44. + let repeated_input = "zero\none\ntwo\nthree\nfour\nMATCH\nsix\nseven\neight\nnine\nten\n"; + let (_s, mut c) = ucmd(); + c.args(&["-w44", "MATCH"]) + .pipe_in(repeated_input) + .succeeds() + .stdout_only("one\ntwo\nthree\nfour\nMATCH\nsix\nseven\neight\nnine\n"); + + // Regression for the original fuzzer case: trailing flags in the same + // cluster are still parsed after the numeric options. + let (_s, mut c) = ucmd(); + c.args(&["-e", "a", "-w44ov"]) + .pipe_in("a\nb\n") + .succeeds() + .stdout_only(""); +} + +#[test] +fn num_shorthand_does_not_capture_option_values() { + // Digits attached to value-taking short options are their values. + let (_s, mut c) = ucmd(); + c.args(&["-e123"]) + .pipe_in("123\n456\n") + .succeeds() + .stdout_only("123\n"); + + let (_s, mut c) = ucmd(); + c.args(&["-m2", "x"]) + .pipe_in("x\nx\nx\n") + .succeeds() + .stdout_only("x\nx\n"); + + let (_s, mut c) = ucmd(); + c.args(&["-wm2", "x"]) + .pipe_in("x\nx\nx\n") + .succeeds() + .stdout_only("x\nx\n"); + + // The next argument is also left alone when the preceding option consumes it. + let (_s, mut c) = ucmd(); + c.args(&["-e", "-2"]) + .pipe_in("-2\nx\n") + .succeeds() + .stdout_only("-2\n"); + + let (_s, mut c) = ucmd(); + c.args(&["--regexp", "-2"]) + .pipe_in("-2\nx\n") + .succeeds() + .stdout_only("-2\n"); + + // This remains true after an earlier shorthand forced the cluster to be expanded. + let (_s, mut c) = ucmd(); + c.args(&["-i2e", "-3"]) + .pipe_in("a\n-3\nb\n") + .succeeds() + .stdout_only("a\n-3\nb\n"); +} + +#[test] +fn num_shorthand_only_changes_short_options() { + let (_s, mut c) = ucmd(); + c.args(&["--regexp=-2"]) + .pipe_in("-2\nx\n") + .succeeds() + .stdout_only("-2\n"); + + // `--` ends option parsing, so the following `-2` is the positional pattern. + let (_s, mut c) = ucmd(); + c.args(&["--", "-2"]) + .pipe_in("-2\nx\n") + .succeeds() + .stdout_only("-2\n"); + + // Digits in an ordinary positional pattern are not option shorthand. + let (_s, mut c) = ucmd(); + c.args(&["w2"]) + .pipe_in("w2\nw3\n") + .succeeds() + .stdout_only("w2\n"); +} + +#[cfg(unix)] +#[test] +fn num_shorthand_preserves_invalid_utf8_arguments() { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + // Platform-native arguments that are not UTF-8 are passed through to clap + // unchanged instead of being sliced or reconstructed by the preprocessor. + let invalid_cluster = OsString::from_vec(vec![b'-', b'w', b'2', 0xff]); + let (_s, mut c) = ucmd(); + c.arg(invalid_cluster).arg("x").pipe_in("x\n").fails(); +} + #[test] fn context_line_prefixes_use_dash() { // Match line uses `:`, context lines use `-`. From 56b9aab52c98154e2ec81efcb8119b4ba12c3f98 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 27 Jul 2026 17:43:52 +0200 Subject: [PATCH 2/2] Simplify and fix expand_num_shorthand --- src/lib.rs | 150 ++++++++++++++++++--------------------------- tests/test_grep.rs | 146 ++++++++++++++++++------------------------- 2 files changed, 120 insertions(+), 176 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 1ac622d..830a5e4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -837,6 +837,8 @@ pub fn uu_app() -> Command { } /// Expand GNU grep's `-NUM` shorthand to `-C NUM`. +/// +/// Note that `-a12b` means `-a -C 12 -b`. fn expand_num_shorthand(args: impl Iterator) -> Vec { const SHORT_OPTS_WITH_VALUE: &[u8] = b"efmABCDd"; const LONG_OPTS_WITH_VALUE: &[&[u8]] = &[ @@ -857,118 +859,84 @@ fn expand_num_shorthand(args: impl Iterator) -> Vec { b"group-separator", ]; - fn short_option_consumes_next(cluster: &[u8]) -> bool { - cluster - .iter() - .position(|b| SHORT_OPTS_WITH_VALUE.contains(b)) - .is_some_and(|i| i + 1 == cluster.len()) + // Rebuild a single option argument out of `prefix` and `bytes`. + fn option(prefix: &str, bytes: &[u8]) -> OsString { + let mut buf = Vec::with_capacity(prefix.len() + bytes.len()); + buf.extend_from_slice(prefix.as_bytes()); + buf.extend_from_slice(bytes); + // SAFETY: `bytes` is a subslice of an OsStr's encoded bytes + // that starts and ends on ASCII code point boundary. + unsafe { OsString::from_encoded_bytes_unchecked(buf) } } - fn has_context_digit(cluster: &[u8]) -> bool { - for b in cluster { - if SHORT_OPTS_WITH_VALUE.contains(b) { - return false; - } - if b.is_ascii_digit() { - return true; - } - } - false - } + let mut out: Vec = args.collect(); + let mut i = 1; // argv[0] is the executable name - let mut args = args; - let mut out = Vec::new(); - let Some(program) = args.next() else { - return out; - }; - out.push(program); + while i < out.len() { + let arg = out[i].as_encoded_bytes(); - let mut parsing_options = true; - let mut consumes_next = false; - - for arg in args { - if !parsing_options || consumes_next { - consumes_next = false; - out.push(arg); - continue; - } - - // Only ASCII short options can contain a numeric shorthand. Leaving any - // other platform-native argument intact also avoids making assumptions - // about the encoding of an OsString. - let Some(arg_str) = arg.to_str() else { - out.push(arg); + // Narrow down to `-flags` (and `--options`) and strip the leading `-` for easier matching. + let Some(arg) = arg.strip_prefix(b"-") else { + // Not a flag, skip. + i += 1; continue; }; - if arg_str == "--" { - parsing_options = false; - out.push(arg); - continue; + // No more options after `--`. + if arg == b"-" { + break; } - let Some(option) = arg_str.strip_prefix('-') else { - out.push(arg); - continue; - }; - - if option.is_empty() { - out.push(arg); + // Skip --long options. + if let Some(name) = arg.strip_prefix(b"-") { + i += 1 + usize::from(LONG_OPTS_WITH_VALUE.contains(&name)); continue; } - if let Some(long_option) = option.strip_prefix('-') { - consumes_next = LONG_OPTS_WITH_VALUE.contains(&long_option.as_bytes()); - out.push(arg); + // Find the first byte that isn't a plain short option: a digit to + // translate, or a byte that claims the rest of the argument (an option + // taking a value like -ePATTERN, or something that's no option at all). + let Some(at) = arg + .iter() + .position(|b| b.is_ascii_digit() || !b.is_ascii() || SHORT_OPTS_WITH_VALUE.contains(b)) + else { + // Plain short options only. + i += 1; continue; - } + }; - let cluster = option.as_bytes(); - if !cluster.is_ascii() { - out.push(arg); + if !arg[at].is_ascii_digit() { + // Nothing to rewrite. A value option with nothing + // attached takes the next argument instead. + let consumes_next = at + 1 == arg.len() && SHORT_OPTS_WITH_VALUE.contains(&arg[at]); + i += 1 + usize::from(consumes_next); continue; } - // A standalone digit run is one context value (`-12` means `-C 12`). - if cluster.iter().all(u8::is_ascii_digit) { - out.push(OsString::from("-C")); - out.push(OsString::from(option)); - continue; - } + // Translate -aNUMb to -a -C NUM -b. option() only deals with ASCII and + // we don't need to deal with anything else either, so the translation + // stops at the byte that claims the rest of the argument. + let end = arg[at..] + .iter() + .position(|b| !b.is_ascii() || SHORT_OPTS_WITH_VALUE.contains(b)) + .map_or(arg.len(), |n| at + n); + let (options, rest) = arg.split_at(end); + let consumes_next = rest.len() == 1 && SHORT_OPTS_WITH_VALUE.contains(&rest[0]); + let mut expanded = Vec::with_capacity(options.len() + 1); - // Digits after a value-taking option belong to that option, not to the - // context shorthand (for example, `-e123` and `-m2`). - if !has_context_digit(cluster) { - consumes_next = short_option_consumes_next(cluster); - out.push(arg); - continue; + for chunk in options.chunk_by(|a, b| a.is_ascii_digit() == b.is_ascii_digit()) { + let prefix = if chunk[0].is_ascii_digit() { "-C" } else { "-" }; + expanded.push(option(prefix, chunk)); } - // A digit embedded in a short-option cluster acts as a single `-NUM` - // option at that point in the cluster. Emitting each option separately - // preserves left-to-right override semantics. - let mut i = 0; - while i < cluster.len() { - let option = cluster[i]; - if option.is_ascii_digit() { - out.push(OsString::from("-C")); - out.push(OsString::from(char::from(option).to_string())); - i += 1; - } else if SHORT_OPTS_WITH_VALUE.contains(&option) { - let mut value_option = String::from("-"); - value_option.push_str( - std::str::from_utf8(&cluster[i..]).expect("short option cluster is ASCII"), - ); - out.push(OsString::from(value_option)); - consumes_next = i + 1 == cluster.len(); - break; - } else { - let mut flag = String::from("-"); - flag.push(char::from(option)); - out.push(OsString::from(flag)); - i += 1; - } + // The tail holds the value option with its value, if any. + if !rest.is_empty() { + expanded.push(option("-", rest)); } + + let len = expanded.len(); + out.splice(i..=i, expanded); + i += len + usize::from(consumes_next); } out diff --git a/tests/test_grep.rs b/tests/test_grep.rs index 90f65d8..717f44c 100644 --- a/tests/test_grep.rs +++ b/tests/test_grep.rs @@ -933,36 +933,27 @@ fn num_shorthand_is_context() { #[test] fn num_shorthand_in_short_option_clusters() { - let input = "a\nb\nMATCH\nc\nd\n"; + // Five lines of leading and trailing context around the match. + const CTX: &str = "zero\none\ntwo\nthree\nfour\nMATCH\nsix\nseven\neight\nnine\nten\n"; + let ok = |args: &[&str], expected: &str| { + let (_s, mut c) = ucmd(); + c.args(args).pipe_in(CTX).succeeds().stdout_only(expected); + }; - // A numeric context option can follow another short option in the same argument. - let (_s, mut c) = ucmd(); - c.args(&["-w2", "MATCH"]) - .pipe_in(input) - .succeeds() - .stdout_only(input); + // A shorthand bundled behind another short option (the reported case). + ok(&["-w2", "MATCH"], "three\nfour\nMATCH\nsix\nseven\n"); + ok(&["-i2", "match"], "three\nfour\nMATCH\nsix\nseven\n"); - let (_s, mut c) = ucmd(); - c.args(&["-i2", "match"]) - .pipe_in(input) - .succeeds() - .stdout_only(input); + // A run of digits is a single number, so `-w44` is 44 lines of context + // rather than two separate `-4` options. + ok(&["-w44", "MATCH"], CTX); - // Preserve option order when a later option overrides the bundled context. - let (_s, mut c) = ucmd(); - c.args(&["-w2C1", "MATCH"]) - .pipe_in(input) - .succeeds() - .stdout_only("b\nMATCH\nc\n"); + // Runs split by another option stay separate values, and the last wins. + ok(&["-w1x2", "MATCH"], "three\nfour\nMATCH\nsix\nseven\n"); - // Each embedded digit is a context option, so `-w44` finishes with four - // lines of context rather than treating the suffix as the number 44. - let repeated_input = "zero\none\ntwo\nthree\nfour\nMATCH\nsix\nseven\neight\nnine\nten\n"; - let (_s, mut c) = ucmd(); - c.args(&["-w44", "MATCH"]) - .pipe_in(repeated_input) - .succeeds() - .stdout_only("one\ntwo\nthree\nfour\nMATCH\nsix\nseven\neight\nnine\n"); + // Order is preserved, so a later option only overrides what it names. + ok(&["-w2C1", "MATCH"], "four\nMATCH\nsix\n"); + ok(&["-2A1", "MATCH"], "three\nfour\nMATCH\nsix\n"); // Regression for the original fuzzer case: trailing flags in the same // cluster are still parsed after the numeric options. @@ -975,67 +966,52 @@ fn num_shorthand_in_short_option_clusters() { #[test] fn num_shorthand_does_not_capture_option_values() { - // Digits attached to value-taking short options are their values. - let (_s, mut c) = ucmd(); - c.args(&["-e123"]) - .pipe_in("123\n456\n") - .succeeds() - .stdout_only("123\n"); - - let (_s, mut c) = ucmd(); - c.args(&["-m2", "x"]) - .pipe_in("x\nx\nx\n") - .succeeds() - .stdout_only("x\nx\n"); + let ok = |args: &[&str], input: &str, expected: &str| { + let (_s, mut c) = ucmd(); + c.args(args).pipe_in(input).succeeds().stdout_only(expected); + }; - let (_s, mut c) = ucmd(); - c.args(&["-wm2", "x"]) - .pipe_in("x\nx\nx\n") - .succeeds() - .stdout_only("x\nx\n"); + // Digits attached to a value-taking short option are its value. + ok(&["-e123"], "123\n456\n", "123\n"); + ok(&["-m2", "x"], "x\nx\nx\n", "x\nx\n"); + ok(&["-wm2", "x"], "x\nx\nx\n", "x\nx\n"); - // The next argument is also left alone when the preceding option consumes it. - let (_s, mut c) = ucmd(); - c.args(&["-e", "-2"]) - .pipe_in("-2\nx\n") - .succeeds() - .stdout_only("-2\n"); + // Only the first value option in a cluster takes a value, so the trailing + // `e` is `-e`'s value here and the following `-2` is still a shorthand. + ok(&["-ee", "-2"], "a\nbee\nc\nd\n", "a\nbee\nc\nd\n"); - let (_s, mut c) = ucmd(); - c.args(&["--regexp", "-2"]) - .pipe_in("-2\nx\n") - .succeeds() - .stdout_only("-2\n"); + // A detached value is left alone, even after an expanded cluster. + ok(&["-e", "-2"], "-2\nx\n", "-2\n"); + ok(&["--regexp", "-2"], "-2\nx\n", "-2\n"); + ok(&["-i2e", "-3"], "a\n-3\nb\n", "a\n-3\nb\n"); - // This remains true after an earlier shorthand forced the cluster to be expanded. - let (_s, mut c) = ucmd(); - c.args(&["-i2e", "-3"]) - .pipe_in("a\n-3\nb\n") - .succeeds() - .stdout_only("a\n-3\nb\n"); + // Non-option arguments are never shorthand. + ok(&["--regexp=-2"], "-2\nx\n", "-2\n"); + ok(&["--", "-2"], "-2\nx\n", "-2\n"); + ok(&["w2"], "w2\nw3\n", "w2\n"); } #[test] -fn num_shorthand_only_changes_short_options() { - let (_s, mut c) = ucmd(); - c.args(&["--regexp=-2"]) - .pipe_in("-2\nx\n") - .succeeds() - .stdout_only("-2\n"); - - // `--` ends option parsing, so the following `-2` is the positional pattern. - let (_s, mut c) = ucmd(); - c.args(&["--", "-2"]) - .pipe_in("-2\nx\n") - .succeeds() - .stdout_only("-2\n"); - - // Digits in an ordinary positional pattern are not option shorthand. - let (_s, mut c) = ucmd(); - c.args(&["w2"]) - .pipe_in("w2\nw3\n") - .succeeds() - .stdout_only("w2\n"); +fn num_shorthand_allows_non_ascii_attached_values() { + // A non-ASCII attached value is legitimate: `-1e🙂` is `-C 1 -e 🙂`. + let ok = |args: &[&str]| { + let (_s, mut c) = ucmd(); + c.args(args) + .pipe_in("a\n🙂\nb\nc\n") + .succeeds() + .stdout_only("a\n🙂\nb\n"); + }; + ok(&["-1e🙂"]); + ok(&["-w1e🙂"]); + + // A digit only reachable past a non-ASCII byte is not a shorthand, leaving + // the cluster the invalid option it would be without the digit. + let fails = |args: &[&str]| { + let (_s, mut c) = ucmd(); + c.args(args).pipe_in("x\n").fails().code_is(2); + }; + fails(&["-🙂2", "x"]); + fails(&["-2🙂3", "x"]); } #[cfg(unix)] @@ -1044,13 +1020,13 @@ fn num_shorthand_preserves_invalid_utf8_arguments() { use std::ffi::OsString; use std::os::unix::ffi::OsStringExt; - // Platform-native arguments that are not UTF-8 are passed through to clap - // unchanged instead of being sliced or reconstructed by the preprocessor. - let invalid_cluster = OsString::from_vec(vec![b'-', b'w', b'2', 0xff]); + // Arguments that are not valid UTF-8 reach clap without being sliced apart. let (_s, mut c) = ucmd(); - c.arg(invalid_cluster).arg("x").pipe_in("x\n").fails(); + c.arg(OsString::from_vec(vec![b'-', b'w', b'2', 0xff])) + .arg("x") + .pipe_in("x\n") + .fails(); } - #[test] fn context_line_prefixes_use_dash() { // Match line uses `:`, context lines use `-`.