Skip to content

fix(parser): run commands before a later syntax error; quote/heredoc-aware $( ) end #2513

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

Merged
chaliy merged 1 commit into from
Oct 6, 2026
Merged

fix(parser): run commands before a later syntax error; quote/heredoc-aware $( ) end #2513

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
42 changes: 39 additions & 3 deletions crates/bashkit/src/interpreter/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,24 @@ fn decode_file_bytes_for_path(_path: &Path, bytes: &[u8]) -> String {
String::from_utf8_lossy(bytes).into_owned()
}

/// Child-shell counterpart of `recover_partial_parse` in `lib.rs`: commands
/// before a syntax error run, then the error is reported (exit 2). `bash -n`
/// keeps whole-script rejection since it only checks syntax.
fn nested_partial_parse(
(mut script, error): (Script, Option<crate::error::Error>),
noexec: bool,
shell_name: &str,
) -> Result<Script> {
match error {
None => Ok(script),
Some(e) if noexec || script.commands.is_empty() => Err(e),
Some(e) => {
script.trailing_error = Some(format!("{shell_name}: syntax error: {e}\n"));
Ok(script)
}
}
}

/// Check if a path refers to /dev/null after normalization.
/// Handles attempts to bypass via paths like `/dev/../dev/null`.
fn is_dev_null(path: &Path) -> bool {
Expand Down Expand Up @@ -2698,6 +2716,7 @@ impl Interpreter {
let max_stdout = self.limits.max_stdout_bytes;
let max_stderr = self.limits.max_stderr_bytes;

let mut stopped = false;
for command in &script.commands {
self.check_cancelled()?;
let emit_before = self.output_emit_count;
Expand Down Expand Up @@ -2750,14 +2769,17 @@ impl Interpreter {
Some(event) => {
exit_code = event.code;
self.last_exit_code = exit_code;
stopped = true;
break;
}
None => continue,
}
} else {
stopped = true;
break;
}
} else {
stopped = true;
break;
}
}
Expand All @@ -2783,11 +2805,25 @@ impl Interpreter {
let suppressed = matches!(command, Command::Pipeline(p) if p.negated)
|| result.errexit_suppressed;
if !suppressed {
stopped = true;
break;
}
}
}

// Syntax error after the commands that ran: bash reads and runs a
// script line by line, so it reports the error only on reaching it.
if !stopped && let Some(message) = &script.trailing_error {
let emit_before = self.output_emit_count;
let err = crate::StreamData::from(message.clone());
self.maybe_emit_output(&crate::StreamData::new(), &err, emit_before);
if !stderr_truncated {
stderr.append(&err);
}
exit_code = 2;
self.last_exit_code = 2;
}

