From 01ab3b2d6bfc97ff8c5ae68a9cf6080af738155e Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 4 Feb 2025 00:28:36 -0500 Subject: [PATCH 1/3] fix: turn file system looop warning into a debug log File system loops are pretty common on Unix platforms. --- src/cargo/sources/path.rs | 34 ++++++++++++++-------------------- tests/testsuite/build.rs | 1 - tests/testsuite/package.rs | 17 ++++++++++++++--- 3 files changed, 28 insertions(+), 24 deletions(-) diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index c1e81528fbd..54938d55d60 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -95,7 +95,7 @@ impl<'gctx> PathSource<'gctx> { /// `package.exclude` to filter the list of files. #[tracing::instrument(skip_all)] pub fn list_files(&self, pkg: &Package) -> CargoResult> { - list_files(pkg, self.gctx) + list_files(pkg) } /// Gets the last modified file in a package. @@ -106,7 +106,7 @@ impl<'gctx> PathSource<'gctx> { self.path ))); } - last_modified_file(&self.path, pkg, self.gctx) + last_modified_file(&self.path, pkg) } /// Returns the root path of this source. @@ -279,7 +279,7 @@ impl<'gctx> RecursivePathSource<'gctx> { /// use other methods like `.gitignore`, `package.include`, or /// `package.exclude` to filter the list of files. pub fn list_files(&self, pkg: &Package) -> CargoResult> { - list_files(pkg, self.gctx) + list_files(pkg) } /// Gets the last modified file in a package. @@ -290,7 +290,7 @@ impl<'gctx> RecursivePathSource<'gctx> { self.path ))); } - last_modified_file(&self.path, pkg, self.gctx) + last_modified_file(&self.path, pkg) } /// Returns the root path of this source. @@ -556,8 +556,8 @@ fn first_package<'p>( /// are relevant for building this package, but it also contains logic to /// use other methods like `.gitignore`, `package.include`, or /// `package.exclude` to filter the list of files. -pub fn list_files(pkg: &Package, gctx: &GlobalContext) -> CargoResult> { - _list_files(pkg, gctx).with_context(|| { +pub fn list_files(pkg: &Package) -> CargoResult> { + _list_files(pkg).with_context(|| { format!( "failed to determine list of files in {}", pkg.root().display() @@ -566,7 +566,7 @@ pub fn list_files(pkg: &Package, gctx: &GlobalContext) -> CargoResult CargoResult> { +fn _list_files(pkg: &Package) -> CargoResult> { let root = pkg.root(); let no_include_option = pkg.manifest().include().is_empty(); let git_repo = if no_include_option { @@ -626,11 +626,11 @@ fn _list_files(pkg: &Package, gctx: &GlobalContext) -> CargoResult bool, - gctx: &GlobalContext, ) -> CargoResult> { debug!("list_files_gix {}", pkg.package_id()); let options = repo @@ -820,10 +819,10 @@ fn list_files_gix( // .git repositories. match gix::open(&file_path) { Ok(sub_repo) => { - files.extend(list_files_gix(pkg, &sub_repo, filter, gctx)?); + files.extend(list_files_gix(pkg, &sub_repo, filter)?); } Err(_) => { - list_files_walk(&file_path, &mut files, false, filter, gctx)?; + list_files_walk(&file_path, &mut files, false, filter)?; } } } else if (filter)(&file_path, is_dir) { @@ -859,7 +858,6 @@ fn list_files_walk( ret: &mut Vec, is_root: bool, filter: &dyn Fn(&Path, bool) -> bool, - gctx: &GlobalContext, ) -> CargoResult<()> { let walkdir = WalkDir::new(path) .follow_links(true) @@ -933,7 +931,7 @@ fn list_files_walk( } } Err(err) if err.loop_ancestor().is_some() => { - gctx.shell().warn(err)?; + debug!(%err); } Err(err) => match err.path() { // If an error occurs with a path, filter it again. @@ -957,14 +955,10 @@ fn list_files_walk( } /// Gets the last modified file in a package. -fn last_modified_file( - path: &Path, - pkg: &Package, - gctx: &GlobalContext, -) -> CargoResult<(FileTime, PathBuf)> { +fn last_modified_file(path: &Path, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> { let mut max = FileTime::zero(); let mut max_path = PathBuf::new(); - for file in list_files(pkg, gctx).with_context(|| { + for file in list_files(pkg).with_context(|| { format!( "failed to determine the most recently modified file in {}", pkg.root().display() diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 5b5ad0dfc3c..e92a6a87265 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -2127,7 +2127,6 @@ fn ignore_broken_symlinks() { p.cargo("build") .with_stderr_data(str![[r#" -[WARNING] File system loop found: [ROOT]/foo/a/b/c/d/foo points to an ancestor [ROOT]/foo/a/b [COMPILING] foo v0.5.0 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index ae1c5d71024..86bb5881fbc 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -1043,9 +1043,20 @@ fn filesystem_loop() { .build() .cargo("package -v") .with_stderr_data(str![[r#" -... -[WARNING] File system loop found: [ROOT]/foo/a/b/c/d/foo points to an ancestor [ROOT]/foo/a/b -... +[WARNING] manifest has no description, license, license-file, documentation, homepage or repository. +See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info. +[WARNING] found (git) Cargo.toml ignored at `target/tmp/cit/t0/foo/Cargo.toml` in workdir `/Users/whlo/dev/cargo/` +[PACKAGING] foo v0.0.1 ([ROOT]/foo) +[ARCHIVING] Cargo.lock +[ARCHIVING] Cargo.toml +[ARCHIVING] Cargo.toml.orig +[ARCHIVING] src/main.rs +[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed) +[VERIFYING] foo v0.0.1 ([ROOT]/foo) +[COMPILING] foo v0.0.1 ([ROOT]/foo/target/package/foo-0.0.1) +[RUNNING] `rustc [..]` +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + "#]]) .run(); } From 1a425a2bbe515be64af3057aa85b81ed03fc401c Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 4 Feb 2025 00:41:05 -0500 Subject: [PATCH 2/3] test: add more file system loop This is a preparation for the next fix to show the behavior change. --- tests/testsuite/package.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 86bb5881fbc..6d0da4c0852 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -1039,9 +1039,12 @@ fn filesystem_loop() { project() .file("src/main.rs", r#"fn main() { println!("hello"); }"#) + .symlink_dir("a/b", "a/b/c/foo") .symlink_dir("a/b", "a/b/c/d/foo") + .symlink_dir("a/b", "a/b/c/d/e/foo") .build() .cargo("package -v") + .env("__CARGO_TEST_FS_LOOP_LIMIT_DO_NOT_USE_THIS", "2") .with_stderr_data(str![[r#" [WARNING] manifest has no description, license, license-file, documentation, homepage or repository. See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info. From bd7d4ee233af14132c0f70619a66e3a20dbdd3ee Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 4 Feb 2025 00:46:54 -0500 Subject: [PATCH 3/3] fix: detect file system loop limit and bail out For #9528 if the loop in inside `/sys/block` it would be an infinite loop. In that case Cargo is better bailing out instead of emitting endless warnings. We set a relativelly large limit to ensure there is any legitimate reason to have file system loop. --- src/cargo/sources/path.rs | 16 ++++++++++++++++ tests/testsuite/package.rs | 16 +++++----------- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index 54938d55d60..1735593ae61 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -895,6 +895,13 @@ fn list_files_walk( true }); + // ALLOWED: For testing cargo itself only. + #[allow(clippy::disallowed_methods)] + let loop_count_limit = std::env::var("__CARGO_TEST_FS_LOOP_LIMIT_DO_NOT_USE_THIS") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(1024); + let mut fs_loop = HashMap::new(); let mut current_symlink_dir = None; for entry in walkdir { match entry { @@ -932,6 +939,15 @@ fn list_files_walk( } Err(err) if err.loop_ancestor().is_some() => { debug!(%err); + let ancestor = err.loop_ancestor().unwrap(); + let loop_count = fs_loop.entry(ancestor.to_path_buf()).or_insert(0u32); + *loop_count += 1; + if *loop_count > loop_count_limit { + anyhow::bail!( + "file system loop detected at `{}` (exceeded limit of {loop_count_limit})", + ancestor.display(), + ); + } } Err(err) => match err.path() { // If an error occurs with a path, filter it again. diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 6d0da4c0852..42495fe8640 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -1045,20 +1045,14 @@ fn filesystem_loop() { .build() .cargo("package -v") .env("__CARGO_TEST_FS_LOOP_LIMIT_DO_NOT_USE_THIS", "2") + .with_status(101) .with_stderr_data(str![[r#" [WARNING] manifest has no description, license, license-file, documentation, homepage or repository. See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info. -[WARNING] found (git) Cargo.toml ignored at `target/tmp/cit/t0/foo/Cargo.toml` in workdir `/Users/whlo/dev/cargo/` -[PACKAGING] foo v0.0.1 ([ROOT]/foo) -[ARCHIVING] Cargo.lock -[ARCHIVING] Cargo.toml -[ARCHIVING] Cargo.toml.orig -[ARCHIVING] src/main.rs -[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed) -[VERIFYING] foo v0.0.1 ([ROOT]/foo) -[COMPILING] foo v0.0.1 ([ROOT]/foo/target/package/foo-0.0.1) -[RUNNING] `rustc [..]` -[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s +[ERROR] failed to determine list of files in [ROOT]/foo + +Caused by: + file system loop detected at `[ROOT]/foo/a/b` (exceeded limit of 2) "#]]) .run();