From 45eb15a5495df1a14f42e982927c37ed6b6c0f2c Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Fri, 19 Jun 2026 20:27:44 +0000 Subject: [PATCH 1/2] refactor(dirstack): move directory stack to typed interpreter state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pushd/popd/dirs stored the whole directory stack in the user variable namespace as _DIRSTACK_SIZE / _DIRSTACK_N, guarded by is_internal_variable() — a data structure leaked into shell variables that scripts could forge. Move it to a typed dir_stack: Arc> on ScopedState (so it is subshell-isolated via the scoped snapshot like aliases/traps) and expose it to the pushd/popd/dirs builtins via ShellRef::dir_stack. Add it to ShellState (serde default) + restore so it roundtrips through session snapshots, and drop _DIRSTACK_ from the is_internal_variable() blocklist and its test list. The DoS size cap now applies to pushd growth instead of clamping a user-forgeable size variable. Replace the magic-variable-inspecting unit tests with interpreter-level spec coverage (dirstack.test.sh), adding dirs -v and a regression that the former _DIRSTACK_* names are now inert ordinary variables. Adds a ShellState dir-stack roundtrip test. Concludes the #1 migration of magic-variable channels to typed state. --- crates/bashkit/src/builtins/dirstack.rs | 464 ++++-------------- crates/bashkit/src/interpreter/mod.rs | 45 +- .../tests/spec_cases/bash/dirstack.test.sh | 30 ++ 3 files changed, 159 insertions(+), 380 deletions(-) diff --git a/crates/bashkit/src/builtins/dirstack.rs b/crates/bashkit/src/builtins/dirstack.rs index 65e48b11f..fcab48726 100644 --- a/crates/bashkit/src/builtins/dirstack.rs +++ b/crates/bashkit/src/builtins/dirstack.rs @@ -1,6 +1,10 @@ //! Directory stack builtins - pushd, popd, dirs //! -//! Stack stored in variables: _DIRSTACK_SIZE, _DIRSTACK_0, _DIRSTACK_1, etc. +//! The stack is typed interpreter state (`ShellRef::dir_stack`), reached via +//! `ctx.shell`. It is bottom-to-top: index 0 is the oldest entry, the last +//! element is the most recently pushed. The current directory (`cwd`) is not +//! part of the vec. (It used to live in the user variable namespace as +//! `_DIRSTACK_SIZE` / `_DIRSTACK_N`, which let scripts forge it.) use async_trait::async_trait; use std::path::PathBuf; @@ -10,41 +14,6 @@ use super::{Builtin, Context}; use crate::error::Result; use crate::interpreter::ExecResult; -fn get_stack_size(ctx: &Context<'_>) -> usize { - ctx.variables - .get("_DIRSTACK_SIZE") - .and_then(|s| s.parse().ok()) - .map(|size: usize| size.min(MAX_DIRSTACK_SIZE)) - .unwrap_or(0) -} - -fn get_stack_entry(ctx: &Context<'_>, idx: usize) -> Option { - ctx.variables.get(&format!("_DIRSTACK_{}", idx)).cloned() -} - -fn set_stack_size(ctx: &mut Context<'_>, size: usize) { - ctx.variables - .insert("_DIRSTACK_SIZE".to_string(), size.to_string()); -} - -fn push_stack(ctx: &mut Context<'_>, dir: &str) { - let size = get_stack_size(ctx); - ctx.variables - .insert(format!("_DIRSTACK_{}", size), dir.to_string()); - set_stack_size(ctx, size + 1); -} - -fn pop_stack(ctx: &mut Context<'_>) -> Option { - let size = get_stack_size(ctx); - if size == 0 { - return None; - } - let entry = get_stack_entry(ctx, size - 1); - ctx.variables.remove(&format!("_DIRSTACK_{}", size - 1)); - set_stack_size(ctx, size - 1); - entry -} - fn normalize_path(base: &std::path::Path, target: &str) -> PathBuf { let path = if target.starts_with('/') { PathBuf::from(target) @@ -54,68 +23,86 @@ fn normalize_path(base: &std::path::Path, target: &str) -> PathBuf { super::resolve_path(&PathBuf::from("/"), &path.to_string_lossy()) } +/// Format the stack as bash's `dirs` default: current dir followed by the +/// stack from top (most recent) to bottom. +fn format_stack(ctx: &Context<'_>) -> String { + let cwd = ctx.cwd.to_string_lossy().to_string(); + let mut parts = vec![cwd]; + if let Some(shell) = ctx.shell.as_ref() { + parts.extend(shell.dir_stack.iter().rev().cloned()); + } + parts.join(" ") +} + /// The pushd builtin - push directory onto stack and cd. /// /// Usage: pushd [dir] /// -/// Without args, swaps top two directories. -/// With dir, pushes current dir onto stack and cd to dir. +/// Without args, swaps current dir with the top of the stack. +/// With dir, pushes current dir onto the stack and cd to dir. pub struct Pushd; #[async_trait] impl Builtin for Pushd { async fn execute(&self, mut ctx: Context<'_>) -> Result { if ctx.args.is_empty() { - // Swap top two: current dir <-> top of stack - let top = pop_stack(&mut ctx); - match top { - Some(dir) => { - let old_cwd = ctx.cwd.to_string_lossy().to_string(); - let new_path = normalize_path(ctx.cwd, &dir); - if ctx.fs.exists(&new_path).await.unwrap_or(false) { - push_stack(&mut ctx, &old_cwd); - *ctx.cwd = new_path; - // Print stack - let output = format_stack(&ctx); - Ok(ExecResult::ok(format!("{}\n", output))) - } else { - // Restore stack - push_stack(&mut ctx, &dir); - Ok(ExecResult::err( - format!("pushd: {}: No such file or directory\n", dir), - 1, - )) - } - } - None => Ok(ExecResult::err( + // Swap current dir with the top of the stack. + let Some(top) = ctx.shell.as_ref().and_then(|s| s.dir_stack.last()).cloned() else { + return Ok(ExecResult::err( "pushd: no other directory\n".to_string(), 1, - )), + )); + }; + let new_path = normalize_path(ctx.cwd, &top); + if !ctx.fs.exists(&new_path).await.unwrap_or(false) { + return Ok(ExecResult::err( + format!("pushd: {}: No such file or directory\n", top), + 1, + )); + } + let old_cwd = ctx.cwd.to_string_lossy().to_string(); + if let Some(shell) = ctx.shell.as_mut() { + shell.dir_stack.pop(); + shell.dir_stack.push(old_cwd); } + *ctx.cwd = new_path; + Ok(ExecResult::ok(format!("{}\n", format_stack(&ctx)))) } else { - let target = &ctx.args[0].clone(); - let new_path = normalize_path(ctx.cwd, target); + let target = ctx.args[0].clone(); + let new_path = normalize_path(ctx.cwd, &target); - if ctx.fs.exists(&new_path).await.unwrap_or(false) { - let meta = ctx.fs.stat(&new_path).await; - if meta.map(|m| m.file_type.is_dir()).unwrap_or(false) { - let old_cwd = ctx.cwd.to_string_lossy().to_string(); - push_stack(&mut ctx, &old_cwd); - *ctx.cwd = new_path; - let output = format_stack(&ctx); - Ok(ExecResult::ok(format!("{}\n", output))) - } else { - Ok(ExecResult::err( - format!("pushd: {}: Not a directory\n", target), - 1, - )) - } - } else { - Ok(ExecResult::err( + if !ctx.fs.exists(&new_path).await.unwrap_or(false) { + return Ok(ExecResult::err( format!("pushd: {}: No such file or directory\n", target), 1, - )) + )); + } + let is_dir = ctx + .fs + .stat(&new_path) + .await + .map(|m| m.file_type.is_dir()) + .unwrap_or(false); + if !is_dir { + return Ok(ExecResult::err( + format!("pushd: {}: Not a directory\n", target), + 1, + )); + } + + let old_cwd = ctx.cwd.to_string_lossy().to_string(); + if let Some(shell) = ctx.shell.as_mut() { + // DoS guard: cap stack growth (the user can no longer forge size). + if shell.dir_stack.len() >= MAX_DIRSTACK_SIZE { + return Ok(ExecResult::err( + "pushd: directory stack full\n".to_string(), + 1, + )); + } + shell.dir_stack.push(old_cwd); } + *ctx.cwd = new_path; + Ok(ExecResult::ok(format!("{}\n", format_stack(&ctx)))) } } } @@ -124,24 +111,20 @@ impl Builtin for Pushd { /// /// Usage: popd /// -/// Removes top directory from stack and cd to it. +/// Removes the top directory from the stack and cd to it. pub struct Popd; #[async_trait] impl Builtin for Popd { async fn execute(&self, mut ctx: Context<'_>) -> Result { - match pop_stack(&mut ctx) { - Some(dir) => { - let new_path = normalize_path(ctx.cwd, &dir); - *ctx.cwd = new_path; - let output = format_stack(&ctx); - Ok(ExecResult::ok(format!("{}\n", output))) - } - None => Ok(ExecResult::err( + let Some(dir) = ctx.shell.as_mut().and_then(|s| s.dir_stack.pop()) else { + return Ok(ExecResult::err( "popd: directory stack empty\n".to_string(), 1, - )), - } + )); + }; + *ctx.cwd = normalize_path(ctx.cwd, &dir); + Ok(ExecResult::ok(format!("{}\n", format_stack(&ctx)))) } } @@ -176,307 +159,34 @@ impl Builtin for Dirs { } if clear { - let size = get_stack_size(&ctx); - for i in 0..size { - ctx.variables.remove(&format!("_DIRSTACK_{}", i)); + if let Some(shell) = ctx.shell.as_mut() { + shell.dir_stack.clear(); } - set_stack_size(&mut ctx, 0); return Ok(ExecResult::ok(String::new())); } let cwd = ctx.cwd.to_string_lossy().to_string(); - let size = get_stack_size(&ctx); + // Stack from top (most recent) to bottom. + let stack: Vec = ctx + .shell + .as_ref() + .map(|s| s.dir_stack.iter().rev().cloned().collect()) + .unwrap_or_default(); if verbose { let mut output = format!(" 0 {}\n", cwd); - for i in (0..size).rev() { - if let Some(dir) = get_stack_entry(&ctx, i) { - output.push_str(&format!(" {} {}\n", size - i, dir)); - } + for (n, dir) in stack.iter().enumerate() { + output.push_str(&format!(" {} {}\n", n + 1, dir)); } Ok(ExecResult::ok(output)) } else if per_line { let mut output = format!("{}\n", cwd); - for i in (0..size).rev() { - if let Some(dir) = get_stack_entry(&ctx, i) { - output.push_str(&format!("{}\n", dir)); - } + for dir in &stack { + output.push_str(&format!("{}\n", dir)); } Ok(ExecResult::ok(output)) } else { - let output = format_stack(&ctx); - Ok(ExecResult::ok(format!("{}\n", output))) - } - } -} - -fn format_stack(ctx: &Context<'_>) -> String { - let cwd = ctx.cwd.to_string_lossy().to_string(); - let size = get_stack_size(ctx); - let mut parts = vec![cwd]; - for i in (0..size).rev() { - if let Some(dir) = get_stack_entry(ctx, i) { - parts.push(dir); + Ok(ExecResult::ok(format!("{}\n", format_stack(&ctx)))) } } - parts.join(" ") -} - -#[cfg(test)] -mod tests { - use super::*; - use std::collections::HashMap; - use std::path::Path; - use std::sync::Arc; - - use crate::fs::{FileSystem, InMemoryFs}; - - async fn setup() -> (Arc, PathBuf, HashMap) { - let fs = Arc::new(InMemoryFs::new()); - let cwd = PathBuf::from("/home/user"); - let variables = HashMap::new(); - fs.mkdir(&cwd, true).await.unwrap(); - fs.mkdir(Path::new("/tmp"), true).await.unwrap(); - fs.mkdir(Path::new("/var"), true).await.unwrap(); - (fs, cwd, variables) - } - - // ==================== pushd ==================== - - #[tokio::test] - async fn pushd_to_directory() { - let (fs, mut cwd, mut variables) = setup().await; - let env = HashMap::new(); - let args = vec!["/tmp".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - let result = Pushd.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 0); - assert_eq!(cwd, PathBuf::from("/tmp")); - // Stack should have old cwd - assert_eq!(variables.get("_DIRSTACK_0").unwrap(), "/home/user"); - } - - #[tokio::test] - async fn pushd_nonexistent_dir() { - let (fs, mut cwd, mut variables) = setup().await; - let env = HashMap::new(); - let args = vec!["/nonexistent".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - let result = Pushd.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 1); - assert!(result.stderr.contains("No such file or directory")); - // cwd unchanged - assert_eq!(cwd, PathBuf::from("/home/user")); - } - - #[tokio::test] - async fn pushd_file_not_dir() { - let (fs, mut cwd, mut variables) = setup().await; - fs.write_file(Path::new("/home/user/file.txt"), b"data") - .await - .unwrap(); - let env = HashMap::new(); - let args = vec!["file.txt".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - let result = Pushd.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 1); - assert!(result.stderr.contains("Not a directory")); - } - - #[tokio::test] - async fn pushd_no_args_empty_stack() { - let (fs, mut cwd, mut variables) = setup().await; - let env = HashMap::new(); - let args: Vec = vec![]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - let result = Pushd.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 1); - assert!(result.stderr.contains("no other directory")); - } - - #[tokio::test] - async fn pushd_no_args_swaps_top() { - let (fs, mut cwd, mut variables) = setup().await; - // Push /tmp first so stack has an entry - let env = HashMap::new(); - let args = vec!["/tmp".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - Pushd.execute(ctx).await.unwrap(); - assert_eq!(cwd, PathBuf::from("/tmp")); - - // Now pushd with no args should swap - let args: Vec = vec![]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - let result = Pushd.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 0); - assert_eq!(cwd, PathBuf::from("/home/user")); - } - - // ==================== popd ==================== - - #[tokio::test] - async fn popd_empty_stack() { - let (fs, mut cwd, mut variables) = setup().await; - let env = HashMap::new(); - let args: Vec = vec![]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - let result = Popd.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 1); - assert!(result.stderr.contains("directory stack empty")); - } - - #[tokio::test] - async fn popd_after_pushd() { - let (fs, mut cwd, mut variables) = setup().await; - let env = HashMap::new(); - - // pushd /tmp - let args = vec!["/tmp".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - Pushd.execute(ctx).await.unwrap(); - assert_eq!(cwd, PathBuf::from("/tmp")); - - // popd - let args: Vec = vec![]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - let result = Popd.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 0); - assert_eq!(cwd, PathBuf::from("/home/user")); - } - - #[tokio::test] - async fn pushd_popd_multiple() { - let (fs, mut cwd, mut variables) = setup().await; - let env = HashMap::new(); - - // pushd /tmp - let args = vec!["/tmp".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - Pushd.execute(ctx).await.unwrap(); - - // pushd /var - let args = vec!["/var".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - Pushd.execute(ctx).await.unwrap(); - assert_eq!(cwd, PathBuf::from("/var")); - - // popd -> /tmp - let args: Vec = vec![]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - Popd.execute(ctx).await.unwrap(); - assert_eq!(cwd, PathBuf::from("/tmp")); - - // popd -> /home/user - let args: Vec = vec![]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - Popd.execute(ctx).await.unwrap(); - assert_eq!(cwd, PathBuf::from("/home/user")); - } - - // ==================== dirs ==================== - - #[tokio::test] - async fn dirs_empty_stack() { - let (fs, mut cwd, mut variables) = setup().await; - let env = HashMap::new(); - let args: Vec = vec![]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - let result = Dirs.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 0); - assert!(result.stdout.contains("/home/user")); - } - - #[tokio::test] - async fn dirs_after_pushd() { - let (fs, mut cwd, mut variables) = setup().await; - let env = HashMap::new(); - - // pushd /tmp - let args = vec!["/tmp".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - Pushd.execute(ctx).await.unwrap(); - - // dirs - let args: Vec = vec![]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - let result = Dirs.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 0); - assert!(result.stdout.contains("/tmp")); - assert!(result.stdout.contains("/home/user")); - } - - #[tokio::test] - async fn dirs_clear() { - let (fs, mut cwd, mut variables) = setup().await; - let env = HashMap::new(); - - // pushd /tmp - let args = vec!["/tmp".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - Pushd.execute(ctx).await.unwrap(); - - // dirs -c - let args = vec!["-c".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - let result = Dirs.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 0); - assert_eq!(get_stack_size_from_vars(&variables), 0); - } - - #[tokio::test] - async fn dirs_per_line() { - let (fs, mut cwd, mut variables) = setup().await; - let env = HashMap::new(); - - // pushd /tmp - let args = vec!["/tmp".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - Pushd.execute(ctx).await.unwrap(); - - // dirs -p - let args = vec!["-p".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - let result = Dirs.execute(ctx).await.unwrap(); - let lines: Vec<&str> = result.stdout.lines().collect(); - assert_eq!(lines.len(), 2); - } - - #[tokio::test] - async fn dirs_verbose() { - let (fs, mut cwd, mut variables) = setup().await; - let env = HashMap::new(); - - // pushd /tmp - let args = vec!["/tmp".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - Pushd.execute(ctx).await.unwrap(); - - // dirs -v - let args = vec!["-v".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - let result = Dirs.execute(ctx).await.unwrap(); - // Verbose format has numbered entries - assert!(result.stdout.contains(" 0 ")); - assert!(result.stdout.contains(" 1 ")); - } - - #[tokio::test] - async fn dirs_limits_user_declared_size() { - let (fs, mut cwd, mut variables) = setup().await; - let env = HashMap::new(); - variables.insert("_DIRSTACK_SIZE".to_string(), "999999999999".to_string()); - variables.insert("_DIRSTACK_0".to_string(), "/tmp".to_string()); - - let args = vec!["-p".to_string()]; - let ctx = Context::new_for_test(&args, &env, &mut variables, &mut cwd, fs.clone(), None); - let result = Dirs.execute(ctx).await.unwrap(); - assert_eq!(result.exit_code, 0); - assert_eq!(result.stdout, "/home/user\n/tmp\n"); - } - - fn get_stack_size_from_vars(vars: &HashMap) -> usize { - vars.get("_DIRSTACK_SIZE") - .and_then(|s| s.parse().ok()) - .unwrap_or(0) - } } diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index 8e6038235..c353154b0 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -304,6 +304,9 @@ pub(crate) struct ShellRef<'a> { pub(crate) var_attrs: &'a mut HashMap, /// Nameref bindings (`declare -n`). Mutable so `unset -n` can clear them. pub(crate) namerefs: &'a mut HashMap, + /// Directory stack for `pushd`/`popd`/`dirs`. Direct mutable access backed + /// by `Arc::make_mut` of the parent's Arc-wrapped stack. + pub(crate) dir_stack: &'a mut Vec, /// Registered builtin commands (read-only, accessed via `has_builtin`). pub(crate) builtins: &'a HashMap>, /// Host-owned builtin registry, when configured (read-only). Needed so @@ -590,7 +593,6 @@ pub(crate) fn is_internal_variable(name: &str) -> bool { || name.starts_with("_LOWER_") || name.starts_with("_INTEGER_") || name.starts_with("_ARRAY_READ_") - || name.starts_with("_DIRSTACK_") || name == "_SHIFT_COUNT" || name == "_SET_POSITIONAL" } @@ -735,6 +737,9 @@ pub struct ShellState { pub aliases: HashMap, /// Trap handlers pub traps: HashMap, + /// Directory stack (`pushd`/`popd`/`dirs`); bottom-to-top, excluding `cwd`. + #[serde(default)] + pub dir_stack: Vec, } /// Lightweight inspection view of shell state. @@ -987,6 +992,11 @@ struct ScopedState { traps: Arc>, /// Shell aliases: name -> expansion value. aliases: Arc>, + /// Directory stack for `pushd`/`popd`/`dirs`. Index 0 is the bottom of the + /// stack; the top (most recently pushed) is last. The current directory is + /// `cwd`, not part of this vec. Previously stored as `_DIRSTACK_*` shell + /// variables; now typed state so it can't be forged from a script. + dir_stack: Arc>, } /// All interpreter state mutated inside a `$(...)` / arithmetic substitution @@ -2086,6 +2096,7 @@ impl Interpreter { }, aliases: (*self.scoped.aliases).clone(), traps: (*self.scoped.traps).clone(), + dir_stack: (*self.scoped.dir_stack).clone(), } } @@ -2161,6 +2172,7 @@ impl Interpreter { self.scoped.functions = Arc::new(restored_functions); self.scoped.aliases = Arc::new(state.aliases.clone()); self.scoped.traps = Arc::new(state.traps.clone()); + self.scoped.dir_stack = Arc::new(state.dir_stack.clone()); // Recompute memory budget from restored state to prevent desync let func_count = self.scoped.functions.len(); let func_bytes: usize = self @@ -5752,6 +5764,7 @@ impl Interpreter { traps: Arc::make_mut(&mut self.scoped.traps), var_attrs: Arc::make_mut(&mut self.scoped.var_attrs), namerefs: Arc::make_mut(&mut self.scoped.namerefs), + dir_stack: Arc::make_mut(&mut self.scoped.dir_stack), call_stack: &self.call_stack, history: &self.history, limits: &self.limits, @@ -5805,6 +5818,7 @@ impl Interpreter { traps: Arc::make_mut(&mut self.scoped.traps), var_attrs: Arc::make_mut(&mut self.scoped.var_attrs), namerefs: Arc::make_mut(&mut self.scoped.namerefs), + dir_stack: Arc::make_mut(&mut self.scoped.dir_stack), call_stack: &self.call_stack, history: &self.history, limits: &self.limits, @@ -11971,8 +11985,6 @@ cat /tmp/test_fd_exec_public.txt"#, "_ARRAY_READ_a", "_SHIFT_COUNT", "_SET_POSITIONAL", - "_DIRSTACK_SIZE", - "_DIRSTACK_0", "SHOPT_e", "SHOPT_x", "SHOPT_expand_aliases", @@ -12042,6 +12054,31 @@ cat /tmp/test_fd_exec_public.txt"#, assert_eq!(out.stdout.trim(), bang.unwrap()); } + #[tokio::test] + async fn test_shell_state_roundtrips_dir_stack() { + let fs = Arc::new(InMemoryFs::new()); + fs.mkdir(Path::new("/tmp"), true).await.unwrap(); + let mut interp = Interpreter::new(fs); + let ast = Parser::new("cd /tmp; pushd /tmp >/dev/null") + .parse() + .unwrap(); + interp.execute(&ast).await.unwrap(); + assert_eq!(&*interp.scoped.dir_stack, &["/tmp".to_string()]); + + let state = interp.shell_state(); + assert_eq!(state.dir_stack, vec!["/tmp".to_string()]); + + let restored_fs = Arc::new(InMemoryFs::new()); + let mut restored = Interpreter::new(restored_fs); + restored.restore_shell_state(&state); + assert_eq!(&*restored.scoped.dir_stack, &["/tmp".to_string()]); + let out = restored + .execute(&Parser::new("dirs").parse().unwrap()) + .await + .unwrap(); + assert!(out.stdout.contains("/tmp")); + } + #[tokio::test] async fn test_restore_shell_state_migrates_legacy_nameref_targets() { let state = ShellState { @@ -12061,6 +12098,7 @@ cat /tmp/test_fd_exec_public.txt"#, functions: HashMap::new(), aliases: HashMap::new(), traps: HashMap::new(), + dir_stack: Vec::new(), }; let mut restored = Interpreter::new(Arc::new(InMemoryFs::new())); @@ -12099,6 +12137,7 @@ cat /tmp/test_fd_exec_public.txt"#, functions: HashMap::new(), aliases: HashMap::new(), traps: HashMap::new(), + dir_stack: Vec::new(), }; interp.restore_shell_state(&clean_state); diff --git a/crates/bashkit/tests/spec_cases/bash/dirstack.test.sh b/crates/bashkit/tests/spec_cases/bash/dirstack.test.sh index 7fff37eff..bbd92aa0e 100644 --- a/crates/bashkit/tests/spec_cases/bash/dirstack.test.sh +++ b/crates/bashkit/tests/spec_cases/bash/dirstack.test.sh @@ -130,3 +130,33 @@ pwd ### expect /tmp ### end + +### dirs_verbose +# dirs -v shows numbered entries, current dir at index 0 +### bash_diff +mkdir -p /tmp/dv1 /tmp/dv2 +cd /tmp +pushd /tmp/dv1 > /dev/null +pushd /tmp/dv2 > /dev/null +dirs -v +### expect + 0 /tmp/dv2 + 1 /tmp/dv1 + 2 /tmp +### end + +### dirstack_internal_names_are_ordinary_vars +# _DIRSTACK_SIZE / _DIRSTACK_0 are no longer the stack's storage; the stack is +# typed interpreter state, so user variables of those names are inert. +### bash_diff +mkdir -p /tmp/di1 +cd /tmp +_DIRSTACK_SIZE=99 +_DIRSTACK_0=/forged +pushd /tmp/di1 > /dev/null +dirs +echo "vars=$_DIRSTACK_SIZE $_DIRSTACK_0" +### expect +/tmp/di1 /tmp +vars=99 /forged +### end From ab80a7dfd74db4c6fd30071c224c7f5c901be9cd Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Fri, 19 Jun 2026 20:33:43 +0000 Subject: [PATCH 2/2] refactor(dirstack): single stat in pushd, avoid stack clone in dirs Review nits: - pushd used exists() + stat() with unwrap_or(false), misreporting IO/TOCTOU errors as 'Not a directory'. Use one stat() and branch on Ok(dir)/Ok(non-dir)/Err(not found). - dirs cloned the whole stack into a Vec even on the default path that doesn't use it. Return early for the default output and iterate the borrowed dir_stack (reversed) for -p/-v without allocating. --- crates/bashkit/src/builtins/dirstack.rs | 56 ++++++++++++------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/crates/bashkit/src/builtins/dirstack.rs b/crates/bashkit/src/builtins/dirstack.rs index fcab48726..65c6bfd9d 100644 --- a/crates/bashkit/src/builtins/dirstack.rs +++ b/crates/bashkit/src/builtins/dirstack.rs @@ -71,23 +71,23 @@ impl Builtin for Pushd { let target = ctx.args[0].clone(); let new_path = normalize_path(ctx.cwd, &target); - if !ctx.fs.exists(&new_path).await.unwrap_or(false) { - return Ok(ExecResult::err( - format!("pushd: {}: No such file or directory\n", target), - 1, - )); - } - let is_dir = ctx - .fs - .stat(&new_path) - .await - .map(|m| m.file_type.is_dir()) - .unwrap_or(false); - if !is_dir { - return Ok(ExecResult::err( - format!("pushd: {}: Not a directory\n", target), - 1, - )); + // Single stat: distinguish "not found" from "not a directory" without + // a redundant exists() + stat() pair (which would misreport IO/TOCTOU + // errors as "Not a directory"). + match ctx.fs.stat(&new_path).await { + Ok(meta) if meta.file_type.is_dir() => {} + Ok(_) => { + return Ok(ExecResult::err( + format!("pushd: {}: Not a directory\n", target), + 1, + )); + } + Err(_) => { + return Ok(ExecResult::err( + format!("pushd: {}: No such file or directory\n", target), + 1, + )); + } } let old_cwd = ctx.cwd.to_string_lossy().to_string(); @@ -165,28 +165,28 @@ impl Builtin for Dirs { return Ok(ExecResult::ok(String::new())); } + if !verbose && !per_line { + // Default output doesn't need to walk the stack separately. + return Ok(ExecResult::ok(format!("{}\n", format_stack(&ctx)))); + } + let cwd = ctx.cwd.to_string_lossy().to_string(); - // Stack from top (most recent) to bottom. - let stack: Vec = ctx - .shell - .as_ref() - .map(|s| s.dir_stack.iter().rev().cloned().collect()) - .unwrap_or_default(); + // Stack from top (most recent) to bottom, borrowed (no clone). + let empty: Vec = Vec::new(); + let stack = ctx.shell.as_ref().map(|s| &*s.dir_stack).unwrap_or(&empty); if verbose { let mut output = format!(" 0 {}\n", cwd); - for (n, dir) in stack.iter().enumerate() { + for (n, dir) in stack.iter().rev().enumerate() { output.push_str(&format!(" {} {}\n", n + 1, dir)); } Ok(ExecResult::ok(output)) - } else if per_line { + } else { let mut output = format!("{}\n", cwd); - for dir in &stack { + for dir in stack.iter().rev() { output.push_str(&format!("{}\n", dir)); } Ok(ExecResult::ok(output)) - } else { - Ok(ExecResult::ok(format!("{}\n", format_stack(&ctx)))) } } }