Exit non-zero on filesystem traversal errors

CHANGELOG.md

@@ -14,6 +14,7 @@
 - Also fire the "search pattern contains a path separator" diagnostic for `--and` patterns, not only the primary positional pattern. `--and` patterns are matched against the file name just like the primary pattern, so a path separator in them silently returned zero results. See #1873.
 - Fix bug where passing "-" as a directory argument didn't actually search that directory, see #849 (@Sean-Kenneth-Doherty).
 - Fix panic when `--changed-before`/`--changed-within` is given an out-of-range `@` Unix timestamp; the value is now rejected gracefully, see #2081 (@nikolauspschuetz).
+- `fd` now exits with a non-zero status if a filesystem/traversal error occurs (e.g. a path exceeding `PATH_MAX`), even when the error isn't printed (i.e. without `--show-errors`). Previously it always reported success as long as it didn't crash, silently hiding the fact that part of the search was skipped. Applies to normal output, `--exec`, and `--exec-batch`. See #1985 (@ezekiel06).
 
 # 10.4.2
 

src/exec/job.rs

@@ -26,6 +26,9 @@ pub fn job(
                 if config.show_filesystem_errors {
                     print_error(err.to_string());
                 }
+                // A traversal error means the search was incomplete; reflect that in
+                // the exit code even if it isn't printed (i.e. without --show-errors).
+                ret = merge_exitcodes([ret, ExitCode::GeneralError]);
                 continue;
             }
         };
@@ -48,6 +51,11 @@ pub fn batch(
     cmd: &CommandSet,
     config: &Config,
 ) -> ExitCode {
+    // Tracks whether any `WorkerResult::Error` was seen while draining `paths` below, so
+    // that a traversal error can still affect the exit code even though it isn't a path
+    // and therefore can't be passed through to `execute_batch`.
+    let had_filesystem_error = std::cell::Cell::new(false);
+
     let paths = results
         .into_iter()
         .filter_map(|worker_result| match worker_result {
@@ -56,9 +64,16 @@ pub fn batch(
                 if config.show_filesystem_errors {
                     print_error(err.to_string());
                 }
+                had_filesystem_error.set(true);
                 None
             }
         });
 
-    cmd.execute_batch(paths, config.batch_size, config.path_separator.as_deref())
+    let exit_code = cmd.execute_batch(paths, config.batch_size, config.path_separator.as_deref());
+
+    if had_filesystem_error.get() {
+        merge_exitcodes([exit_code, ExitCode::GeneralError])
+    } else {
+        exit_code
+    }
 }

src/walk.rs

@@ -146,6 +146,8 @@ struct ReceiverBuffer<'a, W> {
     buffer: Vec<DirEntry>,
     /// Result count.
     num_results: usize,
+    /// Whether a filesystem/traversal error (e.g. `WorkerResult::Error`) was encountered.
+    had_filesystem_error: bool,
 }
 
 impl<'a, W: Write> ReceiverBuffer<'a, W> {
@@ -167,6 +169,7 @@ impl<'a, W: Write> ReceiverBuffer<'a, W> {
             deadline,
             buffer: Vec::with_capacity(MAX_BUFFER_LENGTH),
             num_results: 0,
+            had_filesystem_error: false,
         }
     }
 
@@ -225,6 +228,7 @@ impl<'a, W: Write> ReceiverBuffer<'a, W> {
                             }
                         }
                         WorkerResult::Error(err) => {
+                            self.had_filesystem_error = true;
                             if self.config.show_filesystem_errors {
                                 print_error(err.to_string());
                             }
@@ -285,7 +289,15 @@ impl<'a, W: Write> ReceiverBuffer<'a, W> {
             self.stream()?;
         }
 
-        if self.config.quiet {
+        // A filesystem/traversal error (e.g. a path exceeding PATH_MAX) means the search
+        // was incomplete, even if it otherwise finished normally. Surface that as a
+        // non-zero exit code instead of silently reporting success, mirroring the
+        // behavior of GNU find. This takes priority over the `--quiet` exit status,
+        // since an incomplete search shouldn't be reported the same as a complete one
+        // with no results.
+        if self.had_filesystem_error {
+            Err(ExitCode::GeneralError)
+        } else if self.config.quiet {
             Err(ExitCode::HasResults(self.num_results > 0))
         } else {
             Err(ExitCode::Success)

tests/tests.rs

@@ -2491,6 +2491,50 @@ fn test_owner_root() {
     te.assert_output(&["--owner", ":0", "a.foo"], "");
 }
 
+/// A traversal error (e.g. a directory that can't be read) should be reflected in the
+/// exit code, even though it isn't fatal to the overall search. Previously, `fd` always
+/// reported success as long as it didn't crash, silently hiding the fact that part of
+/// the search was skipped. See https://github.com/sharkdp/fd/issues/1985.
+#[cfg(unix)]
+#[test]
+fn test_exit_code_reflects_traversal_errors() {
+    use std::os::unix::fs::PermissionsExt;
+
+    // This test assumes the current user isn't root (root can read anything,
+    // permission bits notwithstanding).
+    if Uid::current().is_root() {
+        return;
+    }
+
+    let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES);
+    let unreadable_dir = te.test_root().join("one/two/three/directory_foo");
+
+    // A directory search should complete successfully and report success when there
+    // are no filesystem errors.
+    te.assert_success_and_get_output(".", &[]);
+
+    // Remove read+execute permissions so that `fd` can't list this directory's
+    // contents, which surfaces as a traversal (`WorkerResult::Error`) during the walk.
+    fs::set_permissions(&unreadable_dir, fs::Permissions::from_mode(0o000))
+        .expect("could not change permissions for test");
+
+    let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+        te.assert_failure(&[]);
+        // The default output should still include the results fd *was* able to find.
+        te.assert_failure(&["--exec", "true", ";"]);
+        te.assert_failure(&["--exec-batch", "true"]);
+    }));
+
+    // Restore permissions before the temp directory is cleaned up, regardless of
+    // whether the assertions above passed, so we don't leak an unreadable directory.
+    fs::set_permissions(&unreadable_dir, fs::Permissions::from_mode(0o755))
+        .expect("could not restore permissions for test");
+
+    if let Err(err) = result {
+        std::panic::resume_unwind(err);
+    }
+}
+
 #[test]
 fn test_custom_path_separator() {
     let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES);