Be resilient to most IO error and filesystem loop while walking dirs

[weihanglo]

Let PathSource::walk be resilient to most IO errors and filesystem loop.

This PR also

• Add a test validating the resilience against filesystem loop to prevent regression.
• Emit warning when filesystem loop found while walking the filesystem. This is the only way I can think of now to solve Cargo gets lost in the root of a Linux filesystem #9528

Fixes #10213.

[rust-highfive]

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

Is there a way we could detect and filter these cases from walkdir, such as ignoring broken symlinks entirely?

[weihanglo]

Indeed we can recover from broken symlink and some IO errors. For filesystem loop, I guess we can simply emit a warning inside fn walk and skip the entry? At least user stuck in root can be informed that a lot filesystem loops happened.

The code snippet need to change as below:

        for entry in walkdir {
            match entry {
                Ok(entry) => {
                    if !entry.file_type().is_dir() {
                        ret.push(entry.into_path());
                    }
                },
                Err(e) => {
                    if e.loop_ancestor().is_some() {
                        // emit some warning!!!!!
                        continue
                    }
                    if let Some(path) = e.path() {
                        ret.push(path.to_path_buf());
                    }
                }
            }
        }

[alexcrichton]

If a loop is as common as node_modules installations we probably don't want to warn about it but otherwise handling it somewhat gracefully seems reasonable.

[weihanglo]

From my own experience, a normal Node.js project should not have any nested node_modules in its dependencies, so the warning might be rare. I'm ok to remove the warning though if so, #9528 needs a reopen.

[alexcrichton]

I'm curious, what's going on here? This looks like it's ignoring all errors that are not permission denied?

[weihanglo]

If cargo doesn't handle permission error intentionally, this EACCES test case might pass silently. The old behavior before adopting walkdir returns errors from fs::read_dir calls directly, as well as it just unwraps DirEntry without handling Result.

cargo/src/cargo/sources/path.rs

Lines 413 to 416 in a359ce1

	let mut entries: Vec<PathBuf> = fs::read_dir(path)
	.with_context(|| format!("cannot read {:?}", path))?
	.map(|e| e.unwrap().path())
	.collect();

With walkdir, cargo cannot tell whether the error is permission error or a broken link. Generally, walkdir emits three kinds of errors:

1. Symlink loop errors: Cargo already takes care of it and shows a warning in this PR.
2. IO errors with a path: I guess cargo can recover from the path, and later let the caller decide to ignore it or if the caller does access the path and hits the error.
3. IO errros without a path: Cargo can simply propagate it to callers. This is rare because walkdir only emits this kind for either opening the file to check symlink loop or handling the Result<DirEntry> generated from read_dir().

I've updated this PR to reflect aforementioned change. There is an enhancement can do such as emitting warnings for those recoverable errors, but I doubt its usefulness.

[alexcrichton]

Ah ok if we don't expect this to be common then a warning seems fine and we can adjust later if necessary.

[alexcrichton]

@bors: r+

Ok sounds good to me!

[bors]

📌 Commit d92dcea has been approved by alexcrichton

[bors]

⌛ Testing commit d92dcea with merge 2478331...

[bors]

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 2478331 to master...

[weihanglo]

Should this fix be included in next beta? I feel like it should or we need to backport it after 1.59-beta released.

[alexcrichton]

Yeah that seems reasonable to me, would you be up for preparing the PR?