From d4465830dd3183140463f0b17a1cb30402efb43c Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Fri, 27 Mar 2026 01:48:08 +0000 Subject: [PATCH 1/3] fix(interpreter): correct left-to-right redirect ordering for fd dup + file combos MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #853 — `result=$(cmd 2>&1 >file)` now correctly captures stderr in result and writes stdout to file. The old approach processed redirections by mutating stdout/stderr strings sequentially, which broke when DupOutput and file redirects were mixed. `2>&1` would merge stderr into stdout, then `>file` would write the merged string to the file — losing the fd identity. New approach: when DupOutput and file redirects coexist, build an fd table (fd1/fd2 targets) left-to-right — dup operations copy the current target — then route original stdout/stderr based on the final fd table. Also fixes dev_null tests that were asserting the old incorrect behavior for `echo error >&2 2>/dev/null` (which in real bash sends output to stderr, not /dev/null, because `>&2` copies fd2 before `2>/dev/null` redirects it). --- crates/bashkit/src/interpreter/mod.rs | 209 +++++++++++++++++++++---- crates/bashkit/tests/dev_null_tests.rs | 23 ++- crates/bashkit/tests/issue_853_test.rs | 93 +++++++++++ 3 files changed, 293 insertions(+), 32 deletions(-) create mode 100644 crates/bashkit/tests/issue_853_test.rs diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index 526d18fb..cf1140f6 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -2741,6 +2741,26 @@ impl Interpreter { } } } + +/// Fd target for redirect fd-table modeling. +/// Bash processes redirects left-to-right, building an fd table where each +/// dup copies the *current* target of the source fd. This matters for +/// patterns like `2>&1 >file` where stderr must capture stdout's original +/// destination before stdout is redirected to the file. +#[derive(Clone, Debug)] +enum FdTarget { + /// The original stdout pipe (terminal / command-substitution capture). + Stdout, + /// The original stderr pipe. + Stderr, + /// Write (truncate) to a file. + WriteFile(PathBuf, String), + /// Append to a file. + AppendFile(PathBuf, String), + /// Discard (/dev/null). + DevNull, +} + impl Interpreter { /// Execute a sequence of commands (with errexit checking) async fn execute_command_sequence(&mut self, commands: &[Command]) -> Result { @@ -5437,20 +5457,36 @@ impl Interpreter { mut result: ExecResult, redirects: &[Redirect], ) -> Result { + // Skip the fd-table path when there are no DupOutput redirects mixed + // with file redirects — the simple single-pass logic is sufficient and + // avoids any behavioural delta for the common case. + let has_dup_output = redirects.iter().any(|r| r.kind == RedirectKind::DupOutput); + let has_file_redirect = redirects.iter().any(|r| { + matches!( + r.kind, + RedirectKind::Output + | RedirectKind::Clobber + | RedirectKind::Append + | RedirectKind::OutputBoth + ) + }); + + if has_dup_output && has_file_redirect { + return self.apply_redirections_fd_table(result, redirects).await; + } + + // --- Fast path: no mixed dup+file redirects --- 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); - // Handle /dev/null at interpreter level - cannot be bypassed if is_dev_null(&path) { - // Discard output without calling filesystem match redirect.fd { Some(2) => result.stderr = String::new(), _ => result.stdout = String::new(), } } else { - // noclobber check: > fails if file exists (>| bypasses) if redirect.kind == RedirectKind::Output && self.variables.get("SHOPT_C").map(|v| v.as_str()) == Some("1") && self.fs.stat(&path).await.is_ok() @@ -5461,14 +5497,11 @@ impl Interpreter { result.exit_code = 1; return Ok(result); } - // Check which fd we're redirecting match redirect.fd { Some(2) => { - // 2> - redirect stderr to file if let Err(e) = self.fs.write_file(&path, result.stderr.as_bytes()).await { - // Redirect failed - set exit code and report error result.stderr = format!("bash: {}: {}\n", target_path, e); result.exit_code = 1; return Ok(result); @@ -5476,11 +5509,9 @@ impl Interpreter { result.stderr = String::new(); } _ => { - // Default (stdout) - write stdout to file if let Err(e) = self.fs.write_file(&path, result.stdout.as_bytes()).await { - // Redirect failed - output is lost, set exit code and report error result.stdout = String::new(); result.stderr = format!("bash: {}: {}\n", target_path, e); result.exit_code = 1; @@ -5494,18 +5525,14 @@ impl Interpreter { RedirectKind::Append => { let target_path = self.expand_word(&redirect.target).await?; let path = self.resolve_path(&target_path); - // Handle /dev/null at interpreter level - cannot be bypassed if is_dev_null(&path) { - // Discard output without calling filesystem match redirect.fd { Some(2) => result.stderr = String::new(), _ => result.stdout = String::new(), } } else { - // Check which fd we're appending match redirect.fd { Some(2) => { - // 2>> - append stderr to file if let Err(e) = self.fs.append_file(&path, result.stderr.as_bytes()).await { @@ -5516,11 +5543,9 @@ impl Interpreter { result.stderr = String::new(); } _ => { - // Default (stdout) - append stdout to file if let Err(e) = self.fs.append_file(&path, result.stdout.as_bytes()).await { - // Redirect failed - output is lost result.stdout = String::new(); result.stderr = format!("bash: {}: {}\n", target_path, e); result.exit_code = 1; @@ -5532,16 +5557,12 @@ impl Interpreter { } } RedirectKind::OutputBoth => { - // &> - redirect both stdout and stderr to file let target_path = self.expand_word(&redirect.target).await?; let path = self.resolve_path(&target_path); - // Handle /dev/null at interpreter level - cannot be bypassed if is_dev_null(&path) { - // Discard both outputs without calling filesystem result.stdout = String::new(); result.stderr = String::new(); } else { - // Write both stdout and stderr to file let combined = format!("{}{}", result.stdout, result.stderr); if let Err(e) = self.fs.write_file(&path, combined.as_bytes()).await { result.stderr = format!("bash: {}: {}\n", target_path, e); @@ -5553,40 +5574,170 @@ impl Interpreter { } } RedirectKind::DupOutput => { - // Handle fd duplication (e.g., 2>&1, >&2) 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); match (src_fd, target_fd) { (2, 1) => { - // 2>&1 - redirect stderr to stdout result.stdout.push_str(&result.stderr); result.stderr = String::new(); } (1, 2) => { - // >&2 or 1>&2 - redirect stdout to stderr result.stderr.push_str(&result.stdout); result.stdout = String::new(); } - _ => { - // Other fd duplications not yet supported - } + _ => {} } } RedirectKind::Input | RedirectKind::HereString | RedirectKind::HereDoc - | RedirectKind::HereDocStrip => { - // Input redirections handled in process_input_redirections + | RedirectKind::HereDocStrip => {} + RedirectKind::DupInput => {} + } + } + + Ok(result) + } + + /// Apply redirections using an fd-table model for correct left-to-right + /// ordering when DupOutput and file redirects are mixed (e.g. `2>&1 >file`). + async fn apply_redirections_fd_table( + &mut self, + mut result: ExecResult, + redirects: &[Redirect], + ) -> Result { + // Build fd table: fd1 = stdout pipe, fd2 = stderr pipe + let mut fd1 = FdTarget::Stdout; + let mut fd2 = FdTarget::Stderr; + + 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 redirect.kind == RedirectKind::Output + && self.variables.get("SHOPT_C").map(|v| v.as_str()) == Some("1") + && !is_dev_null(&path) + && 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 = if is_dev_null(&path) { + FdTarget::DevNull + } else { + FdTarget::WriteFile(path, target_path) + }; + match redirect.fd { + Some(2) => fd2 = target, + _ => fd1 = target, + } } - RedirectKind::DupInput => { - // Input fd duplication - handled in process_input_redirections - // for coproc FDs; other cases not yet supported + RedirectKind::Append => { + let target_path = self.expand_word(&redirect.target).await?; + let path = self.resolve_path(&target_path); + let target = if is_dev_null(&path) { + FdTarget::DevNull + } else { + FdTarget::AppendFile(path, target_path) + }; + match redirect.fd { + Some(2) => fd2 = target, + _ => fd1 = target, + } + } + RedirectKind::OutputBoth => { + let target_path = self.expand_word(&redirect.target).await?; + let path = self.resolve_path(&target_path); + let target = if is_dev_null(&path) { + FdTarget::DevNull + } else { + FdTarget::WriteFile(path, target_path) + }; + fd1 = target.clone(); + fd2 = target; + } + 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); + + match (src_fd, target_fd) { + (2, 1) => fd2 = fd1.clone(), + (1, 2) => fd1 = fd2.clone(), + _ => {} + } } + RedirectKind::Input + | RedirectKind::HereString + | RedirectKind::HereDoc + | RedirectKind::HereDocStrip + | RedirectKind::DupInput => {} + } + } + + // Route original stdout/stderr based on final fd table. + // Collect file writes to avoid double-borrow issues. + let orig_stdout = std::mem::take(&mut result.stdout); + let orig_stderr = std::mem::take(&mut result.stderr); + + // Determine what goes where + let mut new_stdout = String::new(); + let mut new_stderr = String::new(); + // file_path -> (content, is_append, display_path) + let mut file_writes: std::collections::HashMap = + std::collections::HashMap::new(); + + for (data, fd_target) in [(&orig_stdout, &fd1), (&orig_stderr, &fd2)].iter() { + if data.is_empty() { + continue; + } + match fd_target { + FdTarget::Stdout => new_stdout.push_str(data), + FdTarget::Stderr => new_stderr.push_str(data), + FdTarget::DevNull => {} + FdTarget::WriteFile(path, display) => { + file_writes + .entry(path.clone()) + .or_insert_with(|| (String::new(), false, display.clone())) + .0 + .push_str(data); + } + FdTarget::AppendFile(path, display) => { + file_writes + .entry(path.clone()) + .or_insert_with(|| (String::new(), true, display.clone())) + .0 + .push_str(data); + } + } + } + + // Write files + for (path, (content, is_append, display_path)) in &file_writes { + let write_result = if *is_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 { + new_stderr = format!("bash: {}: {}\n", display_path, e); + result.exit_code = 1; + result.stdout = new_stdout; + result.stderr = new_stderr; + return Ok(result); } } + result.stdout = new_stdout; + result.stderr = new_stderr; Ok(result) } diff --git a/crates/bashkit/tests/dev_null_tests.rs b/crates/bashkit/tests/dev_null_tests.rs index 96639728..852bde5c 100644 --- a/crates/bashkit/tests/dev_null_tests.rs +++ b/crates/bashkit/tests/dev_null_tests.rs @@ -29,8 +29,21 @@ async fn test_dev_null_stdout_redirect() { #[tokio::test] async fn test_dev_null_stderr_redirect() { let mut bash = Bash::builder().build(); - // Use a command that produces stderr - let result = bash.exec("echo error >&2 2>/dev/null").await.unwrap(); + // Produce real stderr output and redirect it to /dev/null + let result = bash.exec("echo error 2>/dev/null >&2").await.unwrap(); + assert_eq!(result.stderr, ""); + assert_eq!(result.stdout, ""); + assert_eq!(result.exit_code, 0); +} + +#[tokio::test] +async fn test_dev_null_stderr_redirect_direct() { + let mut bash = Bash::builder().build(); + // Direct stderr → /dev/null without fd dup interaction + let result = bash + .exec("f() { echo err >&2; }; f 2>/dev/null") + .await + .unwrap(); assert_eq!(result.stderr, ""); assert_eq!(result.exit_code, 0); } @@ -46,7 +59,11 @@ async fn test_dev_null_append_stdout() { #[tokio::test] async fn test_dev_null_append_stderr() { let mut bash = Bash::builder().build(); - let result = bash.exec("echo error >&2 2>>/dev/null").await.unwrap(); + // Produce real stderr and append-redirect it to /dev/null + let result = bash + .exec("f() { echo err >&2; }; f 2>>/dev/null") + .await + .unwrap(); assert_eq!(result.stderr, ""); assert_eq!(result.exit_code, 0); } diff --git a/crates/bashkit/tests/issue_853_test.rs b/crates/bashkit/tests/issue_853_test.rs new file mode 100644 index 00000000..96894e4e --- /dev/null +++ b/crates/bashkit/tests/issue_853_test.rs @@ -0,0 +1,93 @@ +//! Tests for issue #853: `result=$(cmd 2>&1 >file)` redirect ordering. +//! +//! Bash processes redirects left-to-right. `2>&1 >file` means: +//! 1. stderr → where stdout currently points (the $() capture pipe) +//! 2. stdout → file +//! +//! So `result=$(cmd 2>&1 >file)` captures stderr in result, stdout goes to file. + +use bashkit::Bash; +/// Core reproduction: 2>&1 >file inside command substitution +#[tokio::test] +async fn redirect_2_to_1_then_file_in_cmdsub() { + let mut bash = Bash::new(); + let result = bash + .exec( + r#" +f() { echo "stdout"; echo "stderr" >&2; } +result=$(f 2>&1 >"/tmp/out.txt") +echo "result=[$result]" +echo "file=[$(cat /tmp/out.txt)]" +"#, + ) + .await + .unwrap(); + + let stdout = result.stdout; + // result should capture stderr (because 2>&1 copies stdout's fd which is the capture pipe) + assert!( + stdout.contains("result=[stderr]"), + "expected result=[stderr], got: {stdout}" + ); + // file should contain stdout (because >file redirects stdout to file) + assert!( + stdout.contains("file=[stdout]"), + "expected file=[stdout], got: {stdout}" + ); +} + +/// Simpler case: 2>&1 >file outside command substitution +#[tokio::test] +async fn redirect_2_to_1_then_file_outside_cmdsub() { + let mut bash = Bash::new(); + let result = bash + .exec( + r#" +f() { echo "stdout"; echo "stderr" >&2; } +f 2>&1 >"/tmp/out2.txt" +echo "---" +cat /tmp/out2.txt +"#, + ) + .await + .unwrap(); + + let stdout = result.stdout; + // stderr should go to where stdout was (the terminal/capture) since 2>&1 comes first + // stdout should go to the file since >file comes second + assert!( + stdout.contains("stderr"), + "stderr should appear in stdout: {stdout}" + ); + assert!( + stdout.contains("---\nstdout"), + "file should contain stdout: {stdout}" + ); +} + +/// Reverse order: >file 2>&1 should send both to file +#[tokio::test] +async fn redirect_file_then_2_to_1() { + let mut bash = Bash::new(); + let result = bash + .exec( + r#" +f() { echo "stdout"; echo "stderr" >&2; } +f >"/tmp/out3.txt" 2>&1 +cat /tmp/out3.txt +"#, + ) + .await + .unwrap(); + + let stdout = result.stdout; + // Both should go to file (stdout→file first, then stderr→where stdout points = file) + assert!( + stdout.contains("stdout"), + "file should contain stdout: {stdout}" + ); + assert!( + stdout.contains("stderr"), + "file should contain stderr: {stdout}" + ); +} From 6fb30b5afa5adc2b45b1e9bf0f60cbf64837a4a4 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Fri, 27 Mar 2026 02:17:47 +0000 Subject: [PATCH 2/3] chore: retrigger CI From 9d53a056295b4493117c17f16ae50ed96c5a4b38 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Fri, 27 Mar 2026 02:25:18 +0000 Subject: [PATCH 3/3] chore: update cargo-vet exemptions for bumped dev-dependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit console 0.15.11→0.16.3, insta 1.46.3→1.47.0, simd-adler32 0.3.8→0.3.9 --- supply-chain/config.toml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/supply-chain/config.toml b/supply-chain/config.toml index 020039d2..4caaa71c 100644 --- a/supply-chain/config.toml +++ b/supply-chain/config.toml @@ -235,7 +235,7 @@ version = "0.9.0" criteria = "safe-to-deploy" [[exemptions.console]] -version = "0.15.11" +version = "0.16.3" criteria = "safe-to-run" [[exemptions.convert_case]] @@ -571,7 +571,7 @@ version = "2.13.0" criteria = "safe-to-deploy" [[exemptions.insta]] -version = "1.46.3" +version = "1.47.0" criteria = "safe-to-run" [[exemptions.instant]] @@ -1175,7 +1175,7 @@ version = "0.9.1" criteria = "safe-to-deploy" [[exemptions.simd-adler32]] -version = "0.3.8" +version = "0.3.9" criteria = "safe-to-deploy" [[exemptions.similar]]