mkfifo, mknod: set the umask through mode::with_umask by abendrothj · Pull Request #14768 · uutils/coreutils

abendrothj · 2026-09-21T02:22:04Z

Based on #15121; until it merges, the diff here shows its commits too.

mkfifo and mknod change the process-wide umask directly around a single syscall. Overlapping calls can restore each other's mask, and a panic between set and restore leaks the temporary mask into the rest of the run.

They now use uucore::mode::with_umask, which holds the umask mutex and restores the previous mask on unwind; mkdir moves to it in #15121.

No behavior change: mkfifo and mknod modes match GNU 9.12 at umasks 022, 077 and 002. rustix is narrowed to the fs feature in uu_mkfifo.

Closes #14994

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

sylvestre · 2026-09-21T18:11:35Z

-/// ensuring the directory is created atomically with the correct permissions.
-/// This avoids a race condition where the directory briefly exists with
-/// umask-based permissions.
+/// Create a directory, shaping the umask only when it would block a mode bit


7 lines of doc + 5 more inside, nobody will read that :) please make it shorter

Cut to four lines and folded the inner comments into it.

sylvestre · 2026-09-21T18:12:07Z

    };

-    match create_dir_with_mode(path, mkdir_mode, shaped_umask) {
+    match create_dir_with_mode(path, mkdir_mode, is_parent, config.mode) {


mode, is_parent and config.mode all encode the same thing here - could it just take config?

Right — it takes the config now: create_dir_with_mode(path, is_parent, config), and the mode comes from a single mkdir_mode(is_parent, config) that the SELinux labelling also uses, so there's one place deciding it.

sylvestre · 2026-09-21T18:12:20Z

+/// Run an operation with a temporary umask derived from the current value.
+///
+/// Reading, deriving, setting, and restoring the umask are serialized as one
+/// operation. The selector and operation must not call this module's umask


a closure calling get_umask() here deadlocks silently - can we debug_assert instead of only documenting it?

Added an assertion: the lock sets a thread-local flag while held and debug_assert!s on reentry, so a nested call panics with a message instead of hanging. test_reentrant_umask_helper_is_rejected covers it, gated on debug_assertions so a release test run can't deadlock on it.

sylvestre · 2026-09-21T18:12:27Z

+/// operation. The selector and operation must not call this module's umask
+/// helpers recursively.
+#[cfg(unix)]
+pub fn with_umask_from_current<T>(


with_umask has a test, with_umask_from_current none, please add one

Added test_with_umask_from_current_derives_mask_and_restores_it: the selector gets the real current umask (0027), the operation runs under the derived one (0007), and 0027 is back afterwards. It shares the child-process helper with the existing test, which now also checks the child really ran a test instead of trusting the exit status.

Small update since my earlier reply: the test is in-process now, not a child process. It checks the mask passed to the closure, the mask active inside it, and that the original comes back afterwards.

sylvestre · 2026-09-21T18:12:51Z

cool PR :)

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

abendrothj · 2026-09-22T02:45:23Z

Ordering note: this branch sits on top of #12715, so the install hunks in the diff belong to that PR and will vanish once it lands. Say the word and I'll rebase it onto main afterwards if you'd rather review it standalone.

The clippy and doc failures were mine: DEFAULT_PERM became dead code on Windows once the non-unix path stopped using it, and the new doc comment linked to the private UmaskLock type. Both fixed; those jobs are green.

sylvestre · 2026-09-24T21:59:29Z

+}
+
+#[cfg(unix)]
+#[allow(clippy::unnecessary_cast)] // no-op where RawMode is already u32


could u32::from(mode.bits()) work here? it works for both u16 and u32, so we could drop the #[allow]

It compiles both ways, but it doesn't drop the #[allow]: where RawMode is already u32, u32::from(mode.bits()) trips clippy::useless_conversion instead of clippy::unnecessary_cast (checked with --target x86_64-unknown-linux-gnu). #[expect] doesn't work either, on the u16 targets the lint doesn't fire and the expectation goes unfulfilled.

mode.bits() as _ is clean on both, same truncation behaviour, and is how mode_from_umask above avoids an attribute. If you'd rather have no #[allow] here, I can switch to that.

Yes we prefer no allow

Dropped in 5bcac42; mode.bits() as _ is clean on both widths, so no #[allow] is needed.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

codspeed · 2026-09-28T03:49:02Z

Merging this PR will degrade performance by 17.48%

❌ 2 regressed benchmarks
✅ 397 untouched benchmarks
⏩ 54 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Simulation	`three_39_bit_primes`	371.3 ms	467.4 ms	-20.56%
❌	Simulation	`five_38_bit_primes`	1.7 s	2 s	-14.29%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing abendrothj:fix/uucore-serialize-umask-changes (2b0978d) with main (8cf2e4f)}

