From 37ca6edfdc6c3f6da4fd906b7f095772e7d8ddd0 Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Sat, 15 Jan 2022 22:39:07 -0600 Subject: [PATCH] Fix display of bad fd errors --- src/uu/ls/src/ls.rs | 177 ++++++++++++++++++++++++++++----------- tests/by-util/test_ls.rs | 41 ++++++++- 2 files changed, 167 insertions(+), 51 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index fd3e41adb..226dcd1bf 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -21,7 +21,6 @@ use lscolors::LsColors; use number_prefix::NumberPrefix; use once_cell::unsync::OnceCell; use quoting_style::{escape_name, QuotingStyle}; -use std::ffi::OsString; #[cfg(windows)] use std::os::windows::fs::MetadataExt; use std::{ @@ -39,6 +38,7 @@ use std::{ os::unix::fs::{FileTypeExt, MetadataExt}, time::Duration, }; +use std::{ffi::OsString, fs::ReadDir}; use term_grid::{Cell, Direction, Filling, Grid, GridOptions}; use uucore::{ display::Quotable, @@ -165,7 +165,7 @@ impl Display for LsError { LsError::IOError(e) => write!(f, "general io error: {}", e), LsError::IOErrorContext(e, p) => { let error_kind = e.kind(); - let raw_os_error = e.raw_os_error().unwrap_or(13i32); + let errno = e.raw_os_error().unwrap_or(1i32); match error_kind { // No such file or directory @@ -180,7 +180,7 @@ impl Display for LsError { ErrorKind::PermissionDenied => { #[allow(clippy::wildcard_in_or_patterns)] - match raw_os_error { + match errno { 1i32 => { write!( f, @@ -205,12 +205,23 @@ impl Display for LsError { } } } - _ => write!( - f, - "unknown io error: '{:?}', '{:?}'", - p.to_string_lossy(), - e - ), + _ => match errno { + 9i32 => { + write!( + f, + "cannot access '{}': Bad file descriptor", + p.to_string_lossy(), + ) + } + _ => { + write!( + f, + "unknown io error: '{:?}', '{:?}'", + p.to_string_lossy(), + e + ) + } + }, } } } @@ -1342,8 +1353,19 @@ impl PathData { || match get_metadata(self.p_buf.as_path(), self.must_dereference) { Err(err) => { let _ = out.flush(); - show!(LsError::IOErrorContext(err, self.p_buf.clone(),)); - None + let errno = err.raw_os_error().unwrap_or(1i32); + // Wait to enter "directory" to print error for a bad fd + if self.must_dereference && errno.eq(&9i32) { + if let Ok(md) = get_metadata(&self.p_buf, false) { + Some(md) + } else { + show!(LsError::IOErrorContext(err, self.p_buf.clone(),)); + None + } + } else { + show!(LsError::IOErrorContext(err, self.p_buf.clone(),)); + None + } } Ok(md) => Some(md), }, @@ -1351,6 +1373,10 @@ impl PathData { .as_ref() } + fn set_md(&self, md: Metadata) { + self.md.get_or_init(|| Some(md)).as_ref(); + } + fn file_type(&self, out: &mut BufWriter) -> Option<&FileType> { self.ft .get_or_init(|| self.md(out).map(|md| md.file_type())) @@ -1397,16 +1423,28 @@ fn list(locs: Vec<&Path>, config: Config) -> UResult<()> { display_items(&files, &config, &mut out); - for (pos, dir) in dirs.iter().enumerate() { + for (pos, path_data) in dirs.iter().enumerate() { + // Do read_dir call here to match GNU semantics by printing + // read_dir errors before directory headings, names and totals + let read_dir = match fs::read_dir(&path_data.p_buf) { + Err(err) => { + // flush stdout buffer before the error to preserve formatting and order + let _ = out.flush(); + show!(LsError::IOErrorContext(err, path_data.p_buf.clone())); + continue; + } + Ok(rd) => rd, + }; + // Print dir heading - name... 'total' comes after error display if initial_locs_len > 1 || config.recursive { if pos.eq(&0usize) && files.is_empty() { - let _ = writeln!(out, "{}:", dir.p_buf.display()); + let _ = writeln!(out, "{}:", path_data.p_buf.display()); } else { - let _ = writeln!(out, "\n{}:", dir.p_buf.display()); + let _ = writeln!(out, "\n{}:", path_data.p_buf.display()); } } - enter_directory(dir, &config, &mut out); + enter_directory(path_data, read_dir, &config, &mut out); } Ok(()) @@ -1475,12 +1513,29 @@ fn should_display(entry: &DirEntry, config: &Config) -> bool { true } -fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) { +fn enter_directory( + path_data: &PathData, + read_dir: ReadDir, + config: &Config, + out: &mut BufWriter, +) { // Create vec of entries with initial dot files let mut entries: Vec = if config.files == Files::All { vec![ - PathData::new(dir.p_buf.clone(), None, Some(".".into()), config, false), - PathData::new(dir.p_buf.join(".."), None, Some("..".into()), config, false), + PathData::new( + path_data.p_buf.clone(), + None, + Some(".".into()), + config, + false, + ), + PathData::new( + path_data.p_buf.join(".."), + None, + Some("..".into()), + config, + false, + ), ] } else { vec![] @@ -1489,19 +1544,8 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) // Convert those entries to the PathData struct let mut vec_path_data = Vec::new(); - // check for errors early, and ignore entries with errors - let read_dir = match fs::read_dir(&dir.p_buf) { - Err(err) => { - // flush buffer because the error may get printed in the wrong order - let _ = out.flush(); - show!(LsError::IOErrorContext(err, dir.p_buf.clone())); - return; - } - Ok(res) => res, - }; - - for entry in read_dir { - let unwrapped = match entry { + for raw_entry in read_dir { + let dir_entry = match raw_entry { Ok(path) => path, Err(err) => { let _ = out.flush(); @@ -1510,27 +1554,40 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) } }; - if should_display(&unwrapped, config) { - // why check the DirEntry file_type()? B/c the call is - // nearly free compared to a metadata() or file_type() call on a dir/file - let path_data = match unwrapped.file_type() { - Err(err) => { - let _ = out.flush(); - show!(LsError::IOErrorContext(err, unwrapped.path())); - continue; - } - Ok(dir_ft) => { - PathData::new(unwrapped.path(), Some(Ok(dir_ft)), None, config, false) + if should_display(&dir_entry, config) { + // Why prefer to check the DirEntry file_type()? B/c the call is + // nearly free compared to a metadata() or file_type() call on a dir/file. + // + // Why not print an error here? If we wait for the metadata() call, we make + // certain we print the error once. This also seems to match GNU behavior. + let entry_path_data = match dir_entry.file_type() { + Ok(ft) => { + let res = PathData::new(dir_entry.path(), Some(Ok(ft)), None, config, false); + // metadata returned from a DirEntry matches GNU metadata for + // non-dereferenced files, and is *different* from the + // metadata call on the path, see, for example, bad fds + if !res.must_dereference + && ((config.format == Format::Long) + || (config.sort == Sort::Name) + || (config.sort == Sort::None) + || config.inode) + { + if let Ok(md) = dir_entry.metadata() { + res.set_md(md) + } + } + res } + Err(_) => PathData::new(dir_entry.path(), None, None, config, false), }; - vec_path_data.push(path_data); + vec_path_data.push(entry_path_data); }; } sort_entries(&mut vec_path_data, config, out); entries.append(&mut vec_path_data); - // ...and total + // Print total after any error display if config.format == Format::Long { display_total(&entries, config, out); } @@ -1546,8 +1603,17 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) .filter(|p| p.ft.get().unwrap().is_some()) .filter(|p| p.ft.get().unwrap().unwrap().is_dir()) { - let _ = writeln!(out, "\n{}:", e.p_buf.display()); - enter_directory(e, config, out); + match fs::read_dir(&e.p_buf) { + Err(err) => { + let _ = out.flush(); + show!(LsError::IOErrorContext(err, e.p_buf.clone())); + continue; + } + Ok(rd) => { + let _ = writeln!(out, "\n{}:", e.p_buf.display()); + enter_directory(e, rd, config, out); + } + } } } } @@ -1974,7 +2040,24 @@ fn display_item_long( let _ = write!( out, "{}{} {}", - "l?????????".to_string(), + format_args!( + "{}?????????", + if item.ft.get().is_some() && item.ft.get().unwrap().is_some() { + if item.ft.get().unwrap().unwrap().is_char_device() { + "c" + } else if item.ft.get().unwrap().unwrap().is_block_device() { + "b" + } else if item.ft.get().unwrap().unwrap().is_symlink() { + "l" + } else if item.ft.get().unwrap().unwrap().is_dir() { + "d" + } else { + "-" + } + } else { + "-" + }, + ), if item.security_context.len() > 1 { // GNU `ls` uses a "." character to indicate a file with a security context, // but not other alternate access method. diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index b5d49337d..85b336a26 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -7,6 +7,11 @@ use crate::common::util::*; extern crate regex; use self::regex::Regex; +#[cfg(unix)] +use nix::unistd::{close, dup2}; +#[cfg(unix)] +use std::os::unix::io::AsRawFd; + use std::collections::HashMap; use std::path::Path; use std::thread::sleep; @@ -128,10 +133,7 @@ fn test_ls_io_errors() { at.symlink_file("does_not_exist", "some-dir2/dangle"); at.mkdir("some-dir3"); at.mkdir("some-dir3/some-dir4"); - at.mkdir("some-dir3/some-dir5"); - at.mkdir("some-dir3/some-dir6"); - at.mkdir("some-dir3/some-dir7"); - at.mkdir("some-dir3/some-dir8"); + at.mkdir("some-dir4"); scene.ccmd("chmod").arg("000").arg("some-dir1").succeeds(); @@ -177,6 +179,37 @@ fn test_ls_io_errors() { .stderr_does_not_contain( "ls: cannot access 'some-dir2/dangle': No such file or directory\nls: cannot access 'some-dir2/dangle': No such file or directory" ); + + #[cfg(unix)] + { + at.touch("some-dir4/bad-fd.txt"); + let fd1 = at.open("some-dir4/bad-fd.txt").as_raw_fd(); + let fd2 = 25000; + let rv1 = dup2(fd1, fd2); + let rv2 = close(fd1); + + scene + .ucmd() + .arg("-alR") + .arg(format!("/dev/fd/{}", fd2)) + .fails() + .stderr_contains(format!( + "cannot access '/dev/fd/{}': Bad file descriptor", + fd2 + )) + .stdout_does_not_contain(format!("{}:\n", fd2)); + + scene + .ucmd() + .arg("-RiL") + .arg(format!("/dev/fd/{}", fd2)) + .fails() + .stderr_contains(format!("cannot access '/dev/fd/{}': Bad file descriptor", fd2)) + // test we only print bad fd error once + .stderr_does_not_contain(format!("ls: cannot access '/dev/fd/{fd}': Bad file descriptor\nls: cannot access '/dev/fd/{fd}': Bad file descriptor", fd = fd2)); + + let _ = close(fd2); + } } #[test]