Repository navigation
install: create ancestor directories at 0755 regardless of umask - #12715
abendrothj wants to merge 4 commits into
Conversation
|
GNU testsuite comparison: |
Merging this PR will regress 2 benchmarks
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
9cfc786 to
11d3c90
Compare
|
Rebased onto current main, dropped an empty |
11d3c90 to
81625e6
Compare
There was a problem hiding this comment.
Pull request overview
This PR aligns install with GNU behavior by zeroing the process umask early so that ancestor directories created via -D/-d are reliably created at DEFAULT_MODE (0755), independent of the caller’s umask.
Changes:
- Add
uucore::mode::zero_umask()(unix-only) to set umask to 0. - Call
zero_umask()at the start ofinstall::uumainand adjust-ddirectory creation to useDirBuilder::mode(DEFAULT_MODE)(instead ofcreate_dir_all). - Update ancestor-mode tests to assert GNU’s guaranteed
0755behavior for ancestor directories.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/by-util/test_install.rs | Updates ancestor permission assertions to match GNU’s fixed 0755 guarantee. |
| src/uucore/src/lib/features/safe_traversal.rs | Updates docs to clarify umask interaction and the need to zero umask for exact modes. |
| src/uucore/src/lib/features/mode.rs | Adds a unix-only zero_umask() helper based on rustix. |
| src/uu/install/src/install.rs | Zeros umask early; switches -d directory creation to DirBuilder::mode(DEFAULT_MODE); updates comments for new semantics. |
Suppressed comments (2)
tests/by-util/test_install.rs:108
- These assertions now hard-code the expected ancestor mode, but the test no longer exercises the original bug (behavior under a restrictive inherited umask). On typical CI umask=0022, ancestors would still be 0755 even if install didn’t zero umask, so this can pass while regressing. Set a restrictive umask for the spawned
installprocess (and keep the 0755 assertions) to ensure the test fails if zero-umask behavior is removed.
This issue also appears on line 143 of the same file.
ucmd.args(&[mode_arg, directories_arg, target_dir])
.succeeds()
.no_stderr();
tests/by-util/test_install.rs:146
- This test hard-codes 0755 for ancestor dirs, but it doesn’t currently validate the fix under a restrictive inherited umask. On common umask=0022, ancestors would still be 0755 even if
installstopped zeroing umask, so this could pass while regressing. Force a restrictive umask for the spawnedinstallprocess to ensure the test actually exercises the behavior change.
// GNU install zeros umask at startup and creates ancestor dirs at exactly
// 0755 (DEFAULT_MODE). --mode applies only to the final target.
assert_eq!(0o40_755_u32, at.metadata(ancestor1).permissions().mode());
assert_eq!(0o40_755_u32, at.metadata(ancestor2).permissions().mode());
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/uucore/src/lib/features/safe_traversal.rs:1307
- This test asserts the created file mode is exactly 0o600, but file creation modes are always masked by the process umask; a restrictive umask could legitimately clear bits and make this assertion fail even if open_file_at_with_mode honors the requested mode. To keep the test robust across environments, assert that no permission bits outside the requested mode are set (umask can only remove bits).
assert_eq!(mode, 0o600);
src/uucore/src/lib/features/safe_traversal.rs:453
- The new open_file_at_with_mode docs imply the provided mode is applied as-is, but the kernel still applies the process umask to O_CREAT modes. Calling this out helps prevent callers from assuming they can get an exact mode without zeroing/restoring umask.
This issue also appears on line 1307 of the same file.
/// The mode is used only when creating a new file; an existing file keeps
/// its current permissions. Callers that will apply final permissions
/// later should use a restrictive initial mode so failures cannot leave a
/// partially-created file overly permissive.
There was a problem hiding this comment.
🟡 Changes recommended
The new mode assertions in test_install.rs compare full st_mode (including type/special bits), which can make tests brittle across environments even when permission bits are correct.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
tests/by-util/test_install.rs:160
- Same as above: permissions().mode() includes type/special bits, so asserting the full value can be brittle (e.g. setgid inheritance). Mask to the permission bits so the test checks only the intended contract.
assert_eq!(0o40_755_u32, at.metadata(ancestor1).permissions().mode());
assert_eq!(0o40_755_u32, at.metadata(ancestor2).permissions().mode());
// Expected mode only on the target_file.
assert_eq!(0o100_200_u32, at.metadata(target_file).permissions().mode());
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
5743285 to
07d815b
Compare
abendrothj
changed the title
There was a problem hiding this comment.
🔵 Needs a closer look
It changes process-wide umask handling across multiple utilities and introduces new global serialization behavior, which warrants final human validation despite strong test coverage.
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 1
- Review effort level: Lite
07d815b to
3fb489c
Compare
3fb489c to
c2688a6
Compare
c2688a6 to
7d85692
Compare
7d85692 to
b1d311b
Compare
b1d311b to
be13503
Compare
|
@sylvestre since you liked #14768: it's stacked on this one, so this needs to land first. Would you have time for a look? |
| use std::os::unix::fs::DirBuilderExt; | ||
| let mut builder = fs::DirBuilder::new(); | ||
| builder.recursive(true).mode(DEFAULT_MODE); | ||
| if let Err(e) = uucore::mode::with_umask(0, || { |
There was a problem hiding this comment.
we now have with_umask(0, ...) in 5 places.
install never reads the umask, so could we just wrap the whole uumain body once instead?
There was a problem hiding this comment.
Done in 66b40f6: one with_umask(0, ...) around the Directory/Standard dispatch. Mode parsing happens before it with an explicit mask of 0, and strip is a plain status() inside the wrapper, so it inherits the cleared umask.
| continue; | ||
| } | ||
| } | ||
| #[cfg(not(unix))] |
There was a problem hiding this comment.
the unix and non-unix branches are almost the same code, could be dedup, no?
There was a problem hiding this comment.
The duplicated branches this PR added are gone. The older unix/non-unix splits already in install are untouched.
|
|
||
| #[cfg(unix)] | ||
| #[test] | ||
| fn test_uumain_restores_umask() { |
There was a problem hiding this comment.
this is a lot of machinery (re-exec the test binary, unsafe umask) to check a Drop impl.
do we really need it?
There was a problem hiding this comment.
No, dropped test_uumain_restores_umask. The restore is tested once, in-process, on the helper itself, in #15121.
| /// The previous umask is restored when the operation returns or unwinds. | ||
| /// Calls through this module are serialized because the umask is process-wide. | ||
| #[cfg(unix)] | ||
| pub fn with_umask<T>(mask: u32, operation: impl FnOnce() -> T) -> T { |
There was a problem hiding this comment.
the lock isn't reentrant, so calling get_umask() or with_umask() inside the closure deadlocks.
please document that
| /// * `mode` - The mode to use when creating new directories (e.g., 0o755). The actual | ||
| /// mode will be modified by the process umask. | ||
| /// * `mode` - The mode to use when creating new directories (e.g., 0o755). The | ||
| /// kernel applies the umask to the mode on each mkdir, so callers that |
There was a problem hiding this comment.
this doesn't need to mention install, please make it shorter
There was a problem hiding this comment.
Dropped; this PR doesn't touch safe_traversal anymore.
|
|
||
| #[cfg(unix)] | ||
| #[test] | ||
| fn test_with_umask_serializes_overlapping_calls() { |
There was a problem hiding this comment.
60 lines, threads and channels to test a mutex. could we drop it or make it much simpler?
There was a problem hiding this comment.
Dropped it. What's left is the short set/restore test in #15121.
be13503 to
66b40f6
Compare
Utilities that create files or directories at an exact mode, whatever the caller's umask, change the process umask around the call. Done by hand, overlapping calls can restore each other's mask and leave the process at the wrong value, and an unwind between the set and the restore leaks the temporary mask. with_umask runs an operation with a temporary umask, holds a mutex shared with get_umask, and restores the previous mask on return or unwind. Calls nest: the thread that holds the mutex skips it, and get_umask there returns the mask in effect. Only Unix sets the umask this way, so Windows' get_umask takes no lock.
Replace mkdir's own umask guard with the shared helper, so its change is serialized with get_umask and other callers. The shaped umask is computed as a plain mask, which drops mkdir's rustix dependency.
The test set and restored the umask with its own guard, outside the lock that get_umask and with_umask share.
install -d and -D passed 0755 to mkdir(2), so the caller's umask still applied to the ancestors they created. Run the Directory/Standard dispatch under a umask of 0, as GNU does, so new ancestors get exactly 0755 and the strip program sees a umask of 0.
66b40f6 to
592f925
Compare
install -dandinstall -Dpassed 0755 tomkdir(2), so the caller's umask still applied: ancestors came out 0750 under umask 027 and 0700 under 077. GNU 9.12 creates them at 0755 whatever the umask is, and--modeonly applies to the final target.install never reads the umask, so the Directory/Standard dispatch now runs once inside
with_umask(0, ...). That also covers the staging file, so-sworks under a umask that would leave it unreadable to strip, and the strip child sees a umask of 0, like GNU's. Mode parsing happens before, with an explicit mask of 0.This is also why
test_install_ancestors_mode_directories_with_filefailed for anyone whose umask wasn't 022 (#11363): it compared against a probe directory made withmkdir(2). It now asserts 0755 directly. The ancestor tests run under umask 077 and the strip tests under 0777. The executable test fixtures are written by a child process, so the test binary doesn't hold a writable fd to them.Based on #15121; until it merges, the diff here shows its commits too. #15121 also moves mkdir onto the helper, and #14768 does mkfifo and mknod.
Fixes #12714. Fixes #11363.