54 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

github-actions · 2026-09-28T07:48:47Z

GNU testsuite comparison:

Skip an intermittent issue tests/date/resolution (fails in this run but passes in the 'main' branch)

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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.

Both changed the process-wide umask directly around a single syscall. Overlapping calls can restore each other's mask and leave the process at the wrong value, and a panic between the set and the restore leaks the temporary mask into the rest of the run. Use uucore::mode::with_umask, which serializes the change with get_umask and restores the mask on unwind. No behavior change: modes for mkfifo -m and plain mkfifo match GNU 9.12 at umasks 022, 077 and 002. Narrow rustix to the fs feature in uu_mkfifo, which no longer needs process.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

abendrothj · 2026-10-07T00:39:32Z

reworked after #15121: mkdir moved there, as you asked on that PR, and the lock is reentrant now, so this is just mkfifo and mknod, on #15121 directly instead of #12715. with_umask_from_current and the debug_assert are gone: mkdir is single-threaded and with_umask restores the mask before releasing the lock, so its read and set can't go stale. checked outside the suite that mkfifo and mknod modes still match GNU 9.12 at umasks 022, 077 and 002.

Copilot AI lite review requested due to automatic review settings September 21, 2026 02:22

Copilot AI reviewed Sep 21, 2026

abendrothj mentioned this pull request Sep 21, 2026

install: create ancestor directories at 0755 regardless of umask #12715

Open

Copilot AI review requested due to automatic review settings September 21, 2026 02:27

abendrothj force-pushed the fix/uucore-serialize-umask-changes branch from a85690e to 56b1e1d Compare September 21, 2026 02:27

Copilot AI reviewed Sep 21, 2026

abendrothj changed the title ~~uucore: serialize process-wide umask changes~~ mkdir, mkfifo, mknod: use uucore's serialized umask helpers Sep 21, 2026

sylvestre reviewed Sep 21, 2026

View reviewed changes

abendrothj force-pushed the fix/uucore-serialize-umask-changes branch from 56b1e1d to 2f632ab Compare September 22, 2026 01:00

Copilot AI review requested due to automatic review settings September 22, 2026 01:00

Copilot AI reviewed Sep 22, 2026

Copilot AI review requested due to automatic review settings September 22, 2026 01:42

abendrothj force-pushed the fix/uucore-serialize-umask-changes branch from 2f632ab to 42b8bc8 Compare September 22, 2026 01:42

Copilot AI reviewed Sep 22, 2026

sylvestre reviewed Sep 24, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 28, 2026 02:35

Copilot AI reviewed Sep 28, 2026

Copilot AI review requested due to automatic review settings September 28, 2026 07:00

Copilot AI reviewed Sep 28, 2026

Copilot AI lite review requested due to automatic review settings October 6, 2026 07:15

abendrothj force-pushed the fix/uucore-serialize-umask-changes branch from 5a106d1 to 3896a36 Compare October 6, 2026 07:15

Copilot AI reviewed Oct 6, 2026

abendrothj mentioned this pull request Oct 6, 2026

uucore: add mode::with_umask #15121

Open

abendrothj added 4 commits October 6, 2026 16:55

mkdir: set the umask through mode::with_umask

e4768c3

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.

sort: pin the tmp_dir test's umask with mode::with_umask

28f64a5

The test set and restored the umask with its own guard, outside the lock that get_umask and with_umask share.

Copilot AI lite review requested due to automatic review settings October 7, 2026 00:39

abendrothj force-pushed the fix/uucore-serialize-umask-changes branch from 3896a36 to 2b0978d Compare October 7, 2026 00:39

Copilot AI reviewed Oct 7, 2026

abendrothj changed the title ~~mkdir, mkfifo, mknod: use uucore's serialized umask helpers~~ mkfifo, mknod: set the umask through mode::with_umask Oct 7, 2026

Conversation

abendrothj commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylvestre commented Sep 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

abendrothj commented Sep 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

codspeed Bot commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will degrade performance by 17.48%

Performance Changes

Footnotes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

abendrothj commented Oct 7, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

abendrothj commented Sep 21, 2026 •

edited

Loading

codspeed Bot commented Sep 28, 2026 •

edited

Loading