From 9665b3de6a0dbfa77e9b139a012ecac509d6017c Mon Sep 17 00:00:00 2001 From: "Guillem L. Jara" <4lon3ly0@tutanota.com> Date: Mon, 23 Mar 2026 17:10:21 +0100 Subject: [PATCH 1/4] fix(ls): properly release parent nodes in DFS Fixes a regression introduced in #11386 (sorry) where DFS nodes did not properly free the recorded inodes of the parent directories, causing critical errors in some instances of recursive symlinks. --- src/uu/ls/src/ls.rs | 55 ++++++++++++++++++++----------- src/uucore/src/lib/features/fs.rs | 1 + 2 files changed, 37 insertions(+), 19 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index f689f5f6865..c9ca59caa74 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -974,7 +974,11 @@ impl Colorable for PathData<'_> { } } -type DirData = (PathBuf, bool); +struct LsNode { + path: PathBuf, + needs_blank_line: bool, + fi: FileInformation, +} // A struct to encapsulate state that is passed around from `list` functions. #[cfg_attr(not(unix), allow(dead_code))] @@ -995,7 +999,7 @@ struct ListState<'a> { #[cfg(not(unix))] gid_cache: (), recent_time_range: RangeInclusive, - stack: Vec, + stack: Vec, listed_ancestors: FxHashSet, initial_locs_len: usize, display_buf: Vec, @@ -1109,22 +1113,19 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { )?; // Only runs if it must list recursively. - while let Some(dir_data) = state.stack.pop() { - let read_dir = match fs::read_dir(&dir_data.0) { + while let Some(node) = state.stack.pop() { + let read_dir = match fs::read_dir(&node.path) { Err(err) => { // flush stdout buffer before the error to preserve formatting and order state.out.flush()?; - show!(LsError::IOErrorContext( - path_data.path().to_path_buf(), - err, - path_data.command_line - )); + show!(LsError::IOErrorContext(node.path, err, false)); continue; } Ok(rd) => rd, }; - depth_first_list(dir_data, read_dir, config, &mut state, &mut dired, false)?; + state.listed_ancestors.remove(&node.fi); + depth_first_list(node.into(), read_dir, config, &mut state, &mut dired, false)?; // Heuristic to ensure stack does not keep its capacity forever if there is // combinatorial explosion; we decrease it logarithmically here. @@ -1208,14 +1209,14 @@ fn sort_entries(entries: &mut [PathData], config: &Config) { } fn depth_first_list( - (dir_path, needs_blank_line): DirData, + (dir_path, needs_blank_line): (PathBuf, bool), mut read_dir: ReadDir, config: &Config, state: &mut ListState, dired: &mut DiredOutput, is_top_level: bool, ) -> UResult<()> { - let path_data = PathData::new(dir_path.as_path().into(), None, None, config, false); + let path_data = PathData::new(dir_path.into(), None, None, config, false); // Print dir heading - name... 'total' comes after error display if state.initial_locs_len > 1 || config.recursive { @@ -1253,7 +1254,7 @@ fn depth_first_list( } // Append entries with initial dot files and record their existence - let (ref mut buf, trim) = if config.files == Files::All { + let (mut buf, trim) = if config.files == Files::All { const DOT_DIRECTORIES: usize = 2; let v = vec![ PathData::new( @@ -1299,20 +1300,20 @@ fn depth_first_list( // Relinquish unused space since we won't need it anymore. buf.shrink_to_fit(); - sort_entries(buf, config); + sort_entries(&mut buf, config); if config.format == Format::Long || config.alloc_size { - let total = write_total(buf, config, &mut state.out)?; + let total = write_total(&buf, config, &mut state.out)?; if config.dired { dired::add_total(dired, total); } } - display_items(buf, config, state, dired)?; + display_items(&buf, config, state, dired)?; if config.recursive { for e in buf - .iter() + .into_iter() .skip(trim) .filter(|p| p.file_type().is_some_and(FileType::is_dir)) .rev() @@ -1327,13 +1328,13 @@ fn depth_first_list( )); } else { let fi = FileInformation::from_path(e.path(), e.must_dereference)?; - if state.listed_ancestors.insert(fi) { + if state.listed_ancestors.insert(fi.clone()) { // Push to stack, but with a less aggressive growth curve. let (cap, len) = (state.stack.capacity(), state.stack.len()); if cap == len { state.stack.reserve_exact(len / 4 + 4); } - state.stack.push((e.path().to_path_buf(), true)); + state.stack.push(LsNode::new(e.p_buf, needs_blank_line, fi)); } else { state.out.flush()?; show!(LsError::AlreadyListedError(e.path().to_path_buf())); @@ -1344,6 +1345,22 @@ fn depth_first_list( Ok(()) } +impl LsNode { + fn new(path: impl Into, needs_blank_line: bool, fi: FileInformation) -> Self { + Self { + path: path.into(), + needs_blank_line, + fi, + } + } +} + +impl From for (PathBuf, bool) { + fn from(val: LsNode) -> Self { + (val.path, val.needs_blank_line) + } +} + fn get_metadata_with_deref_opt(p_buf: &Path, dereference: bool) -> std::io::Result { if dereference { p_buf.metadata() diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index 5e10f2ea513..03aec6bf1e1 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -46,6 +46,7 @@ macro_rules! has { } /// Information to uniquely identify a file +#[derive(Clone)] pub struct FileInformation( #[cfg(unix)] nix::sys::stat::FileStat, #[cfg(windows)] winapi_util::file::Information, From 91b1ab370dffc0d24c7b3adc1cca5601c853bfb0 Mon Sep 17 00:00:00 2001 From: "Guillem L. Jara" <4lon3ly0@tutanota.com> Date: Tue, 24 Mar 2026 14:42:52 +0100 Subject: [PATCH 2/4] fix(ls): properly skip . & .. on --recursive Depending on the sorting it would not, so it could recurse back through `..`. --- src/uu/ls/src/display.rs | 1 + src/uu/ls/src/ls.rs | 23 +++++++++++++---------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/uu/ls/src/display.rs b/src/uu/ls/src/display.rs index 929ce7e4610..ae831644528 100644 --- a/src/uu/ls/src/display.rs +++ b/src/uu/ls/src/display.rs @@ -765,6 +765,7 @@ fn display_item_name( target_path.file_name().map(Cow::Borrowed), config, false, + false, ); // Check if the target actually needs coloring diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index c9ca59caa74..9651b4558c3 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -810,6 +810,7 @@ struct PathData<'a> { p_buf: Cow<'a, Path>, must_dereference: bool, command_line: bool, + synthetic: bool, } impl<'a> PathData<'a> { @@ -819,6 +820,7 @@ impl<'a> PathData<'a> { file_name: Option>, config: &Config, command_line: bool, + synthetic: bool, ) -> Self { // We cannot use `Path::ends_with` or `Path::Components`, because they remove occurrences of '.' // For '..', the filename is None @@ -885,6 +887,7 @@ impl<'a> PathData<'a> { p_buf, must_dereference, command_line, + synthetic, } } @@ -1039,7 +1042,7 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { }; for loc in locs { - let path_data = PathData::new(loc.into(), None, None, config, true); + let path_data = PathData::new(loc.into(), None, None, config, true, false); // Getting metadata here is no big deal as it's just the CWD // and we really just want to know if the strings exist as files/dirs @@ -1216,7 +1219,7 @@ fn depth_first_list( dired: &mut DiredOutput, is_top_level: bool, ) -> UResult<()> { - let path_data = PathData::new(dir_path.into(), None, None, config, false); + let path_data = PathData::new(dir_path.into(), None, None, config, false, false); // Print dir heading - name... 'total' comes after error display if state.initial_locs_len > 1 || config.recursive { @@ -1254,15 +1257,15 @@ fn depth_first_list( } // Append entries with initial dot files and record their existence - let (mut buf, trim) = if config.files == Files::All { - const DOT_DIRECTORIES: usize = 2; - let v = vec![ + let mut buf = if config.files == Files::All { + vec![ PathData::new( path_data.path().into(), None, Some(OsStr::new(".").into()), config, false, + true, ), PathData::new( path_data.path().join("..").into(), @@ -1270,11 +1273,11 @@ fn depth_first_list( Some(OsStr::new("..").into()), config, false, + true, ), - ]; - (v, DOT_DIRECTORIES) + ] } else { - (Vec::new(), 0) + Vec::new() }; // Convert those entries to the PathData struct @@ -1288,6 +1291,7 @@ fn depth_first_list( None, config, false, + false, )); } } @@ -1314,8 +1318,7 @@ fn depth_first_list( if config.recursive { for e in buf .into_iter() - .skip(trim) - .filter(|p| p.file_type().is_some_and(FileType::is_dir)) + .filter(|p| p.file_type().is_some_and(FileType::is_dir) && !p.synthetic) .rev() { // Try to open only to report any errors in order to match GNU semantics. From 6f3c5da2e19c644e59966818640eb6b5caea619a Mon Sep 17 00:00:00 2001 From: "Guillem L. Jara" <4lon3ly0@tutanota.com> Date: Wed, 25 Mar 2026 05:17:57 +0100 Subject: [PATCH 3/4] fix(ls): add assertion for ancestor count --- src/uu/ls/src/ls.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 9651b4558c3..1c688e519f6 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1144,6 +1144,13 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { if config.dired && !config.hyperlink { dired::print_dired_output(config, &dired, &mut state.out)?; } + + // Check all nodes are cleared after recursive listing in integration tests. + // FIXME: Turn into a proper test; integration testing most likely requires + // hard-linked directories, so it is a no-go. A unit test may be more + // feasible given a heavy refactor that decouples [`depth_first_list`]. + debug_assert!(state.listed_ancestors.is_empty()); + Ok(()) } From c08ea544c28d82019c3cc70701bbac9d05498680 Mon Sep 17 00:00:00 2001 From: "Guillem L. Jara" <4lon3ly0@tutanota.com> Date: Wed, 25 Mar 2026 09:31:53 +0100 Subject: [PATCH 4/4] chore(ls): document and rename PathData::synthetic It is now PathData::is_dot_dir, which is more descriptive. --- src/uu/ls/src/ls.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 1c688e519f6..5cb2a9a0096 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -810,7 +810,8 @@ struct PathData<'a> { p_buf: Cow<'a, Path>, must_dereference: bool, command_line: bool, - synthetic: bool, + // True if it is one of the two dot dirs: `.` and `..`. + is_dot_dir: bool, } impl<'a> PathData<'a> { @@ -887,7 +888,7 @@ impl<'a> PathData<'a> { p_buf, must_dereference, command_line, - synthetic, + is_dot_dir: synthetic, } } @@ -1325,7 +1326,7 @@ fn depth_first_list( if config.recursive { for e in buf .into_iter() - .filter(|p| p.file_type().is_some_and(FileType::is_dir) && !p.synthetic) + .filter(|p| p.file_type().is_some_and(FileType::is_dir) && !p.is_dot_dir) .rev() { // Try to open only to report any errors in order to match GNU semantics.