From 80f7bb219573addc05be68e9b7c78a12601e7c27 Mon Sep 17 00:00:00 2001 From: Nikola Lukovic Date: Tue, 14 Apr 2026 11:39:12 +0200 Subject: [PATCH 1/4] chmod: detect symlink cycles and exit early just like GNU does it --- src/uu/chmod/src/chmod.rs | 97 +++++++++++++++++++++++++++++++------ tests/by-util/test_chmod.rs | 23 +++++++++ 2 files changed, 105 insertions(+), 15 deletions(-) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 6eb9add77ed..a008e014417 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -6,6 +6,7 @@ // spell-checker:ignore (ToDO) Chmoder cmode fmode fperm fref ugoa RFILE RFILE's use clap::{Arg, ArgAction, Command}; +use std::collections::HashSet; use std::ffi::OsString; use std::fs; use std::os::unix::fs::{MetadataExt, PermissionsExt}; @@ -13,7 +14,7 @@ use std::path::{Path, PathBuf}; use thiserror::Error; use uucore::display::Quotable; use uucore::error::{ExitCode, UError, UResult, USimpleError, UUsageError, set_exit_code}; -use uucore::fs::display_permissions_unix; +use uucore::fs::{FileInformation, display_permissions_unix}; use uucore::mode; use uucore::perms::{TraverseSymlinks, configure_symlink_and_recursion}; @@ -335,6 +336,7 @@ impl Chmoder { &self, path: &Path, is_command_line_arg: bool, + ancestors: &mut HashSet, ) -> UResult<()> { let should_follow_symlink = match self.traverse_symlinks { TraverseSymlinks::All => true, @@ -347,7 +349,7 @@ impl Chmoder { } match fs::metadata(path) { - Ok(meta) if meta.is_dir() => self.walk_dir_with_context(path, false), + Ok(meta) if meta.is_dir() => self.walk_dir_with_context(path, false, ancestors), Ok(_) => { // It's a file symlink, chmod it self.chmod_file(path) @@ -403,7 +405,10 @@ impl Chmoder { return Err(ChmodError::PreserveRoot("/".into()).into()); } if self.recursive { - r = self.walk_dir_with_context(file, true).and(r); + let mut ancestors = HashSet::new(); + r = self + .walk_dir_with_context(file, true, &mut ancestors) + .and(r); } else { r = self.chmod_file(file).and(r); } @@ -417,7 +422,12 @@ impl Chmoder { // Non-safe traversal implementation for platforms without safe_traversal support #[cfg(any(not(unix), target_os = "redox"))] - fn walk_dir_with_context(&self, file_path: &Path, is_command_line_arg: bool) -> UResult<()> { + fn walk_dir_with_context( + &self, + file_path: &Path, + is_command_line_arg: bool, + ancestors: &mut HashSet, + ) -> UResult<()> { let mut r = self.chmod_file(file_path); // Determine whether to traverse symlinks based on context and traversal mode @@ -429,6 +439,15 @@ impl Chmoder { // If the path is a directory (or we should follow symlinks), recurse into it if (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir() { + // Cycle detection: check if this directory (resolved) is an ancestor. + // Use dereference=true so a symlink-to-dir is identified by its target's inode. + if let Ok(info) = FileInformation::from_path(file_path, true) { + if ancestors.contains(&info) { + return r; + } + ancestors.insert(info); + } + // We buffer all paths in this dir to not keep too many fd's open during recursion let mut paths_in_this_dir = Vec::new(); @@ -445,22 +464,39 @@ impl Chmoder { #[cfg(not(unix))] { if path.is_symlink() { - r = self.handle_symlink_during_recursion(&path).and(r); + r = self + .handle_symlink_during_recursion(&path, ancestors) + .and(r); } else { - r = self.walk_dir_with_context(path.as_path(), false).and(r); + r = self + .walk_dir_with_context(path.as_path(), false, ancestors) + .and(r); } } #[cfg(target_os = "redox")] { - r = self.walk_dir_with_context(path.as_path(), false).and(r); + r = self + .walk_dir_with_context(path.as_path(), false, ancestors) + .and(r); } } + + // Backtrack: remove this directory from ancestors so sibling subtrees + // are not falsely treated as cycles. + if let Ok(info) = FileInformation::from_path(file_path, true) { + ancestors.remove(&info); + } } r } #[cfg(all(unix, not(target_os = "redox")))] - fn walk_dir_with_context(&self, file_path: &Path, is_command_line_arg: bool) -> UResult<()> { + fn walk_dir_with_context( + &self, + file_path: &Path, + is_command_line_arg: bool, + ancestors: &mut HashSet, + ) -> UResult<()> { let mut r = self.chmod_file(file_path); // Determine whether to traverse symlinks based on context and traversal mode @@ -474,7 +510,7 @@ impl Chmoder { if (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir() { match DirFd::open(file_path, SymlinkBehavior::Follow) { Ok(dir_fd) => { - r = self.safe_traverse_dir(&dir_fd, file_path).and(r); + r = self.safe_traverse_dir(&dir_fd, file_path, ancestors).and(r); } Err(err) => { // Handle permission denied errors with proper file path context @@ -490,9 +526,22 @@ impl Chmoder { } #[cfg(all(unix, not(target_os = "redox")))] - fn safe_traverse_dir(&self, dir_fd: &DirFd, dir_path: &Path) -> UResult<()> { + fn safe_traverse_dir( + &self, + dir_fd: &DirFd, + dir_path: &Path, + ancestors: &mut HashSet, + ) -> UResult<()> { let mut r = Ok(()); + // Cycle detection: check if this directory (resolved through any symlink) is an ancestor. + if let Ok(info) = FileInformation::from_path(dir_path, true) { + if ancestors.contains(&info) { + return r; + } + ancestors.insert(info); + } + let entries = dir_fd.read_dir()?; // Determine if we should follow symlinks (doesn't depend on entry_name) @@ -516,7 +565,12 @@ impl Chmoder { if entry_path.is_symlink() { r = self - .handle_symlink_during_safe_recursion(&entry_path, dir_fd, &entry_name) + .handle_symlink_during_safe_recursion( + &entry_path, + dir_fd, + &entry_name, + ancestors, + ) .and(r); } else { // For regular files and directories, chmod them. @@ -537,7 +591,9 @@ impl Chmoder { if meta.is_dir() { match dir_fd.open_subdir(&entry_name, SymlinkBehavior::Follow) { Ok(child_dir_fd) => { - r = self.safe_traverse_dir(&child_dir_fd, &entry_path).and(r); + r = self + .safe_traverse_dir(&child_dir_fd, &entry_path, ancestors) + .and(r); } Err(err) => { let error = if err.kind() == std::io::ErrorKind::PermissionDenied { @@ -551,6 +607,12 @@ impl Chmoder { } } } + + // Backtrack: remove this directory so sibling subtrees are not falsely flagged. + if let Ok(info) = FileInformation::from_path(dir_path, true) { + ancestors.remove(&info); + } + r } @@ -560,6 +622,7 @@ impl Chmoder { path: &Path, dir_fd: &DirFd, entry_name: &std::ffi::OsStr, + ancestors: &mut HashSet, ) -> UResult<()> { // During recursion, determine behavior based on traversal mode match self.traverse_symlinks { @@ -571,7 +634,7 @@ impl Chmoder { // followed by Follow is the intended behavior and not a TOCTOU concern. // Check if the symlink target is a directory, but handle dangling symlinks gracefully match fs::metadata(path) { - Ok(meta) if meta.is_dir() => self.walk_dir_with_context(path, false), + Ok(meta) if meta.is_dir() => self.walk_dir_with_context(path, false, ancestors), Ok(meta) => { // It's a file symlink, chmod it using safe traversal self.safe_chmod_file( @@ -626,9 +689,13 @@ impl Chmoder { } #[cfg(not(unix))] - fn handle_symlink_during_recursion(&self, path: &Path) -> UResult<()> { + fn handle_symlink_during_recursion( + &self, + path: &Path, + ancestors: &mut HashSet, + ) -> UResult<()> { // Use the common symlink handling logic - self.handle_symlink_during_traversal(path, false) + self.handle_symlink_during_traversal(path, false, ancestors) } fn chmod_file(&self, file: &Path) -> UResult<()> { diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index ac28c5ce50f..73e06033fe5 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -1440,3 +1440,26 @@ fn test_chmod_colored_output() { .stderr_contains("\x1b[31merreur\x1b[0m") // Red "erreur" in French .stderr_contains("\x1b[33m--invalid-option\x1b[0m"); // Yellow invalid option } + +#[test] +fn test_chmod_symlink_cycles() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.mkdir_all("a/b/c"); + at.symlink_dir("a", "a/b/c/d"); + + scene + .ucmd() + .arg("-vRL") + .arg("+r") + .arg("a") + .succeeds() + .stdout_contains_line("mode of 'a' retained as 0755 (rwxr-xr-x)") + .stdout_contains_line("mode of 'a/b' retained as 0755 (rwxr-xr-x)") + .stdout_contains_line("mode of 'a/b/c' retained as 0755 (rwxr-xr-x)") + .stdout_contains_line("mode of 'a/b/c/d' retained as 0755 (rwxr-xr-x)") + .stdout_does_not_contain("mode of 'a/b/c/d/b' retained as 0755 (rwxr-xr-x)") + .stdout_does_not_contain("mode of 'a/b/c/d/b/c' retained as 0755 (rwxr-xr-x)") + .stdout_does_not_contain("mode of 'a/b/c/d/b/c/d' retained as 0755 (rwxr-xr-x)"); +} From 4d9a802674d5f588e8a13654f537a514624eefb9 Mon Sep 17 00:00:00 2001 From: Nikola Lukovic Date: Tue, 14 Apr 2026 12:42:04 +0200 Subject: [PATCH 2/4] chown: detect symlink cycles and exit early just like GNU does it --- src/uucore/src/lib/features/perms.rs | 30 +++++++++++++++++-- tests/by-util/test_chmod.rs | 40 ++++++++++++++++--------- tests/by-util/test_chown.rs | 44 ++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 17 deletions(-) diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 865377872bd..7212552ce26 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -16,11 +16,15 @@ use clap::{Arg, ArgMatches, Command}; use libc::{gid_t, uid_t}; use options::traverse; +#[cfg(target_os = "linux")] +use std::collections::HashSet; use std::ffi::OsString; #[cfg(not(target_os = "linux"))] use walkdir::WalkDir; +#[cfg(target_os = "linux")] +use crate::features::fs::FileInformation; #[cfg(target_os = "linux")] use crate::features::safe_traversal::{DirFd, SymlinkBehavior}; @@ -450,13 +454,28 @@ impl ChownExecutor { return 1; }; + let mut ancestors = HashSet::new(); let mut ret = 0; - self.safe_traverse_dir(&dir_fd, root, &mut ret); + self.safe_traverse_dir(&dir_fd, root, &mut ret, &mut ancestors); ret } #[cfg(target_os = "linux")] - fn safe_traverse_dir(&self, dir_fd: &DirFd, dir_path: &Path, ret: &mut i32) { + fn safe_traverse_dir( + &self, + dir_fd: &DirFd, + dir_path: &Path, + ret: &mut i32, + ancestors: &mut HashSet, + ) { + // Cycle detection: resolve through symlinks so a symlink-to-ancestor is caught. + if let Ok(info) = FileInformation::from_path(dir_path, true) { + if ancestors.contains(&info) { + return; // cycle detected, stop silently + } + ancestors.insert(info); + } + // Read directory entries let entries = match dir_fd.read_dir() { Ok(entries) => entries, @@ -540,7 +559,7 @@ impl ChownExecutor { if meta.is_dir() && (follow || !meta.file_type().is_symlink()) { match dir_fd.open_subdir(&entry_name, SymlinkBehavior::Follow) { Ok(subdir_fd) => { - self.safe_traverse_dir(&subdir_fd, &entry_path, ret); + self.safe_traverse_dir(&subdir_fd, &entry_path, ret, ancestors); } Err(e) => { *ret = 1; @@ -555,6 +574,11 @@ impl ChownExecutor { } } } + + // Backtrack: remove this directory so sibling subtrees are not falsely flagged. + if let Ok(info) = FileInformation::from_path(dir_path, true) { + ancestors.remove(&info); + } } #[cfg(not(target_os = "linux"))] diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index 73e06033fe5..6001f0d4d2f 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -2,7 +2,7 @@ // // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (words) dirfd subdirs openat FDCWD +// spell-checker:ignore (words) dirfd subdirs openat FDCWD rwxr use std::fs::{OpenOptions, Permissions, metadata, set_permissions}; use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; @@ -1449,17 +1449,29 @@ fn test_chmod_symlink_cycles() { at.mkdir_all("a/b/c"); at.symlink_dir("a", "a/b/c/d"); - scene - .ucmd() - .arg("-vRL") - .arg("+r") - .arg("a") - .succeeds() - .stdout_contains_line("mode of 'a' retained as 0755 (rwxr-xr-x)") - .stdout_contains_line("mode of 'a/b' retained as 0755 (rwxr-xr-x)") - .stdout_contains_line("mode of 'a/b/c' retained as 0755 (rwxr-xr-x)") - .stdout_contains_line("mode of 'a/b/c/d' retained as 0755 (rwxr-xr-x)") - .stdout_does_not_contain("mode of 'a/b/c/d/b' retained as 0755 (rwxr-xr-x)") - .stdout_does_not_contain("mode of 'a/b/c/d/b/c' retained as 0755 (rwxr-xr-x)") - .stdout_does_not_contain("mode of 'a/b/c/d/b/c/d' retained as 0755 (rwxr-xr-x)"); + let result = scene.ucmd().arg("-vRL").arg("+r").arg("a").run(); + + if cfg!(target_os = "android") { + result + // cSpell:disable + .stdout_contains_line("mode of 'a' retained as 0700 (rwx------)") + .stdout_contains_line("mode of 'a/b' retained as 0700 (rwx------)") + .stdout_contains_line("mode of 'a/b/c' retained as 0700 (rwx------)") + .stdout_contains_line("mode of 'a/b/c/d' retained as 0700 (rwx------)") + .stdout_does_not_contain("mode of 'a/b/c/d/b' retained as 0700 (rwx------)") + .stdout_does_not_contain("mode of 'a/b/c/d/b/c' retained as 0700 (rwx------)") + .stdout_does_not_contain("mode of 'a/b/c/d/b/c/d' retained as 0700 (rwx------)"); + // cSpell:enable + } else { + result + // cSpell:disable + .stdout_contains_line("mode of 'a' retained as 0755 (rwxr-xr-x)") + .stdout_contains_line("mode of 'a/b' retained as 0755 (rwxr-xr-x)") + .stdout_contains_line("mode of 'a/b/c' retained as 0755 (rwxr-xr-x)") + .stdout_contains_line("mode of 'a/b/c/d' retained as 0755 (rwxr-xr-x)") + .stdout_does_not_contain("mode of 'a/b/c/d/b' retained as 0755 (rwxr-xr-x)") + .stdout_does_not_contain("mode of 'a/b/c/d/b/c' retained as 0755 (rwxr-xr-x)") + .stdout_does_not_contain("mode of 'a/b/c/d/b/c/d' retained as 0755 (rwxr-xr-x)"); + // cSpell:enable + } } diff --git a/tests/by-util/test_chown.rs b/tests/by-util/test_chown.rs index b5db811ac5a..268f879d9a0 100644 --- a/tests/by-util/test_chown.rs +++ b/tests/by-util/test_chown.rs @@ -943,3 +943,47 @@ fn test_chown_no_dereference_symlink_to_dir() { "dir's ctime should not have changed" ); } + +#[test] +fn test_chown_symlink_cycles() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let result = scene.cmd("whoami").run(); + if skipping_test_is_okay(&result, "whoami: cannot find name for user ID") { + return; + } + let user_name = String::from(result.stdout_str().trim()); + assert!(!user_name.is_empty()); + + at.mkdir_all("a/b/c"); + at.symlink_dir("a", "a/b/c/d"); + + let result = scene.ucmd().arg("-vRL").arg(&user_name).arg("a").run(); + + if cfg!(target_os = "macos") || cfg!(target_os = "openbsd") || cfg!(target_os = "android") { + result + .stderr_contains(format!("ownership of 'a' retained as {user_name}")) + .stderr_contains(format!("ownership of 'a/b' retained as {user_name}")) + .stderr_contains(format!("ownership of 'a/b/c' retained as {user_name}")) + .stderr_does_not_contain(format!("ownership of 'a/b/c/d' retained as {user_name}")) + .stderr_does_not_contain(format!("ownership of 'a/b/c/d/b' retained as {user_name}")) + .stderr_does_not_contain(format!( + "ownership of 'a/b/c/d/b/c' retained as {user_name}" + )); + } else { + result + .success() + .stderr_contains(format!("ownership of 'a' retained as {user_name}")) + .stderr_contains(format!("ownership of 'a/b' retained as {user_name}")) + .stderr_contains(format!("ownership of 'a/b/c' retained as {user_name}")) + .stderr_contains(format!("ownership of 'a/b/c/d' retained as {user_name}")) + .stderr_does_not_contain(format!("ownership of 'a/b/c/d/b' retained as {user_name}")) + .stderr_does_not_contain(format!( + "ownership of 'a/b/c/d/b/c' retained as {user_name}" + )) + .stderr_does_not_contain(format!( + "ownership of 'a/b/c/d/b/c/d' retained as {user_name}" + )); + } +} From d3fa5f0619bf30c44bbec38fbb30ad5315ac315c Mon Sep 17 00:00:00 2001 From: Nikola Lukovic Date: Sun, 31 May 2026 11:40:10 +0200 Subject: [PATCH 3/4] chmod, chown: reuse directory FileInformation during cycle-detection backtrack --- src/uu/chmod/src/chmod.rs | 31 +++++++++++--------- src/uucore/src/lib/features/fs.rs | 1 + src/uucore/src/lib/features/perms.rs | 15 ++++++---- tests/by-util/test_chmod.rs | 29 +++++++++++++++++++ tests/by-util/test_chown.rs | 42 ++++++++++++++++++++++++++++ 5 files changed, 99 insertions(+), 19 deletions(-) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index a008e014417..dca28639f9b 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -439,13 +439,15 @@ impl Chmoder { // If the path is a directory (or we should follow symlinks), recurse into it if (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir() { - // Cycle detection: check if this directory (resolved) is an ancestor. - // Use dereference=true so a symlink-to-dir is identified by its target's inode. - if let Ok(info) = FileInformation::from_path(file_path, true) { - if ancestors.contains(&info) { + // Cycle detection: identify this directory by its target's inode (resolved + // via dereference=true) once, and reuse it for the backtrack below so we + // don't stat the same directory twice. If it's already on the current path, + // it's a cycle. + let dir_info = FileInformation::from_path(file_path, true).ok(); + if let Some(info) = &dir_info { + if !ancestors.insert(info.clone()) { return r; } - ancestors.insert(info); } // We buffer all paths in this dir to not keep too many fd's open during recursion @@ -483,7 +485,7 @@ impl Chmoder { // Backtrack: remove this directory from ancestors so sibling subtrees // are not falsely treated as cycles. - if let Ok(info) = FileInformation::from_path(file_path, true) { + if let Some(info) = dir_info { ancestors.remove(&info); } } @@ -534,12 +536,14 @@ impl Chmoder { ) -> UResult<()> { let mut r = Ok(()); - // Cycle detection: check if this directory (resolved through any symlink) is an ancestor. - if let Ok(info) = FileInformation::from_path(dir_path, true) { - if ancestors.contains(&info) { - return r; + // Cycle detection: identify this directory by (dev, ino) via the already-open + // fd. Using the fd is TOCTOU-safe (no path re-resolution through symlinks) and + // avoids a redundant path walk. If it's already on the current path, it's a cycle. + let dir_info = FileInformation::from_file(dir_fd).ok(); + if let Some(info) = &dir_info { + if !ancestors.insert(info.clone()) { + return r; // cycle: this directory is already an ancestor } - ancestors.insert(info); } let entries = dir_fd.read_dir()?; @@ -608,8 +612,9 @@ impl Chmoder { } } - // Backtrack: remove this directory so sibling subtrees are not falsely flagged. - if let Ok(info) = FileInformation::from_path(dir_path, true) { + // Backtrack so sibling subtrees that legitimately reach the same directory + // (e.g. two symlinks to one dir) are not mistaken for cycles. + if let Some(info) = dir_info { ancestors.remove(&info); } diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index aece06bd604..776b946914b 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -38,6 +38,7 @@ macro_rules! has { } /// Information to uniquely identify a file +#[derive(Clone)] pub struct FileInformation( #[cfg(unix)] rustix::fs::Stat, #[cfg(windows)] winapi_util::file::Information, diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 7212552ce26..998cb00bdca 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -468,12 +468,14 @@ impl ChownExecutor { ret: &mut i32, ancestors: &mut HashSet, ) { - // Cycle detection: resolve through symlinks so a symlink-to-ancestor is caught. - if let Ok(info) = FileInformation::from_path(dir_path, true) { - if ancestors.contains(&info) { + // Cycle detection: identify this directory by (dev, ino) via the already-open + // fd. Using the fd is TOCTOU-safe (no path re-resolution through symlinks) and + // avoids a redundant path walk. If it's already on the current path, it's a cycle. + let dir_info = FileInformation::from_file(dir_fd).ok(); + if let Some(info) = &dir_info { + if !ancestors.insert(info.clone()) { return; // cycle detected, stop silently } - ancestors.insert(info); } // Read directory entries @@ -575,8 +577,9 @@ impl ChownExecutor { } } - // Backtrack: remove this directory so sibling subtrees are not falsely flagged. - if let Ok(info) = FileInformation::from_path(dir_path, true) { + // Backtrack so sibling subtrees that legitimately reach the same directory + // (e.g. two symlinks to one dir) are not mistaken for cycles. + if let Some(info) = dir_info { ancestors.remove(&info); } } diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index 6001f0d4d2f..e359ff66359 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -1475,3 +1475,32 @@ fn test_chmod_symlink_cycles() { // cSpell:enable } } + +#[test] +fn test_chmod_symlink_two_links_same_dir() { + // Two symlinks pointing at the same directory is NOT a cycle: neither link is + // an ancestor of the other, so the target's contents must be visited through + // *both* links (and through the real directory). This guards the backtracking + // in the cycle-detection logic against false positives. + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + // cSpell:disable + at.mkdir_all("base/realdir"); + at.touch("base/realdir/file"); + at.symlink_dir("base/realdir", "base/link1"); + at.symlink_dir("base/realdir", "base/link2"); + + // Assert on the path only (not the mode bits), so the test is robust to the + // file's umask-dependent permissions across platforms. + scene + .ucmd() + .arg("-vRL") + .arg("+r") + .arg("base") + .run() + .stdout_contains("mode of 'base/realdir/file'") + .stdout_contains("mode of 'base/link1/file'") + .stdout_contains("mode of 'base/link2/file'"); + // cSpell:enable +} diff --git a/tests/by-util/test_chown.rs b/tests/by-util/test_chown.rs index 268f879d9a0..c03682b4ec5 100644 --- a/tests/by-util/test_chown.rs +++ b/tests/by-util/test_chown.rs @@ -987,3 +987,45 @@ fn test_chown_symlink_cycles() { )); } } + +#[test] +fn test_chown_symlink_two_links_same_dir() { + // Two symlinks pointing at the same directory is NOT a cycle: neither link is + // an ancestor of the other, so the target's contents must be visited through + // *both* links. This guards the backtracking in the linux safe-traversal + // cycle detection against false positives. + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let result = scene.cmd("whoami").run(); + if skipping_test_is_okay(&result, "whoami: cannot find name for user ID") { + return; + } + let user_name = String::from(result.stdout_str().trim()); + assert!(!user_name.is_empty()); + + // cSpell:disable + at.mkdir_all("base/realdir"); + at.touch("base/realdir/file"); + at.symlink_dir("base/realdir", "base/link1"); + at.symlink_dir("base/realdir", "base/link2"); + + let result = scene.ucmd().arg("-vRL").arg(&user_name).arg("base").run(); + + // Only linux uses the safe-traversal cycle detection that this test exercises; + // other platforms fall back to walkdir with its own loop handling. + if cfg!(target_os = "linux") { + result + .success() + .stderr_contains(format!( + "ownership of 'base/realdir/file' retained as {user_name}" + )) + .stderr_contains(format!( + "ownership of 'base/link1/file' retained as {user_name}" + )) + .stderr_contains(format!( + "ownership of 'base/link2/file' retained as {user_name}" + )); + } + // cSpell:enable +} From 65c0dc87476a4310ade81ffefc99eee83f510794 Mon Sep 17 00:00:00 2001 From: Nikola Lukovic Date: Mon, 1 Jun 2026 13:40:10 +0200 Subject: [PATCH 4/4] chmod: test_chmod_symlink_cycles pin dir modes --- tests/by-util/test_chmod.rs | 46 +++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 25 deletions(-) diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index e359ff66359..2541a9f5f0b 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -1449,31 +1449,27 @@ fn test_chmod_symlink_cycles() { at.mkdir_all("a/b/c"); at.symlink_dir("a", "a/b/c/d"); - let result = scene.ucmd().arg("-vRL").arg("+r").arg("a").run(); - - if cfg!(target_os = "android") { - result - // cSpell:disable - .stdout_contains_line("mode of 'a' retained as 0700 (rwx------)") - .stdout_contains_line("mode of 'a/b' retained as 0700 (rwx------)") - .stdout_contains_line("mode of 'a/b/c' retained as 0700 (rwx------)") - .stdout_contains_line("mode of 'a/b/c/d' retained as 0700 (rwx------)") - .stdout_does_not_contain("mode of 'a/b/c/d/b' retained as 0700 (rwx------)") - .stdout_does_not_contain("mode of 'a/b/c/d/b/c' retained as 0700 (rwx------)") - .stdout_does_not_contain("mode of 'a/b/c/d/b/c/d' retained as 0700 (rwx------)"); - // cSpell:enable - } else { - result - // cSpell:disable - .stdout_contains_line("mode of 'a' retained as 0755 (rwxr-xr-x)") - .stdout_contains_line("mode of 'a/b' retained as 0755 (rwxr-xr-x)") - .stdout_contains_line("mode of 'a/b/c' retained as 0755 (rwxr-xr-x)") - .stdout_contains_line("mode of 'a/b/c/d' retained as 0755 (rwxr-xr-x)") - .stdout_does_not_contain("mode of 'a/b/c/d/b' retained as 0755 (rwxr-xr-x)") - .stdout_does_not_contain("mode of 'a/b/c/d/b/c' retained as 0755 (rwxr-xr-x)") - .stdout_does_not_contain("mode of 'a/b/c/d/b/c/d' retained as 0755 (rwxr-xr-x)"); - // cSpell:enable - } + // Pin the directory modes so the assertions below don't depend on the ambient + // umask (a fresh mkdir under umask 077/027 would not be 0755). + at.set_mode("a", 0o755); + at.set_mode("a/b", 0o755); + at.set_mode("a/b/c", 0o755); + + scene + .ucmd() + .arg("-vRL") + .arg("+r") + .arg("a") + .run() + // cSpell:disable + .stdout_contains_line("mode of 'a' retained as 0755 (rwxr-xr-x)") + .stdout_contains_line("mode of 'a/b' retained as 0755 (rwxr-xr-x)") + .stdout_contains_line("mode of 'a/b/c' retained as 0755 (rwxr-xr-x)") + .stdout_contains_line("mode of 'a/b/c/d' retained as 0755 (rwxr-xr-x)") + .stdout_does_not_contain("mode of 'a/b/c/d/b' retained as 0755 (rwxr-xr-x)") + .stdout_does_not_contain("mode of 'a/b/c/d/b/c' retained as 0755 (rwxr-xr-x)") + .stdout_does_not_contain("mode of 'a/b/c/d/b/c/d' retained as 0755 (rwxr-xr-x)"); + // cSpell:enable } #[test]