From 322fc75a3da11242c3e23a60bc4cddbc688552b0 Mon Sep 17 00:00:00 2001
From: Jakub Wieczorek <jakub.adam.wieczorek@gmail.com>
Date: Tue, 3 Sep 2019 21:47:33 +0000
Subject: ignore: make walker visit untraversable directories

This commit fixes an inconsistency between the serial and the parallel
directory walkers around visiting a directory for which the user holds
insufficient permissions to descend into.

The serial walker does produce a successful entry for a directory that
it cannot descend into due to insufficient permissions. However, before
this change that has not been the case for the parallel walker, which
would produce an `Err` item not only when descending into a directory
that it cannot read from but also for the directory entry itself.

This change brings the behaviour of the parallel variant in line with
that of the serial one.

Fixes #1346, Closes #1365
---
 ignore/src/walk.rs | 56 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 11 deletions(-)

diff --git a/ignore/src/walk.rs b/ignore/src/walk.rs
index 365d049d..bbe6d7d7 100644
--- a/ignore/src/walk.rs
+++ b/ignore/src/walk.rs
@@ -1409,16 +1409,7 @@ impl<'s> Worker<'s> {
                     return;
                 }
             }
-            let readdir = match work.read_dir() {
-                Ok(readdir) => readdir,
-                Err(err) => {
-                    if self.visitor.visit(Err(err)).is_quit() {
-                        self.quit_now();
-                        return;
-                    }
-                    continue;
-                }
-            };
+
             let descend = if let Some(root_device) = work.root_device {
                 match is_same_file_system(root_device, work.dent.path()) {
                     Ok(true) => true,
@@ -1435,6 +1426,13 @@ impl<'s> Worker<'s> {
                 true
             };
 
+            // Try to read the directory first before we transfer ownership
+            // to the provided closure. Do not unwrap it immediately, though,
+            // as we may receive an `Err` value e.g. in the case when we do not
+            // have sufficient read permissions to list the directory.
+            // In that case we still want to provide the closure with a valid
+            // entry before passing the error value.
+            let readdir = work.read_dir();
             let depth = work.dent.depth();
             match self.visitor.visit(Ok(work.dent)) {
                 WalkState::Continue => {}
@@ -1447,11 +1445,28 @@ impl<'s> Worker<'s> {
             if !descend {
                 continue;
             }
+
+            let readdir = match readdir {
+                Ok(readdir) => readdir,
+                Err(err) => {
+                    if self.visitor.visit(Err(err)).is_quit() {
+                        self.quit_now();
+                        return;
+                    }
+                    continue;
+                }
+            };
+
             if self.max_depth.map_or(false, |max| depth >= max) {
                 continue;
             }
             for result in readdir {
-                let state = self.run_one(&work.ignore, depth + 1, work.root_device, result);
+                let state = self.run_one(
+                    &work.ignore,
+                    depth + 1,
+                    work.root_device,
+                    result,
+                );
                 if state.is_quit() {
                     self.quit_now();
                     return;
@@ -2155,4 +2170,23 @@ mod tests {
         builder.follow_links(true).same_file_system(true);
         assert_paths(td.path(), &builder, &["same_file", "same_file/alink"]);
     }
+
+    #[cfg(target_os = "linux")]
+    #[test]
+    fn no_read_permissions() {
+        let dir_path = Path::new("/root");
+
+        // There's no /etc/sudoers.d, skip the test.
+        if !dir_path.is_dir() {
+            return;
+        }
+        // We're the root, so the test won't check what we want it to.
+        if fs::read_dir(&dir_path).is_ok() {
+            return;
+        }
+
+        // Check that we can't descend but get an entry for the parent dir.
+        let builder = WalkBuilder::new(&dir_path);
+        assert_paths(dir_path.parent().unwrap(), &builder, &["root"]);
+    }
 }
-- 
cgit v1.2.3