From da5808d4aca0e8a997048c8b98bea3d968a322f4 Mon Sep 17 00:00:00 2001 From: Niyaz Nigmatullin Date: Mon, 11 Jul 2022 18:18:58 +0300 Subject: [PATCH] ls: add already listed message (#3707) * ls: handle looping symlinks infinite printing * ls: better coloring and printing symlinks when dereferenced * tests/ls: add dereferencing and symlink loop tests * ls: reformat changed using rustfmt * ls: follow clippy advice for cleaner code * uucore/fs: fix FileInformation to open directory handles in Windows as well --- src/uu/ls/src/ls.rs | 76 ++++++++++++++++++++++++------- src/uucore/src/lib/features/fs.rs | 5 +- tests/by-util/test_ls.rs | 59 ++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 18 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index cd400ec7c5a..59d09fbff48 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -18,6 +18,7 @@ use glob::Pattern; use lscolors::LsColors; use number_prefix::NumberPrefix; use once_cell::unsync::OnceCell; +use std::collections::HashSet; #[cfg(windows)] use std::os::windows::fs::MetadataExt; use std::{ @@ -151,6 +152,7 @@ enum LsError { IOError(std::io::Error), IOErrorContext(std::io::Error, PathBuf), BlockSizeParseError(String), + AlreadyListedError(PathBuf), } impl UError for LsError { @@ -160,6 +162,7 @@ impl UError for LsError { LsError::IOError(_) => 1, LsError::IOErrorContext(_, _) => 1, LsError::BlockSizeParseError(_) => 1, + LsError::AlreadyListedError(_) => 2, } } } @@ -236,6 +239,13 @@ impl Display for LsError { }, } } + LsError::AlreadyListedError(path) => { + write!( + f, + "{}: not listing already-listed directory", + path.to_string_lossy() + ) + } } } } @@ -1488,16 +1498,26 @@ impl PathData { // Why prefer to check the DirEntry file_type()? B/c the call is // nearly free compared to a metadata() call on a Path - let ft = match de { - Some(ref de) => { - if let Ok(ft_de) = de.file_type() { - OnceCell::from(Some(ft_de)) - } else if let Ok(md_pb) = p_buf.metadata() { - OnceCell::from(Some(md_pb.file_type())) - } else { - OnceCell::new() + fn get_file_type( + de: &DirEntry, + p_buf: &Path, + must_dereference: bool, + ) -> OnceCell> { + if must_dereference { + if let Ok(md_pb) = p_buf.metadata() { + return OnceCell::from(Some(md_pb.file_type())); } } + if let Ok(ft_de) = de.file_type() { + OnceCell::from(Some(ft_de)) + } else if let Ok(md_pb) = p_buf.symlink_metadata() { + OnceCell::from(Some(md_pb.file_type())) + } else { + OnceCell::new() + } + } + let ft = match de { + Some(ref de) => get_file_type(de, &p_buf, must_dereference), None => OnceCell::new(), }; @@ -1619,7 +1639,12 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> { writeln!(out, "\n{}:", path_data.p_buf.display())?; } } - enter_directory(path_data, read_dir, config, &mut out)?; + let mut listed_ancestors = HashSet::new(); + listed_ancestors.insert(FileInformation::from_path( + &path_data.p_buf, + path_data.must_dereference, + )?); + enter_directory(path_data, read_dir, config, &mut out, &mut listed_ancestors)?; } Ok(()) @@ -1715,6 +1740,7 @@ fn enter_directory( read_dir: ReadDir, config: &Config, out: &mut BufWriter, + listed_ancestors: &mut HashSet, ) -> UResult<()> { // Create vec of entries with initial dot files let mut entries: Vec = if config.files == Files::All { @@ -1780,8 +1806,17 @@ fn enter_directory( continue; } Ok(rd) => { - writeln!(out, "\n{}:", e.p_buf.display())?; - enter_directory(e, rd, config, out)?; + if !listed_ancestors + .insert(FileInformation::from_path(&e.p_buf, e.must_dereference)?) + { + out.flush()?; + show!(LsError::AlreadyListedError(e.p_buf.clone())); + } else { + writeln!(out, "\n{}:", e.p_buf.display())?; + enter_directory(e, rd, config, out, listed_ancestors)?; + listed_ancestors + .remove(&FileInformation::from_path(&e.p_buf, e.must_dereference)?); + } } } } @@ -2254,6 +2289,7 @@ fn get_inode(metadata: &Metadata) -> String { use std::sync::Mutex; #[cfg(unix)] use uucore::entries; +use uucore::fs::FileInformation; use uucore::quoting_style; #[cfg(unix)] @@ -2518,12 +2554,17 @@ fn display_file_name( let mut width = name.width(); if let Some(ls_colors) = &config.color { - name = color_name( - name, - &path.p_buf, - path.p_buf.symlink_metadata().ok().as_ref(), - ls_colors, - ); + let md = path.md(out); + name = if md.is_some() { + color_name(name, &path.p_buf, md, ls_colors) + } else { + color_name( + name, + &path.p_buf, + path.p_buf.symlink_metadata().ok().as_ref(), + ls_colors, + ) + }; } if config.format != Format::Long && !more_info.is_empty() { @@ -2564,6 +2605,7 @@ fn display_file_name( if config.format == Format::Long && path.file_type(out).is_some() && path.file_type(out).unwrap().is_symlink() + && !path.must_dereference { if let Ok(target) = path.p_buf.read_link() { name.push_str(" -> "); diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index d1b48a12735..d92f0f1e6d2 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -79,9 +79,12 @@ impl FileInformation { use std::fs::OpenOptions; use std::os::windows::prelude::*; let mut open_options = OpenOptions::new(); + let mut custom_flags = 0; if !dereference { - open_options.custom_flags(winapi::um::winbase::FILE_FLAG_OPEN_REPARSE_POINT); + custom_flags |= winapi::um::winbase::FILE_FLAG_OPEN_REPARSE_POINT; } + custom_flags |= winapi::um::winbase::FILE_FLAG_BACKUP_SEMANTICS; + open_options.custom_flags(custom_flags); let file = open_options.read(true).open(path.as_ref())?; Self::from_file(&file) } diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index c0d54a7cd96..2d8bf0c6d9a 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -3065,3 +3065,62 @@ fn test_ls_quoting_color() { .succeeds() .stdout_contains("\'need quoting\'"); } + +#[test] +fn test_ls_dereference_looped_symlinks_recursive() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.mkdir("loop"); + at.relative_symlink_dir("../loop", "loop/sub"); + + ucmd.args(&["-RL", "loop"]) + .fails() + .stderr_contains("not listing already-listed directory"); +} + +#[test] +fn test_dereference_dangling_color() { + let (at, mut ucmd) = at_and_ucmd!(); + at.relative_symlink_file("wat", "nonexistent"); + let out_exp = ucmd.args(&["--color"]).run().stdout_move_str(); + + let (at, mut ucmd) = at_and_ucmd!(); + at.relative_symlink_file("wat", "nonexistent"); + ucmd.args(&["-L", "--color"]) + .fails() + .code_is(1) + .stderr_contains("No such file or directory") + .stdout_is(out_exp); +} + +#[test] +fn test_dereference_symlink_dir_color() { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("dir1"); + at.mkdir("dir1/link"); + let out_exp = ucmd.args(&["--color", "dir1"]).run().stdout_move_str(); + + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("dir1"); + at.mkdir("dir2"); + at.relative_symlink_dir("../dir2", "dir1/link"); + ucmd.args(&["-L", "--color", "dir1"]) + .succeeds() + .stdout_is(out_exp); +} + +#[test] +fn test_dereference_symlink_file_color() { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("dir1"); + at.touch("dir1/link"); + let out_exp = ucmd.args(&["--color", "dir1"]).run().stdout_move_str(); + + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("dir1"); + at.touch("file"); + at.relative_symlink_file("../file", "dir1/link"); + ucmd.args(&["-L", "--color", "dir1"]) + .succeeds() + .stdout_is(out_exp); +}