Fix display of bad fd errors

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!(
+                    _ => match errno {
-                        f,
+                        9i32 => {
-                        "unknown io error: '{:?}', '{:?}'",
+                            write!(
-                        p.to_string_lossy(),
+                                f,
-                        e
+                                "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(),));
+                        let errno = err.raw_os_error().unwrap_or(1i32);
-                        None
+                        // 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<Stdout>) -> 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<Stdout>) {
+fn enter_directory(
+    path_data: &PathData,
+    read_dir: ReadDir,
+    config: &Config,
+    out: &mut BufWriter<Stdout>,
+) {
     // Create vec of entries with initial dot files
     let mut entries: Vec<PathData> = if config.files == Files::All {
         vec![
-            PathData::new(dir.p_buf.clone(), None, Some(".".into()), config, false),
+            PathData::new(
-            PathData::new(dir.p_buf.join(".."), None, Some("..".into()), config, false),
+                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<Stdout>)
     // Convert those entries to the PathData struct
     let mut vec_path_data = Vec::new();
 
-    // check for errors early, and ignore entries with errors
+    for raw_entry in read_dir {
-    let read_dir = match fs::read_dir(&dir.p_buf) {
+        let dir_entry = match raw_entry {
-        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 {
             Ok(path) => path,
             Err(err) => {
                 let _ = out.flush();
@@ -1510,27 +1554,40 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter<Stdout>)
             }
         };
 
-        if should_display(&unwrapped, config) {
+        if should_display(&dir_entry, config) {
-            // why check the DirEntry file_type()?  B/c the call is
+            // 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
+            // nearly free compared to a metadata() or file_type() call on a dir/file.
-            let path_data = match unwrapped.file_type() {
+            //
-                Err(err) => {
+            // Why not print an error here?  If we wait for the metadata() call, we make
-                    let _ = out.flush();
+            // certain we print the error once.  This also seems to match GNU behavior.
-                    show!(LsError::IOErrorContext(err, unwrapped.path()));
+            let entry_path_data = match dir_entry.file_type() {
-                    continue;
+                Ok(ft) => {
-                }
+                    let res = PathData::new(dir_entry.path(), Some(Ok(ft)), None, config, false);
-                Ok(dir_ft) => {
+                    // metadata returned from a DirEntry matches GNU metadata for
-                    PathData::new(unwrapped.path(), Some(Ok(dir_ft)), None, config, false)
+                    // 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<Stdout>)
             .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());
+            match fs::read_dir(&e.p_buf) {
-            enter_directory(e, config, out);
+                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.

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-dir4");
-    at.mkdir("some-dir3/some-dir6");
-    at.mkdir("some-dir3/some-dir7");
-    at.mkdir("some-dir3/some-dir8");
 
     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]