ls: properly release parent nodes in DFS

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

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<Cow<'a, OsStr>>,
         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,
         }
     }
 
@@ -974,7 +977,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 +1002,7 @@ struct ListState<'a> {
     #[cfg(not(unix))]
     gid_cache: (),
     recent_time_range: RangeInclusive<SystemTime>,
-    stack: Vec<DirData>,
+    stack: Vec<LsNode>,
     listed_ancestors: FxHashSet<FileInformation>,
     initial_locs_len: usize,
     display_buf: Vec<u8>,
@@ -1035,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
@@ -1109,22 +1116,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 +1212,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, false);
 
     // Print dir heading - name... 'total' comes after error display
     if state.initial_locs_len > 1 || config.recursive {
@@ -1253,27 +1257,27 @@ fn depth_first_list(
     }
 
     // Append entries with initial dot files and record their existence
-    let (ref 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(),
                 None,
                 Some(OsStr::new("..").into()),
                 config,
                 false,
+                true,
             ),
-        ];
-        (v, DOT_DIRECTORIES)
+        ]
     } else {
-        (Vec::new(), 0)
+        Vec::new()
     };
 
     // Convert those entries to the PathData struct
@@ -1287,6 +1291,7 @@ fn depth_first_list(
                         None,
                         config,
                         false,
+                        false,
                     ));
                 }
             }
@@ -1299,22 +1304,21 @@ 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()
-            .skip(trim)
-            .filter(|p| p.file_type().is_some_and(FileType::is_dir))
+            .into_iter()
+            .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.
@@ -1327,13 +1331,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 +1348,22 @@ fn depth_first_list(
     Ok(())
 }
 
+impl LsNode {
+    fn new(path: impl Into<PathBuf>, needs_blank_line: bool, fi: FileInformation) -> Self {
+        Self {
+            path: path.into(),
+            needs_blank_line,
+            fi,
+        }
+    }
+}
+
+impl From<LsNode> 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<Metadata> {
     if dereference {
         p_buf.metadata()

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,

tests/by-util/test_ls.rs

@@ -7119,3 +7119,14 @@
 
     scene.ucmd().succeeds().stdout_does_not_contain(".hidden");
 }
+
+#[test]
+#[cfg(target_os = "windows")]
+fn test_ls_recursive_hardlink_cycles() {
+    use std::path::PathBuf;
+
+    let scene = TestScenario::new(util_name!());
+    let mut at = PathBuf::from(std::env::var_os("WINDIR").unwrap_or("C:\\Windows".into()));
+    at.push("WinSxS");
+    assert_ne!(scene.ucmd().arg("-laR").arg(at).run().code(), 2); // critical error.
+}