From 2beb3bc069c1f019931057627ca9758b51375362 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Fri, 27 Mar 2026 00:01:31 +0000 Subject: [PATCH] fix(interpreter): apply redirects left-to-right for correct fd duplication Closes #853 --- crates/bashkit/src/interpreter/mod.rs | 168 ++++++++++++++++++ .../spec_cases/bash/pipes-redirects.test.sh | 33 ++++ 2 files changed, 201 insertions(+) diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index 526d18fb..4c88b45f 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -5436,6 +5436,174 @@ impl Interpreter { &mut self, mut result: ExecResult, redirects: &[Redirect], + ) -> Result { + // Model fd destinations to handle left-to-right evaluation order correctly. + // In bash, `2>&1 >file` means: fd2 duplicates fd1's current target (stdout), + // THEN fd1 is redirected to file. So fd2->stdout, fd1->file. + // We track where each fd points and resolve content routing at the end. + + // Use fd-table modeling when DupOutput coexists with file redirects. + // Without this, `>file 2>&1` or `2>&1 >file` would route content + // incorrectly because simple sequential application can't track + // that a dup should alias the file target rather than the buffer. + let has_dup = redirects + .iter() + .any(|r| matches!(r.kind, RedirectKind::DupOutput)); + let has_file = redirects.iter().any(|r| { + matches!( + r.kind, + RedirectKind::Output + | RedirectKind::Clobber + | RedirectKind::Append + | RedirectKind::OutputBoth + ) + }); + let needs_fd_table = has_dup && has_file; + + if !needs_fd_table { + // Fast path: no dup-then-redirect combos, use simple direct application. + return self.apply_redirections_simple(result, redirects).await; + } + + // Fd routing table: track where fd1 (stdout) and fd2 (stderr) point. + #[derive(Clone, Debug)] + enum FdTarget { + /// The original stdout buffer (returned in result.stdout) + Stdout, + /// The original stderr buffer (returned in result.stderr) + Stderr, + /// Write to a file (path, display_name, append) + File(PathBuf, String, bool), + /// Discard output + DevNull, + } + + let mut fd1_target = FdTarget::Stdout; + let mut fd2_target = FdTarget::Stderr; + + // Build the fd routing table by processing redirects left-to-right. + for redirect in redirects { + match redirect.kind { + RedirectKind::Output | RedirectKind::Clobber => { + let target_path = self.expand_word(&redirect.target).await?; + let path = self.resolve_path(&target_path); + if is_dev_null(&path) { + match redirect.fd { + Some(2) => fd2_target = FdTarget::DevNull, + _ => fd1_target = FdTarget::DevNull, + } + } else { + // noclobber check + if redirect.kind == RedirectKind::Output + && self.variables.get("SHOPT_C").map(|v| v.as_str()) == Some("1") + && self.fs.stat(&path).await.is_ok() + { + result.stdout = String::new(); + result.stderr = + format!("bash: {}: cannot overwrite existing file\n", target_path); + result.exit_code = 1; + return Ok(result); + } + let target = FdTarget::File(path, target_path, false); + match redirect.fd { + Some(2) => fd2_target = target, + _ => fd1_target = target, + } + } + } + RedirectKind::Append => { + let target_path = self.expand_word(&redirect.target).await?; + let path = self.resolve_path(&target_path); + if is_dev_null(&path) { + match redirect.fd { + Some(2) => fd2_target = FdTarget::DevNull, + _ => fd1_target = FdTarget::DevNull, + } + } else { + let target = FdTarget::File(path, target_path, true); + match redirect.fd { + Some(2) => fd2_target = target, + _ => fd1_target = target, + } + } + } + RedirectKind::OutputBoth => { + let target_path = self.expand_word(&redirect.target).await?; + let path = self.resolve_path(&target_path); + if is_dev_null(&path) { + fd1_target = FdTarget::DevNull; + fd2_target = FdTarget::DevNull; + } else { + fd1_target = FdTarget::File(path.clone(), target_path.clone(), false); + fd2_target = FdTarget::File(path, target_path, false); + } + } + RedirectKind::DupOutput => { + let target = self.expand_word(&redirect.target).await?; + let target_fd: i32 = target.parse().unwrap_or(1); + let src_fd = redirect.fd.unwrap_or(1); + // Duplicate: make src_fd point where target_fd currently points + match (src_fd, target_fd) { + (2, 1) => fd2_target = fd1_target.clone(), + (1, 2) => fd1_target = fd2_target.clone(), + _ => {} + } + } + RedirectKind::Input + | RedirectKind::HereString + | RedirectKind::HereDoc + | RedirectKind::HereDocStrip + | RedirectKind::DupInput => { + // Input redirections handled elsewhere + } + } + } + + // Route original stdout/stderr content to final destinations. + let orig_stdout = std::mem::take(&mut result.stdout); + let orig_stderr = std::mem::take(&mut result.stderr); + + // Collect file writes to handle same-file merging. + let mut file_writes: std::collections::HashMap = + std::collections::HashMap::new(); + + for (content, target) in [(&orig_stdout, &fd1_target), (&orig_stderr, &fd2_target)] { + match target { + FdTarget::Stdout => result.stdout.push_str(content), + FdTarget::Stderr => result.stderr.push_str(content), + FdTarget::DevNull => {} + FdTarget::File(path, display_name, append) => { + let entry = file_writes + .entry(path.clone()) + .or_insert_with(|| (String::new(), display_name.clone(), *append)); + entry.0.push_str(content); + } + } + } + + // Write accumulated file content + for (path, (content, display_name, append)) in &file_writes { + let write_result = if *append { + self.fs.append_file(path, content.as_bytes()).await + } else { + self.fs.write_file(path, content.as_bytes()).await + }; + if let Err(e) = write_result { + result.stderr = format!("bash: {}: {}\n", display_name, e); + result.exit_code = 1; + return Ok(result); + } + } + + Ok(result) + } + + /// Simple redirect application without fd-table modeling. + /// Used when there are no DupOutput redirects followed by other redirects. + async fn apply_redirections_simple( + &mut self, + mut result: ExecResult, + redirects: &[Redirect], ) -> Result { for redirect in redirects { match redirect.kind { diff --git a/crates/bashkit/tests/spec_cases/bash/pipes-redirects.test.sh b/crates/bashkit/tests/spec_cases/bash/pipes-redirects.test.sh index 58438ec3..4d02dcb0 100644 --- a/crates/bashkit/tests/spec_cases/bash/pipes-redirects.test.sh +++ b/crates/bashkit/tests/spec_cases/bash/pipes-redirects.test.sh @@ -212,3 +212,36 @@ END ### expect value is expanded ### end + +### redirect_2_to_1_then_file +# 2>&1 >file: stderr captured, stdout to file (left-to-right order) +f() { echo "stdout_line"; echo "stderr_line" >&2; } +result=$(f 2>&1 >/tmp/redir_order.txt) +echo "result=[$result]" +cat /tmp/redir_order.txt +### expect +result=[stderr_line] +stdout_line +### end + +### redirect_file_then_2_to_1 +# >file 2>&1: both stdout and stderr go to file, nothing captured +g() { echo "out"; echo "err" >&2; } +result=$(g >/tmp/redir_order2.txt 2>&1) +echo "result=[$result]" +cat /tmp/redir_order2.txt +### expect +result=[] +out +err +### end + +### redirect_2_to_1_only +# 2>&1 alone: stderr merges into stdout (both captured) +i() { echo "out"; echo "err" >&2; } +result=$(i 2>&1) +echo "result=[$result]" +### expect +result=[out +err] +### end