// Run EXIT trap if registered (only for top-level execute)
#[allow(clippy::collapsible_if)]
if run_exit_trap {
Expand Down Expand Up @@ -4737,7 +4773,7 @@ impl Interpreter {
Some(parser_timeout),
)
.with_execution_budget(self.execution_budget.clone());
match parser.parse() {
match nested_partial_parse(parser.parse_recovering(), noexec, shell_name) {
Ok(s) => s,
Err(e) => {
return Ok(ExecResult::err(
Expand All @@ -4759,12 +4795,12 @@ impl Interpreter {
let parser =
Parser::with_limits(&script_owned, max_ast_depth, max_parser_operations)
.with_execution_budget(execution_budget);
parser.parse()
parser.parse_recovering()
})
.await
})
.await;
match parse_result {
match parse_result.map(|r| r.map(|p| nested_partial_parse(p, noexec, shell_name))) {
Ok(Ok(Ok(s))) => s,
Ok(Ok(Err(e))) => {
return Ok(ExecResult::err(
Expand Down
38 changes: 29 additions & 9 deletions crates/bashkit/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,7 +1058,7 @@ impl Bash {
Some(parser_timeout),
)
.with_execution_budget(self.interpreter.execution_budget().clone());
parser.parse()?
recover_partial_parse(parser.parse_recovering())?
};

// On native targets, parse inline for small scripts (avoid threadpool
Expand All @@ -1068,7 +1068,7 @@ impl Bash {
let ast = if input_len <= SPAWN_BLOCKING_THRESHOLD {
let parser = Parser::with_limits(script, max_ast_depth, max_parser_operations)
.with_execution_budget(self.interpreter.execution_budget().clone());
match parser.parse() {
match recover_partial_parse(parser.parse_recovering()) {
Ok(ast) => {
#[cfg(feature = "logging")]
tracing::debug!(target: "bashkit::parser", "Parse completed (inline)");
Expand All @@ -1088,7 +1088,7 @@ impl Bash {
let parser =
Parser::with_limits(&script_owned, max_ast_depth, max_parser_operations)
.with_execution_budget(execution_budget);
parser.parse()
recover_partial_parse(parser.parse_recovering())
})
.await
})
Expand Down Expand Up @@ -3978,6 +3978,26 @@ pub mod logging_guide {}
#[doc = include_str!("../docs/hooks.md")]
pub mod hooks_guide {}

/// Turn a [`parser::Parser::parse_recovering`] result into the script to run.
///
/// Decision: like bash, commands on lines before a syntax error still run and
/// the error is reported afterwards (stderr + exit 2, via
/// `Script::trailing_error`). With nothing runnable before it, the error stays
/// a hard `Err` exactly as before, so callers that only validate syntax are
/// unaffected.
fn recover_partial_parse(
(mut script, error): (parser::Script, Option<Error>),
) -> Result<parser::Script> {
match error {
None => Ok(script),
Some(e) if script.commands.is_empty() => Err(e),
Some(e) => {
script.trailing_error = Some(format!("bash: syntax error: {e}\n"));
Ok(script)
}
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -6923,15 +6943,15 @@ if true; then
echo missing fi"#,
)
.await;
// Should fail to parse due to missing 'fi'
assert!(result.is_err());
let err = result.unwrap_err();
let err_msg = format!("{}", err);
// Like bash: line 1 runs, then the missing 'fi' is reported (exit 2)
let result = result.unwrap();
assert_eq!(result.stdout, "ok\n");
assert_eq!(result.exit_code, 2);
// Error should mention line number
assert!(
err_msg.contains("line") || err_msg.contains("parse"),
result.stderr.contains("line") && result.stderr.contains("parse"),
"Error should be a parse error: {}",
err_msg
result.stderr
);
}

Expand Down
7 changes: 7 additions & 0 deletions crates/bashkit/src/parser/ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ pub struct Script {
pub commands: Vec<Command>,
/// Source span of the entire script
pub span: Span,
/// Syntax error found after `commands` (see `Parser::parse_recovering`).
/// Reported as stderr + exit 2 once every command ran, unless one of
/// them stopped the script first (`exit`, `set -e`), matching bash's
/// line-by-line read-then-execute model.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub trailing_error: Option<String>,
}

/// A single command in the script.
Expand Down Expand Up @@ -1281,6 +1287,7 @@ mod tests {
let script = Script {
commands: vec![],
span: Span::new(),
trailing_error: None,
};
assert!(script.commands.is_empty());
}
Expand Down
1 change: 1 addition & 0 deletions crates/bashkit/src/parser/budget.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,7 @@ mod tests {
let ast = Script {
commands,
span: Span::new(),
trailing_error: None,
};
let limits = ExecutionLimits::default();
let err = validate(&ast, &limits).unwrap_err();
Expand Down
157 changes: 18 additions & 139 deletions crates/bashkit/src/parser/lexer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,44 +821,10 @@ impl<'a> Lexer<'a> {
}
}
} else {
// THREAT[TM-DOS]: Track substitution nesting depth to
// enforce max_subst_depth consistently in both quoted
// and unquoted contexts (issue #996).
let mut depth = 1;
let mut subst_depth = 1usize;
while let Some(c) = self.peek_char() {
word.push(c);
self.advance();
if c == '$' && self.peek_char() == Some('(') {
subst_depth += 1;
if subst_depth > self.max_subst_depth {
// Depth limit exceeded — consume remaining
// parens and stop nesting deeper.
while let Some(ic) = self.peek_char() {
word.push(ic);
self.advance();
if ic == '(' {
depth += 1;
} else if ic == ')' {
depth -= 1;
if depth == 0 {
break;
}
}
}
break;
}
}
if c == '(' {
depth += 1;
} else if c == ')' {
depth -= 1;
if depth == 0 {
break;
}
}
}
if depth > 0 {
// Quote/heredoc-aware end of the body (see subst_scan).
// Nesting depth is bounded where bodies are parsed
// (parser max_depth) and run (max_subst_depth).
if !self.read_command_subst_body(&mut word) {
return Some(Token::Error(
"unterminated command substitution".to_string(),
));
Expand Down Expand Up @@ -1634,112 +1600,25 @@ impl<'a> Lexer<'a> {
result
}

/// Read command substitution content after `$(`, handling nested parens and quotes.
/// Appends chars to `content` and adds the closing `)`.
/// THREAT[TM-DOS-044]: `subst_depth` tracks nesting to prevent stack overflow.
/// Read command substitution content after `$(`, handling nested parens,
/// quotes, comments and heredoc bodies (see `subst_scan`). Appends chars
/// to `content` and adds the closing `)`.
fn read_command_subst_into(&mut self, content: &mut String) {
self.read_command_subst_into_depth(content, 0);
self.read_command_subst_body(content);
}

fn read_command_subst_into_depth(&mut self, content: &mut String, subst_depth: usize) {
if subst_depth >= self.max_subst_depth {
// Depth limit exceeded — consume until matching ')' and emit error token
let mut depth = 1;
while let Some(c) = self.peek_char() {
self.advance();
match c {
'(' => depth += 1,
')' => {
depth -= 1;
if depth == 0 {
content.push(')');
return;
}
}
_ => {}
}
}
return;
}

let mut depth = 1;
while let Some(c) = self.peek_char() {
match c {
'(' => {
depth += 1;
content.push(c);
self.advance();
}
')' => {
depth -= 1;
self.advance();
if depth == 0 {
content.push(')');
break;
}
content.push(c);
}
'"' => {
// Nested double-quoted string inside $()
content.push('"');
self.advance();
while let Some(qc) = self.peek_char() {
match qc {
'"' => {
content.push('"');
self.advance();
break;
}
'\\' => {
content.push('\\');
self.advance();
if let Some(esc) = self.peek_char() {
content.push(esc);
self.advance();
}
}
'$' => {
content.push('$');
self.advance();
if self.peek_char() == Some('(') {
content.push('(');
self.advance();
self.read_command_subst_into_depth(content, subst_depth + 1);
}
}
_ => {
content.push(qc);
self.advance();
}
}
}
}
'\'' => {
// Single-quoted string inside $()
content.push('\'');
self.advance();
while let Some(qc) = self.peek_char() {
content.push(qc);
self.advance();
if qc == '\'' {
break;
}
}
}
'\\' => {
content.push('\\');
self.advance();
if let Some(esc) = self.peek_char() {
content.push(esc);
self.advance();
}
}
_ => {
content.push(c);
self.advance();
}
/// Like `read_command_subst_into`; returns whether the closing `)` was
/// found. The scanner is iterative, so deep nesting cannot overflow the
/// host stack here (TM-DOS-044).
fn read_command_subst_body(&mut self, content: &mut String) -> bool {
let mut scanner = super::subst_scan::SubstScanner::new();
while let Some(c) = self.advance() {
content.push(c);
if scanner.feed(c) == super::subst_scan::Step::Close {
return true;
}
}
false
}

/// Read parameter expansion content after `${`, handling nested braces and quotes.
Expand Down
Loading
Loading