Skip to content

chmod/chown: symlink cycles detection#11805

Merged
sylvestre merged 4 commits into
uutils:mainfrom
nikolalukovic:fix/11614
Jun 1, 2026
Merged

chmod/chown: symlink cycles detection#11805
sylvestre merged 4 commits into
uutils:mainfrom
nikolalukovic:fix/11614

Conversation

@nikolalukovic
Copy link
Copy Markdown
Contributor

@nikolalukovic nikolalukovic commented Apr 14, 2026

Attempts to fix: #11614

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 14, 2026 •
edited
Loading

GNU testsuite comparison:

Skip an intermittent issue tests/date/date-locale-hour (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/date/resolution (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/tail-n0f (fails in this run but passes in the 'main' branch)
Note: The gnu test tests/basenc/bounded-memory is now being skipped but was previously passing.
Note: The gnu test tests/cut/bounded-memory is now being skipped but was previously passing.
Note: The gnu test tests/env/env-signal-handler was skipped on 'main' but is now failing.

@nikolalukovic nikolalukovic force-pushed the fix/11614 branch 3 times, most recently from e17c9e9 to 1990e91 Compare April 14, 2026 11:49
@nikolalukovic nikolalukovic marked this pull request as ready for review April 14, 2026 12:12
@nikolalukovic
Copy link
Copy Markdown
Contributor Author

nikolalukovic commented Apr 14, 2026 •
edited
Loading

Failing tests are unrelated to these changes.
All tests pass now.

@nikolalukovic nikolalukovic force-pushed the fix/11614 branch 2 times, most recently from 168aac1 to 16ecfdd Compare April 14, 2026 13:42
@nikolalukovic
Copy link
Copy Markdown
Contributor Author

nikolalukovic commented Apr 17, 2026

@sylvestre can you please take a look?

blakshya
blakshya reviewed Apr 22, 2026
View reviewed changes
Comment thread src/uu/chmod/src/chmod.rs
@@ -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();
Copy link
Copy Markdown

@blakshya blakshya Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
I'm new here and found this PR when looking at how first issues are handled. Please correct me if i'm missing something.

Looking at GNU implementation, they push down the onus of cycle detection to a generic read function and only reference a cycle detection flag. That looks modular enough to not pass a new hash set everytime?

How would this be extended extended to other places when required?

@nikolalukovic
Copy link
Copy Markdown
Contributor Author

nikolalukovic commented Apr 23, 2026

@sylvestre bump

@nikolalukovic
Copy link
Copy Markdown
Contributor Author

nikolalukovic commented Apr 30, 2026

bump² @sylvestre

@nikolalukovic
Copy link
Copy Markdown
Contributor Author

nikolalukovic commented May 8, 2026

@cakebaker I hope it's okay to ping you as well like this

sylvestre
sylvestre reviewed May 11, 2026
View reviewed changes
Comment thread src/uu/chmod/src/chmod.rs Outdated
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) {
Copy link
Copy Markdown
Contributor

@sylvestre sylvestre May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems duplicated from above, no ?

@sylvestre
Copy link
Copy Markdown
Contributor

sylvestre commented May 11, 2026

it seems that you have two stat() calls per directory (insert + remove) on top of the existing read_dir.

Also, please add a "two symlinks to same dir = not a cycle" test.

sylvestre
sylvestre requested changes May 16, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment

sylvestre
sylvestre reviewed May 31, 2026
View reviewed changes
Comment thread src/uu/chmod/src/chmod.rs Outdated
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) {
Copy link
Copy Markdown
Contributor

@sylvestre sylvestre May 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TOCTOU re-resolution in the safe-traversal path. Inside safe_traverse_dir (both chmod.rs:526 and perms.rs:461) the code already holds dir_fd, an open fd to exactly this directory - but cycle identity is computed with FileInformation::from_path(dir_path, true), which re-resolves the path through symlinks. That reintroduces the very race window safe_traversal exists to close. Use FileInformation::from_file(dir_fd) instead - it's TOCTOU-safe and avoids a redundant path walk. (DirFd: AsFd and FileInformation::from_file(&impl AsFd) both already exist.)

Copy link
Copy Markdown
Contributor Author

@nikolalukovic nikolalukovic May 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it should be better. Please take a look.

@nikolalukovic nikolalukovic force-pushed the fix/11614 branch 2 times, most recently from ea423d1 to 2639aa1 Compare May 31, 2026 09:47
@nikolalukovic nikolalukovic requested a review from sylvestre May 31, 2026 10:29
@nikolalukovic
Copy link
Copy Markdown
Contributor Author

nikolalukovic commented May 31, 2026

openbsd test failing doesn't seem to have anything to do with these changes

sylvestre
sylvestre reviewed May 31, 2026
View reviewed changes
Comment thread tests/by-util/test_chmod.rs Outdated
} else {
result
// cSpell:disable
.stdout_contains_line("mode of 'a' retained as 0755 (rwxr-xr-x)")
Copy link
Copy Markdown
Contributor

@sylvestre sylvestre May 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This asserts 0755, which assumes umask 022 - set the dir mode explicitly so it doesn't fail under a different umask.

Copy link
Copy Markdown
Contributor Author

@nikolalukovic nikolalukovic Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@nikolalukovic nikolalukovic requested a review from sylvestre June 1, 2026 13:12
@sylvestre sylvestre merged commit 3fbcb79 into uutils:main Jun 1, 2026
170 of 172 checks passed
@sylvestre
Copy link
Copy Markdown
Contributor

sylvestre commented Jun 1, 2026

well done

@nikolalukovic nikolalukovic deleted the fix/11614 branch June 1, 2026 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sylvestre sylvestre Awaiting requested review from sylvestre

1 more reviewer

@blakshya blakshya blakshya left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

chmod and chown do not detect symlink cycles

3 participants

@nikolalukovic @sylvestre @blakshya