diff --git a/crates/bashkit/src/builtins/read.rs b/crates/bashkit/src/builtins/read.rs index 5deb7fa9d..73d95d33f 100644 --- a/crates/bashkit/src/builtins/read.rs +++ b/crates/bashkit/src/builtins/read.rs @@ -56,7 +56,17 @@ impl Builtin for Read { } else { &rest }; - nchars = n_str.parse().ok(); + // Bash rejects a non-numeric count rather than + // reading an unbounded line. + match n_str.parse() { + Ok(n) => nchars = Some(n), + Err(_) => { + return Ok(ExecResult::err( + format!("read: {n_str}: invalid number\n"), + 1, + )); + } + } break; } 'p' => { @@ -76,7 +86,15 @@ impl Builtin for Read { } else { &rest }; - timeout = t_str.parse().ok().filter(|t: &f64| t.is_finite()); + match t_str.parse::() { + Ok(t) if t.is_finite() => timeout = Some(t), + _ => { + return Ok(ExecResult::err( + format!("read: {t_str}: invalid timeout specification\n"), + 1, + )); + } + } break; } 'u' => { diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index 76f827df7..7dbe0614c 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -889,6 +889,18 @@ fn normalize_dev_path(path: &Path) -> PathBuf { /// THREAT[TM-INJ-009,TM-INJ-016]: Check if a variable name is an internal marker. /// Used by builtins and interpreter to block user assignment to internal prefixes. /// Note: `_TTY_` is intentionally excluded — it is user-configurable (bashkit extension). +/// A name `unset -v` accepts: an identifier, optionally with a subscript. +fn is_unset_target_name(name: &str) -> bool { + let base = match name.find('[') { + Some(i) if name.ends_with(']') => &name[..i], + Some(_) => return false, + None => name, + }; + let mut chars = base.chars(); + matches!(chars.next(), Some(c) if c.is_ascii_alphabetic() || c == '_') + && chars.all(|c| c.is_ascii_alphanumeric() || c == '_') +} + pub(crate) fn is_internal_variable(name: &str) -> bool { name.starts_with("SHOPT_") || name.starts_with("_NAMEREF_") @@ -1438,6 +1450,10 @@ pub struct Interpreter { /// `unset LINENO` makes it an ordinary variable for the rest of the /// shell's life, like bash. lineno_unset: bool, + /// How many `source`d scripts are running. `return` is only legal inside + /// a function or a sourced script, so it needs to tell the two apart from + /// a plain top-level command. + source_depth: usize, /// HTTP client for network builtins (curl, wget) #[cfg(feature = "http_client")] http_client: Option>, @@ -2046,6 +2062,7 @@ impl Interpreter { jobs: jobs::new_shared_job_table(), current_line: 1, lineno_unset: false, + source_depth: 0, #[cfg(feature = "http_client")] http_client: None, #[cfg(feature = "git")] @@ -2189,6 +2206,7 @@ impl Interpreter { fs: Arc::clone(&self.fs), env: self.env.clone(), scoped: self.scoped.clone(), + source_depth: self.source_depth, flags: self.flags, cwd: self.cwd.clone(), last_exit_code: self.last_exit_code, @@ -3354,6 +3372,13 @@ impl Interpreter { break; } } else { + // `return N` carries its status in the control flow, not + // in `exit_code`, so a sourced script's `return 4` is + // `source`'s status. + if let ControlFlow::Return(code) = result.control_flow { + exit_code = code; + self.last_exit_code = code; + } stopped = true; break; } @@ -7275,6 +7300,21 @@ impl Interpreter { self.insert_variable_checked("_".to_string(), name.to_string()); } + // `return` outside a function or a sourced script is a usage + // error in bash: it reports, exits 2 and the script carries on. + // `keeps_arg0` marks a function's frame; a frame that only carries + // top-level positional parameters (`bashkit script.sh a b`) does + // not make `return` legal. + let in_function = self.call_stack.iter().any(|f| f.keeps_arg0); + if name == "return" && !in_function && self.source_depth == 0 { + self.last_exit_code = 2; + return Ok(ExecResult::err( + "bash: return: can only `return' from a function or sourced script\n" + .to_string(), + 2, + )); + } + // Check for nounset error from argument expansion if let Some(err_msg) = self.take_expansion_error() { self.last_exit_code = 1; @@ -8637,7 +8677,9 @@ impl Interpreter { // Execute the script commands in the current shell context. // Use execute_script_body (not execute) to preserve depth counters. + self.source_depth += 1; let exec_result = self.execute_script_body(&script, false, true).await; + self.source_depth -= 1; // Pop source depth and BASH_SOURCE (always, even on error) self.counters.pop_function(); @@ -8646,6 +8688,13 @@ impl Interpreter { let mut result = exec_result?; + // `return N` inside a sourced script is the status of `source`, and + // stops the sourced file only, not the script that sourced it. + if let ControlFlow::Return(code) = result.control_flow { + result.exit_code = code; + result.control_flow = ControlFlow::None; + } + // Restore positional parameters if has_source_args { if let Some(saved) = saved_positional { @@ -9382,6 +9431,7 @@ impl Interpreter { ) -> Result { let mut unset_nameref = false; let mut unset_function = false; + let mut explicit_variable = false; let mut var_args: Vec<&String> = Vec::new(); for arg in args { if arg == "-n" { @@ -9389,12 +9439,31 @@ impl Interpreter { } else if arg == "-f" { unset_function = true; } else if arg == "-v" { - // -v (variable, default) - explicit variable mode + explicit_variable = true; } else { var_args.push(arg); } } + // With an explicit `-v` bash insists the name is one it could have + // assigned; without it, and under `-f`, any word is accepted and a + // name that does not exist is simply skipped. + if explicit_variable && !unset_function { + for arg in &var_args { + if !is_unset_target_name(arg) { + return self + .apply_redirections( + ExecResult::err( + format!("bash: unset: `{arg}': not a valid identifier\n"), + 1, + ), + redirects, + ) + .await; + } + } + } + let mut stderr = String::new(); let mut exit_code: i32 = 0; diff --git a/crates/bashkit/tests/integration/builtin_validation_tests.rs b/crates/bashkit/tests/integration/builtin_validation_tests.rs new file mode 100644 index 000000000..8415520e9 --- /dev/null +++ b/crates/bashkit/tests/integration/builtin_validation_tests.rs @@ -0,0 +1,87 @@ +//! Builtin arguments bash refuses, and what it says about them. +//! +//! `return` outside a function or a sourced script, a name `unset -v` could +//! never have assigned, and a non-numeric `read -t`/`-n` are all usage errors +//! in bash. Each reports and fails; the script keeps running. + +use bashkit::Bash; + +async fn run(script: &str) -> bashkit::ExecResult { + Bash::builder().build().exec(script).await.unwrap() +} + +#[tokio::test] +async fn return_outside_a_function_reports_and_keeps_going() { + let r = run("return\necho \"rc=$?\"\necho after").await; + assert_eq!( + r.stderr.to_string(), + "bash: return: can only `return' from a function or sourced script\n" + ); + assert_eq!(r.stdout.to_string(), "rc=2\nafter\n"); +} + +#[tokio::test] +async fn return_inside_a_function_still_works() { + let r = run("f() { return 3; }\nf\necho \"rc=$?\"").await; + assert_eq!(r.stderr.to_string(), ""); + assert_eq!(r.stdout.to_string(), "rc=3\n"); +} + +#[tokio::test] +async fn return_in_a_sourced_script_is_its_status() { + let r = run("printf 'echo in\\nreturn 4\\necho no\\n' > s.sh\n. ./s.sh\necho \"rc=$?\"").await; + assert_eq!(r.stderr.to_string(), ""); + assert_eq!(r.stdout.to_string(), "in\nrc=4\n"); +} + +#[tokio::test] +async fn unset_v_insists_on_an_identifier() { + for name in ["arr[", "a-b", "1bad"] { + let r = run(&format!("unset -v '{name}'\necho \"rc=$?\"")).await; + assert_eq!( + r.stderr.to_string(), + format!("bash: unset: `{name}': not a valid identifier\n") + ); + assert_eq!(r.stdout.to_string(), "rc=1\n"); + } +} + +/// Without `-v`, and under `-f`, bash takes any word and skips what it +/// cannot find. +#[tokio::test] +async fn unset_without_v_accepts_any_word() { + let r = run("unset 'arr['\nunset -f 'f['\necho \"rc=$?\"").await; + assert_eq!(r.stderr.to_string(), ""); + assert_eq!(r.stdout.to_string(), "rc=0\n"); +} + +#[tokio::test] +async fn unset_v_still_takes_a_subscript() { + let r = run("a=(x y)\nunset -v 'a[0]'\necho \"${a[@]}\"").await; + assert_eq!(r.stderr.to_string(), ""); + assert_eq!(r.stdout.to_string(), "y\n"); +} + +#[tokio::test] +async fn read_rejects_a_non_numeric_timeout() { + let r = run("read -t abc x <<< ''\necho \"rc=$?\"").await; + assert_eq!( + r.stderr.to_string(), + "read: abc: invalid timeout specification\n" + ); + assert_eq!(r.stdout.to_string(), "rc=1\n"); +} + +#[tokio::test] +async fn read_rejects_a_non_numeric_count() { + let r = run("read -n abc x <<< ''\necho \"rc=$?\"").await; + assert_eq!(r.stderr.to_string(), "read: abc: invalid number\n"); + assert_eq!(r.stdout.to_string(), "rc=1\n"); +} + +#[tokio::test] +async fn read_still_takes_a_numeric_count() { + let r = run("read -n 3 x <<< 'hello'\necho \"$x\"").await; + assert_eq!(r.stderr.to_string(), ""); + assert_eq!(r.stdout.to_string(), "hel\n"); +} diff --git a/crates/bashkit/tests/integration/main.rs b/crates/bashkit/tests/integration/main.rs index c781b5591..99b368ac1 100644 --- a/crates/bashkit/tests/integration/main.rs +++ b/crates/bashkit/tests/integration/main.rs @@ -30,6 +30,7 @@ pub mod builtin_cap_security_tests; pub mod builtin_error_security_tests; pub mod builtin_fs_error_tests; pub mod builtin_registry_tests; +pub mod builtin_validation_tests; pub mod byte_range_panic_tests; pub mod byte_stream_tests; pub mod cancellation_tests; diff --git a/crates/bashkit/tests/spec_cases/bash/builtin-validation.test.sh b/crates/bashkit/tests/spec_cases/bash/builtin-validation.test.sh new file mode 100644 index 000000000..5858700b7 --- /dev/null +++ b/crates/bashkit/tests/spec_cases/bash/builtin-validation.test.sh @@ -0,0 +1,69 @@ +### return_outside_a_function_reports_and_keeps_going +return 2>/dev/null +echo "rc=$?" +echo after +### expect +rc=2 +after +### end + +### return_inside_a_function_still_works +f() { return 3; } +f +echo "rc=$?" +### expect +rc=3 +### end + +### return_in_a_sourced_script_is_its_status +printf 'echo in\nreturn 4\necho no\n' > s.sh +. ./s.sh +echo "rc=$?" +### expect +in +rc=4 +### end + +### unset_v_insists_on_an_identifier +unset -v 'arr[' 2>/dev/null +echo "rc=$?" +### expect +rc=1 +### end + +### unset_without_v_accepts_any_word +unset 'arr[' +unset -f 'f[' +echo "rc=$?" +### expect +rc=0 +### end + +### unset_v_still_takes_a_subscript +a=(x y) +unset -v 'a[0]' +echo "${a[@]}" +### expect +y +### end + +### read_rejects_a_non_numeric_timeout +read -t abc x <<< '' 2>/dev/null +echo "rc=$?" +### expect +rc=1 +### end + +### read_rejects_a_non_numeric_count +read -n abc x <<< '' 2>/dev/null +echo "rc=$?" +### expect +rc=1 +### end + +### read_still_takes_a_numeric_count +read -n 3 x <<< 'hello' +echo "$x" +### expect +hel +### end diff --git a/knowledge/log.md b/knowledge/log.md index 25f79e3a6..f764d00a2 100644 --- a/knowledge/log.md +++ b/knowledge/log.md @@ -4,6 +4,7 @@ * **Fix**: `grep` follows GNU grep 3.11. Prefixes print as `FILE:LINE:BYTE:` (context lines with `-`), group separators appear between non-adjacent groups and files whenever a context option is given (`-NUM`, `--group-separator`, `--no-group-separator`), trailing context after `-m` is printed in full, `-o` skips empty matches and is POSIX leftmost-longest (`grep -oE 'ab|abcd'` prints `abcd`), `-b -o` gives each match's offset. New `grep_pattern` translates GNU BRE/ERE (brackets, intervals, `\< \> \b \w`, literal leading `*` in BRE, dropped leading ERE operator with GNU's warning, glibc error messages) onto awk's leftmost-longest `AwkRegex`; back-references work in BRE and ERE via fancy-regex (TM-DOS-025 cap). Options parse like getopt_long (unique prefixes such as `--no-gr`, `-T`, `-d`, `--label`, `-` for stdin, `-R` follows symlinks), an empty `-f` file matches nothing, a directory operand without `-r` reports `Is a directory`, invalid UTF-8 output turns the file binary, `\r` is line data. Debian-oracle grep score 60.5% to 96.8% (185 cases); grep's probes in the regex set all pass except two raw-`\x01` patterns the shell drops. Remaining misses are readdir order (L-GREP-003); L-GREP-002 lists the other gaps. See foundations/builtins "grep". +* **Fix**: builtins reject the arguments bash rejects. `return` outside a function or a sourced script reports `can only \`return' from a function or sourced script`, exits 2 and the script carries on (it used to end the script with status 0); `return N` inside a sourced script is now `source`'s status; `unset -v` insists the name is one it could have assigned (`arr[`, `a-b`, `1bad` are errors, `a[0]` is fine) while plain `unset` and `unset -f` still take any word; and `read -t`/`-n` report `invalid timeout specification` / `invalid number` instead of silently ignoring the option. * **Fix**: jq `implode` maps invalid code points to U+FFFD, `strptime`/`fromdate` fail with jq's `date "X" does not match format "F"`, and `@base64d` accepts unpadded input and non-zero trailing bits with jq's error texts. * **Feature**: jq destructuring alternatives `T as P1 ?// P2 | BODY`: every pattern variable is bound (null when unbound), an error in a pattern or the body moves to the next pattern, the last error propagates. jaq does not parse `?//`; `builtins/jq/altpat.rs` rewrites it to `try`/`catch` before compiling (body copied per pattern, at most 16 rewrites). * **Feature**: jq `$__loc__` (`{"file":"","line":N}`), including `{$__loc__}` shorthand and string interpolation. jaq has no `$__loc__`, so the filter text is rewritten before compiling (`builtins/jq/loc.rs` skips strings and comments).