Auto merge of #10188 - weihanglo:issue-9528, r=alexcrichton

Detect filesystem loop during walking the projects

Resolves #9528

~~This PR also adds a new dependency `same-file` but since it's already a
dependency of `cargo-util`, so nothing added actually.~~

Use `walkdir` to detect filesystem loop and gain performance boost!

Author: bors

src/cargo/sources/path.rs

@@ -1,6 +1,5 @@
 use std::collections::HashSet;
 use std::fmt::{self, Debug, Formatter};
-use std::fs;
 use std::path::{Path, PathBuf};
 
 use crate::core::source::MaybePackage;
@@ -11,8 +10,8 @@ use anyhow::Context as _;
 use cargo_util::paths;
 use filetime::FileTime;
 use ignore::gitignore::GitignoreBuilder;
-use ignore::Match;
 use log::{trace, warn};
+use walkdir::WalkDir;
 
 pub struct PathSource<'cfg> {
     source_id: SourceId,
@@ -131,39 +130,36 @@ impl<'cfg> PathSource<'cfg> {
         }
         let ignore_include = include_builder.build()?;
 
-        let ignore_should_package = |relative_path: &Path, is_dir: bool| -> CargoResult<bool> {
+        let ignore_should_package = |relative_path: &Path, is_dir: bool| {
             // "Include" and "exclude" options are mutually exclusive.
             if no_include_option {
-                match ignore_exclude.matched_path_or_any_parents(relative_path, is_dir) {
-                    Match::None => Ok(true),
-                    Match::Ignore(_) => Ok(false),
-                    Match::Whitelist(_) => Ok(true),
-                }
+                !ignore_exclude
+                    .matched_path_or_any_parents(relative_path, is_dir)
+                    .is_ignore()
             } else {
                 if is_dir {
                     // Generally, include directives don't list every
                     // directory (nor should they!). Just skip all directory
                     // checks, and only check files.
-                    return Ok(true);
+                    return true;
                 }
-                match ignore_include
+                ignore_include
                     .matched_path_or_any_parents(relative_path, /* is_dir */ false)
-                {
-                    Match::None => Ok(false),
-                    Match::Ignore(_) => Ok(true),
-                    Match::Whitelist(_) => Ok(false),
-                }
+                    .is_ignore()
             }
         };
 
-        let mut filter = |path: &Path, is_dir: bool| -> CargoResult<bool> {
-            let relative_path = path.strip_prefix(root)?;
+        let mut filter = |path: &Path, is_dir: bool| {
+            let relative_path = match path.strip_prefix(root) {
+                Ok(p) => p,
+                Err(_) => return false,
+            };
 
             let rel = relative_path.as_os_str();
             if rel == "Cargo.lock" {
-                return Ok(pkg.include_lockfile());
+                return pkg.include_lockfile();
             } else if rel == "Cargo.toml" {
-                return Ok(true);
+                return true;
             }
 
             ignore_should_package(relative_path, is_dir)
@@ -225,7 +221,7 @@ impl<'cfg> PathSource<'cfg> {
         &self,
         pkg: &Package,
         repo: &git2::Repository,
-        filter: &mut dyn FnMut(&Path, bool) -> CargoResult<bool>,
+        filter: &mut dyn FnMut(&Path, bool) -> bool,
     ) -> CargoResult<Vec<PathBuf>> {
         warn!("list_files_git {}", pkg.package_id());
         let index = repo.index()?;
@@ -347,7 +343,7 @@ impl<'cfg> PathSource<'cfg> {
                         PathSource::walk(&file_path, &mut ret, false, filter)?;
                     }
                 }
-            } else if (*filter)(&file_path, is_dir)? {
+            } else if filter(&file_path, is_dir) {
                 assert!(!is_dir);
                 // We found a file!
                 warn!("  found {}", file_path.display());
@@ -379,7 +375,7 @@ impl<'cfg> PathSource<'cfg> {
     fn list_files_walk(
         &self,
         pkg: &Package,
-        filter: &mut dyn FnMut(&Path, bool) -> CargoResult<bool>,
+        filter: &mut dyn FnMut(&Path, bool) -> bool,
     ) -> CargoResult<Vec<PathBuf>> {
         let mut ret = Vec::new();
         PathSource::walk(pkg.root(), &mut ret, true, filter)?;
@@ -390,39 +386,46 @@ impl<'cfg> PathSource<'cfg> {
         path: &Path,
         ret: &mut Vec<PathBuf>,
         is_root: bool,
-        filter: &mut dyn FnMut(&Path, bool) -> CargoResult<bool>,
+        filter: &mut dyn FnMut(&Path, bool) -> bool,
     ) -> CargoResult<()> {
-        let is_dir = path.is_dir();
-        if !is_root && !(*filter)(path, is_dir)? {
-            return Ok(());
-        }
-        if !is_dir {
-            ret.push(path.to_path_buf());
-            return Ok(());
-        }
-        // Don't recurse into any sub-packages that we have.
-        if !is_root && path.join("Cargo.toml").exists() {
-            return Ok(());
-        }
+        let walkdir = WalkDir::new(path)
+            .follow_links(true)
+            .into_iter()
+            .filter_entry(|entry| {
+                let path = entry.path();
+                let at_root = is_root && entry.depth() == 0;
+                let is_dir = entry.file_type().is_dir();
+
+                if !at_root && !filter(path, is_dir) {
+                    return false;
+                }
 
-        // For package integration tests, we need to sort the paths in a deterministic order to
-        // be able to match stdout warnings in the same order.
-        //
-        // TODO: drop `collect` and sort after transition period and dropping warning tests.
-        // See rust-lang/cargo#4268 and rust-lang/cargo#4270.
-        let mut entries: Vec<PathBuf> = fs::read_dir(path)
-            .with_context(|| format!("cannot read {:?}", path))?
-            .map(|e| e.unwrap().path())
-            .collect();
-        entries.sort_unstable_by(|a, b| a.as_os_str().cmp(b.as_os_str()));
-        for path in entries {
-            let name = path.file_name().and_then(|s| s.to_str());
-            if is_root && name == Some("target") {
-                // Skip Cargo artifacts.
-                continue;
+                if !is_dir {
+                    return true;
+                }
+
+                // Don't recurse into any sub-packages that we have.
+                if !at_root && path.join("Cargo.toml").exists() {
+                    return false;
+                }
+
+                // Skip root Cargo artifacts.
+                if is_root
+                    && entry.depth() == 1
+                    && path.file_name().and_then(|s| s.to_str()) == Some("target")
+                {
+                    return false;
+                }
+
+                true
+            });
+        for entry in walkdir {
+            let entry = entry?;
+            if !entry.file_type().is_dir() {
+                ret.push(entry.path().to_path_buf());
             }
-            PathSource::walk(&path, ret, false, filter)?;
         }
+
         Ok(())
     }
 

tests/testsuite/build_script.rs

@@ -4653,7 +4653,7 @@ Caused by:
   failed to determine list of files in [..]/foo
 
 Caused by:
-  cannot read \"[..]/foo/secrets\"
+  IO error for operation on [..]/foo/secrets: [..]
 
 Caused by:
   [..]

tests/testsuite/package.rs

@@ -794,10 +794,10 @@ fn broken_symlink() {
         .with_status(101)
         .with_stderr_contains(
             "\
-error: failed to prepare local package for uploading
+error: failed to determine list of files [..]/foo
 
 Caused by:
-  failed to open for archiving: `[..]foo.rs`
+  IO error for operation on [..]/foo/src/foo.rs: [..]
 
 Caused by:
   [..]
@@ -826,6 +826,28 @@ fn package_symlink_to_dir() {
         .run();
 }
 
+#[cargo_test]
+/// Tests if a symlink to ancestor causes filesystem loop error.
+///
+/// This test requires you to be able to make symlinks.
+/// For windows, this may require you to enable developer mode.
+fn filesystem_loop() {
+    if !symlink_supported() {
+        return;
+    }
+
+    project()
+        .file("src/main.rs", r#"fn main() { println!("hello"); }"#)
+        .symlink_dir("a/b", "a/b/c/d/foo")
+        .build()
+        .cargo("package -v")
+        .with_status(101)
+        .with_stderr_contains(
+            "  File system loop found: [..]/a/b/c/d/foo points to an ancestor [..]/a/b",
+        )
+        .run();
+}
+
 #[cargo_test]
 fn do_not_package_if_repository_is_dirty() {
     let p = project().build();
@@ -1798,7 +1820,8 @@ fn package_restricted_windows() {
         .build();
 
     p.cargo("package")
-        .with_stderr(
+        // use unordered here because the order of the warning is different on each platform.
+        .with_stderr_unordered(
             "\
 [WARNING] file src/aux/mod.rs is a reserved Windows filename, it will not work on Windows platforms
 [WARNING] file src/con.rs is a reserved Windows filename, it will not work on Windows platforms