chmod, chown: check --preserve-root on the directory actually descended into - #14972
Open
abendrothj wants to merge 2 commits into
Open
abendrothj wants to merge 2 commits into
abendrothj wants to merge 2 commits into
Conversation
Merging this PR will improve performance by 12.57%
Performance Changes
Tip Curious why performance improved? Comment Comparing 
Footnotes
|
|
GNU testsuite comparison: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Author
|
Opened #14990. |
The recursive operand's --preserve-root check looked the path up again, separately from the stat that the operand's descriptor is then verified against. A rename landing between the two let that check see a harmless directory while the chown and the descent went into "/" under -H or -L. Also decide on that stat, through a new uucore::fs::dev_ino_is_root_dir. The path lookup stays, so a symlink to "/" that the stat did not follow is still reported as before.
The operand of a recursive chmod was checked against --preserve-root by path, changed by path and then opened by path to descend, so a rename landing between those steps could send the mode change and the descent into "/" under -H, or through a symlink swapped in under -P. Open the operand first, check that descriptor is not "/", and change the mode through it. The directory is then opened again to descend, as GNU does, so the new mode still decides whether it can be read, and the descent goes ahead only if that is the same directory. A directory that cannot be read before its mode change, and -H with --no-dereference, still go by path.
abendrothj
force-pushed
the
chmod-chown-operand-by-fd
branch
from
October 1, 2026 01:17
b5edd8c to
ab7438f
Compare
Contributor
Author
|
The SELinux GNU job failed while booting its VM, before any test ran; unrelated to this change. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The recursive operand of chmod, chown and chgrp was checked against --preserve-root with its own path lookup, separate from the one the change and the descent used. A rename landing in between could send a -H/-L recursion into "/" after the check had passed.
chown/chgrp now also decide on the stat that the operand's descriptor is already verified against. chmod opens the operand, checks that descriptor, and changes its mode through it; it still reopens the directory to descend, like GNU, and only continues if it is the same directory. Under -P this also stops the operand's own mode change from following a symlink swapped in at the directory's name.
Closes #14990