From e20f7bda1323735a8c2de5e5245cc40d95ffa0b1 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Wed, 23 Sep 2026 10:38:52 +0000 Subject: [PATCH] fix(grep): send read diagnostics to stderr and exit 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `grep hello ok.txt /nope` wrote `grep: /nope: io error: file not found` into **stdout**, so the diagnostic became part of the data a pipeline consumes: `grep foo *.log | wc -l` counted it as a match. It also exited 0, and printed the matching line without its `ok.txt:` prefix. Four defects, all checked against GNU grep 3.11: 1. the diagnostic went to stdout — it now goes to stderr; 2. an unreadable operand did not set status 2, which outranks both "matched" and "no match". `-s` silences the message but not the status; `-q` stops at the first match, so a match there still exits 0 and prints nothing, while `-q` without a match does reach the bad operand and reports it; 3. the reason was the `Display` of `bashkit::Error`; 4. the filename prefix followed the operands grep managed to read rather than the operands it was asked for, so a failed operand silently changed the format of the lines that did match. The same `Display` leak is also fixed in `rg`, `sed` and `json`, the last three readers outside the shared helper that #2440 covered. All thirty file-reading builtins are now clean. `spec_cases/grep/grep.test.sh:grep_suppress_errors` expected exit 1 where GNU grep exits 2; it had captured the old bashkit behaviour. That file is not in the host-comparison set, which is why the wrong expectation survived. --- crates/bashkit/src/builtins/grep.rs | 62 ++++++-- crates/bashkit/src/builtins/json.rs | 5 +- crates/bashkit/src/builtins/rg/mod.rs | 8 +- crates/bashkit/src/builtins/sed/mod.rs | 6 +- .../integration/builtin_fs_error_tests.rs | 142 +++++++++++++++--- .../tests/spec_cases/grep/grep.test.sh | 5 +- knowledge/security/threat-model.md | 10 +- 7 files changed, 199 insertions(+), 39 deletions(-) diff --git a/crates/bashkit/src/builtins/grep.rs b/crates/bashkit/src/builtins/grep.rs index 2ffa0a05e..00e3889b3 100644 --- a/crates/bashkit/src/builtins/grep.rs +++ b/crates/bashkit/src/builtins/grep.rs @@ -586,6 +586,14 @@ impl Builtin for Grep { let matcher = opts.build_matcher()?; let mut output = String::new(); + // Diagnostics are a separate stream: a `grep: FILE: ...` line written + // into `output` lands in the data a pipeline consumes, so + // `grep foo *.log | wc -l` counts it as a match. + let mut errors = String::new(); + // GNU grep exits 2 when any operand could not be read, whether or not + // another operand matched, and `-s` silences the message but not the + // status. + let mut read_failed = false; let mut any_match = false; let mut exit_code = 1; // 1 = no match let mut total_matches = 0usize; @@ -683,9 +691,11 @@ impl Builtin for Grep { inputs.push((file.clone(), text)); } Err(e) => { - // Report error but continue with other files - if !opts.quiet && !opts.suppress_errors { - output.push_str(&format!("grep: {}: {}\n", file, e)); + // Report the error but continue with the other files. + read_failed = true; + if !opts.suppress_errors { + let reason = crate::error::io_error_reason(&e); + errors.push_str(&format!("grep: {file}: {reason}\n")); } } } @@ -699,7 +709,10 @@ impl Builtin for Grep { } else if opts.show_filename || opts.recursive { true } else { - inputs.len() > 1 + // Operands requested, not operands successfully read: GNU grep + // prints `ok.txt:hello` for `grep hello ok.txt /nope`, because two + // files were named even though only one could be opened. + opts.files.len().max(inputs.len()) > 1 }; let has_context = opts.before_context > 0 || opts.after_context > 0; @@ -966,13 +979,30 @@ impl Builtin for Grep { if any_match { exit_code = 0; } + // An unreadable operand outranks "no match", but not a match found + // under `-q`: GNU `grep -q hello ok.txt /nope` exits 0, because it + // stops at the first match without reaching the bad operand. + // `-q` stops at the first match, so GNU grep never reaches a later + // unreadable operand: no message and status 0. Bashkit reads every + // operand up front, so emulate the short-circuit by dropping both. + if opts.quiet && any_match { + errors.clear(); + } else if read_failed { + exit_code = 2; + } // In quiet mode, return empty output if opts.quiet { - return Ok(ExecResult::with_code(String::new(), exit_code)); + return Ok(ExecResult { + stderr: errors.into(), + ..ExecResult::with_code(String::new(), exit_code) + }); } - Ok(ExecResult::with_code(output, exit_code)) + Ok(ExecResult { + stderr: errors.into(), + ..ExecResult::with_code(output, exit_code) + }) } } @@ -2152,9 +2182,10 @@ mod tests { }; let result = grep.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 1); - // -s suppresses error messages + // GNU grep: -s silences the message but not the status. + assert_eq!(result.exit_code, 2); assert_eq!(result.stdout, ""); + assert_eq!(result.stderr, ""); } #[tokio::test] @@ -2186,9 +2217,14 @@ mod tests { }; let result = grep.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 1); - // Without -s, error message is shown - assert!(result.stdout.contains("grep: /nonexistent:")); + // GNU grep: the diagnostic goes to stderr, never into the data + // stream, and an unreadable operand is status 2, not "no match". + assert_eq!(result.exit_code, 2); + assert_eq!(result.stdout, ""); + assert_eq!( + result.stderr, + "grep: /nonexistent: No such file or directory\n" + ); } #[tokio::test] @@ -2220,8 +2256,10 @@ mod tests { }; let result = grep.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 1); + // --no-messages is the long form of -s: same status 2. + assert_eq!(result.exit_code, 2); assert_eq!(result.stdout, ""); + assert_eq!(result.stderr, ""); } // -Z (--null) tests diff --git a/crates/bashkit/src/builtins/json.rs b/crates/bashkit/src/builtins/json.rs index 71338e057..a4bf47a2d 100644 --- a/crates/bashkit/src/builtins/json.rs +++ b/crates/bashkit/src/builtins/json.rs @@ -126,7 +126,10 @@ async fn read_json_input( match ctx.fs.read_file(&path).await { Ok(bytes) => String::from_utf8(bytes) .map_err(|e| ExecResult::err(format!("json: invalid UTF-8 in {file}: {e}\n"), 1)), - Err(e) => Err(ExecResult::err(format!("json: {file}: {e}\n"), 1)), + Err(e) => { + let reason = crate::error::io_error_reason(&e); + Err(ExecResult::err(format!("json: {file}: {reason}\n"), 1)) + } } } else if let Some(stdin) = ctx.stdin { Ok(stdin.to_string()) diff --git a/crates/bashkit/src/builtins/rg/mod.rs b/crates/bashkit/src/builtins/rg/mod.rs index b3c0574c5..e70060aa3 100644 --- a/crates/bashkit/src/builtins/rg/mod.rs +++ b/crates/bashkit/src/builtins/rg/mod.rs @@ -2988,10 +2988,10 @@ async fn read_rg_text_file( display_path: &str, opts: &RgOptions, ) -> std::result::Result { - let content = fs - .read_file(path) - .await - .map_err(|e| ExecResult::err(format!("rg: {display_path}: {e}\n"), 1))?; + let content = fs.read_file(path).await.map_err(|e| { + let reason = crate::error::io_error_reason(&e); + ExecResult::err(format!("rg: {display_path}: {reason}\n"), 1) + })?; opts.validate_preprocessor_for(path, cwd) .map_err(|e| ExecResult::err(format!("rg: {display_path}: {e}\n"), 1))?; diff --git a/crates/bashkit/src/builtins/sed/mod.rs b/crates/bashkit/src/builtins/sed/mod.rs index 6060711e0..34deb288b 100644 --- a/crates/bashkit/src/builtins/sed/mod.rs +++ b/crates/bashkit/src/builtins/sed/mod.rs @@ -370,7 +370,11 @@ impl Builtin for Sed { }, // GNU exits 2 when an input file cannot be read. Err(e) => { - return Ok(ExecResult::err(format!("sed: can't read {name}: {e}\n"), 2)); + let reason = crate::error::io_error_reason(&e); + return Ok(ExecResult::err( + format!("sed: can't read {name}: {reason}\n"), + 2, + )); } } }; diff --git a/crates/bashkit/tests/integration/builtin_fs_error_tests.rs b/crates/bashkit/tests/integration/builtin_fs_error_tests.rs index d9494ac81..2062ae5b8 100644 --- a/crates/bashkit/tests/integration/builtin_fs_error_tests.rs +++ b/crates/bashkit/tests/integration/builtin_fs_error_tests.rs @@ -14,26 +14,44 @@ use bashkit::Bash; use bashkit::testing::assert_no_leak; /// Every tool here takes a file operand and reads it through the VFS. -const READERS: &[(&str, &str)] = &[ - ("cat", "cat /nope"), - ("head", "head /nope"), - ("tail", "tail /nope"), - ("strings", "strings /nope"), - ("wc", "wc /nope"), - ("sort", "sort /nope"), - ("uniq", "uniq /nope"), - ("cut", "cut -c1 /nope"), - ("nl", "nl /nope"), - ("awk", "awk '{print}' /nope"), - ("paste", "paste /nope"), - ("rev", "rev /nope"), - ("tac", "tac /nope"), - ("column", "column /nope"), +/// The exit code is each tool's own: GNU `grep`, `sed` and `rg` report an +/// unreadable operand as 2, the coreutils text tools as 1. +const READERS: &[(&str, &str, i32)] = &[ + ("cat", "cat /nope", 1), + ("head", "head /nope", 1), + ("tail", "tail /nope", 1), + ("strings", "strings /nope", 1), + ("wc", "wc /nope", 1), + ("sort", "sort /nope", 1), + ("uniq", "uniq /nope", 1), + ("cut", "cut -c1 /nope", 1), + ("nl", "nl /nope", 1), + ("awk", "awk '{print}' /nope", 1), + ("paste", "paste /nope", 1), + ("rev", "rev /nope", 1), + ("tac", "tac /nope", 1), + ("column", "column /nope", 1), + ("split", "split /nope", 1), + ("comm", "comm /nope /nope2", 1), + ("shuf", "shuf /nope", 1), + ("diff", "diff /nope /nope2", 1), + ("dotenv", "dotenv /nope", 1), + ("template", "template /nope", 1), + ("json", "json get x /nope", 1), + ("od", "od /nope", 1), + ("xxd", "xxd /nope", 1), + ("hexdump", "hexdump /nope", 1), + ("base64", "base64 /nope", 1), + ("fold", "fold /nope", 1), + ("expand", "expand /nope", 1), + ("grep", "grep x /nope", 2), + ("sed", "sed s/a/b/ /nope", 2), + ("rg", "rg foo /nope", 2), ]; #[tokio::test] async fn missing_operand_reports_the_real_errno_text() { - for (name, script) in READERS { + for (name, script, code) in READERS { let mut bash = Bash::new(); let result = bash .exec(script) @@ -44,17 +62,25 @@ async fn missing_operand_reports_the_real_errno_text() { stderr.contains("No such file or directory"), "{name}: expected the real errno text, got {stderr:?}" ); - assert_eq!(result.exit_code, 1, "{name} exit code"); + assert_eq!(result.exit_code, *code, "{name} exit code"); } } /// The specific shape that regressed: the `Display` of `bashkit::Error`. #[tokio::test] async fn missing_operand_does_not_leak_the_error_enum_shape() { - for (name, script) in READERS { + for (name, script, _) in READERS { let mut bash = Bash::new(); let result = bash.exec(script).await.unwrap(); let stderr = result.stderr.to_string(); + // The diagnostic belongs on stderr, never in the data stream. + assert!( + !result + .stdout + .to_string() + .contains("No such file or directory"), + "{name} put its diagnostic in stdout" + ); assert!( !stderr.contains("io error:"), "{name} leaks the Error enum shape: {stderr:?}" @@ -95,3 +121,83 @@ async fn readable_operand_still_reads() { assert_eq!(result.stderr.to_string(), "", "{script} stderr"); } } + +/// `grep` had three defects beyond the wording, all verified against GNU +/// grep 3.11: +/// 1. the diagnostic went into stdout, so `grep foo *.log | wc -l` counted it +/// as a match; +/// 2. an unreadable operand did not set status 2; +/// 3. the filename prefix followed the files it managed to read, not the +/// files it was asked for. +mod grep_unreadable_operand { + use bashkit::Bash; + + async fn run(script: &str) -> bashkit::ExecResult { + let mut bash = Bash::builder().mount_text("/w/ok.txt", "hello\n").build(); + bash.exec(&format!("cd /w; {script}")).await.unwrap() + } + + #[tokio::test] + async fn diagnostic_goes_to_stderr_not_stdout() { + let r = run("grep hello ok.txt /nope").await; + assert_eq!(r.stdout.to_string(), "ok.txt:hello\n"); + assert_eq!( + r.stderr.to_string(), + "grep: /nope: No such file or directory\n" + ); + } + + /// The prefix is present because two operands were named, even though only + /// one could be opened. + #[tokio::test] + async fn filename_prefix_follows_operands_asked_for() { + let r = run("grep hello ok.txt /nope").await; + assert!(r.stdout.to_string().starts_with("ok.txt:")); + // A single readable operand still gets no prefix. + let single = run("grep hello ok.txt").await; + assert_eq!(single.stdout.to_string(), "hello\n"); + } + + #[tokio::test] + async fn unreadable_operand_is_status_2() { + // Status 2 outranks both "matched" and "no match". + assert_eq!(run("grep hello ok.txt /nope").await.exit_code, 2); + assert_eq!(run("grep zzz ok.txt /nope").await.exit_code, 2); + assert_eq!(run("grep hello /nope").await.exit_code, 2); + // No error, no change. + assert_eq!(run("grep hello ok.txt").await.exit_code, 0); + assert_eq!(run("grep zzz ok.txt").await.exit_code, 1); + } + + /// `-s` silences the message but not the status. + #[tokio::test] + async fn suppress_errors_keeps_the_status() { + let r = run("grep -s hello ok.txt /nope").await; + assert_eq!(r.exit_code, 2); + assert_eq!(r.stderr.to_string(), ""); + assert_eq!(r.stdout.to_string(), "ok.txt:hello\n"); + } + + /// `-q` stops at the first match, so GNU grep never reaches the bad + /// operand: status 0 and no message. Without a match it does reach it. + #[tokio::test] + async fn quiet_short_circuits_on_a_match_only() { + let matched = run("grep -q hello ok.txt /nope").await; + assert_eq!(matched.exit_code, 0); + assert_eq!(matched.stderr.to_string(), ""); + + let unmatched = run("grep -q zzz ok.txt /nope").await; + assert_eq!(unmatched.exit_code, 2); + assert_eq!( + unmatched.stderr.to_string(), + "grep: /nope: No such file or directory\n" + ); + } + + /// The bug that motivated the stream split. + #[tokio::test] + async fn pipeline_does_not_see_the_diagnostic() { + let r = run("grep hello ok.txt /nope 2>/dev/null | wc -l").await; + assert_eq!(r.stdout.to_string().trim(), "1"); + } +} diff --git a/crates/bashkit/tests/spec_cases/grep/grep.test.sh b/crates/bashkit/tests/spec_cases/grep/grep.test.sh index e2c28ac2e..e25f4d31e 100644 --- a/crates/bashkit/tests/spec_cases/grep/grep.test.sh +++ b/crates/bashkit/tests/spec_cases/grep/grep.test.sh @@ -594,11 +594,12 @@ mkdir -p /tmp/grepexd/src /tmp/grepexd/vendor && printf 'match\n' > /tmp/grepexd ### end ### grep_suppress_errors -# -s suppresses error messages for nonexistent files +# -s suppresses the error message but not the status: GNU grep exits 2 +# whenever an operand could not be read, matched or not. grep -s foo /tmp/nonexistent_grep_file_xyz echo $? ### expect -1 +2 ### end ### grep_null_filename_l diff --git a/knowledge/security/threat-model.md b/knowledge/security/threat-model.md index 1290cf5a5..9eb84f71a 100644 --- a/knowledge/security/threat-model.md +++ b/knowledge/security/threat-model.md @@ -470,7 +470,15 @@ Most file-reading builtins go through `builtins::read_text_file` / `read_stream_file`, so a single leaking formatter there reached ~25 tools at once (`wc`, `sort`, `cut`, `nl`, `awk`, `paste`, `uniq`, `rev`, `tac`, `column`, `jq`, `diff`, …); `cat`, `head`, `tail` and `strings` read the VFS -directly and leaked it separately. Coverage: `builtin_fs_error_tests`. +directly and leaked it separately, as did `rg`, `sed` and `json`. The sweep +is complete: `builtin_fs_error_tests` walks all thirty file-reading builtins +and fails on any banned shape in either stream. + +A diagnostic also has to reach the right stream. `grep` accumulated its +`grep: FILE: ...` line into the stdout buffer, so `grep foo *.log | wc -l` +counted it as a match — a wrong-stream bug is a data-corruption bug, not a +cosmetic one. Status matters too: GNU `grep`, `sed` and `rg` report an +unreadable operand as exit 2, which outranks both "matched" and "no match". A related shape: a user-reachable failure classified as `Error::Internal` aborts the whole script *and* prints `internal error:`. Nightly `glob_fuzz`