uucore: add mode::with_umask by abendrothj · Pull Request #15121 · uutils/coreutils

abendrothj · 2026-10-06T07:27:52Z

install, mkdir, mkfifo and mknod need to change the process-wide umask around a few syscalls. Done by hand, two overlapping changes can restore each other's mask, and a panic between set and restore leaks the temporary mask.

uucore::mode::with_umask runs a closure with the given umask while holding the same mutex as get_umask, and restores the previous mask before releasing it, also on unwind. Calls nest: on the thread holding the mutex, with_umask and get_umask skip it, and get_umask returns the mask in effect. Windows' get_umask takes no lock, since nothing sets the umask there.

mkdir and the sort tmp_dir test switch to it from their own guards; mkdir no longer needs rustix. Tests check the mask is set inside and restored after, also when nested.

No user-visible change; also used by #12715 and #14768.

Part of #12714 and #14994

Copilot

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

codspeed · 2026-10-06T15:38:59Z

Merging this PR will degrade performance by 33.07%

⚠️

120 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

sort_reverse_utf8_locale

sort_mixed_utf8_locale

sort_unique_utf8_locale

sort_very_long_lines_utf8_locale

cksum_default

dd_copy_partial

cksum_multiple_files

du_all_wide_tree[(5000, 500)]

du_deep_tree[(100, 3)]

cksum_raw_output

…and 110 more

ℹ️ 1 benchmark has no flame graph

The flamegraph could not be generated because too much time was spent in runtime internals, making the visualization not useful for performance analysis.

sort_dictionary_order[500000]

⚠️

Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 3 regressed benchmarks
✅ 398 untouched benchmarks
⏩ 70 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Simulation	`factor_three_39_bit_primes`	300.2 ms	817 ms	-63.25%
❌	Simulation	`factor_five_38_bit_primes`	1.6 s	1.9 s	-13.99%
❌	Simulation	`factor_thirteen_39_bit_primes`	8.9 s	9.4 s	-5.14%

Tip

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

_{Comparing abendrothj:uucore-with-umask (b434a5c) with main (98f90f2)²}

70 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. ↩
No successful run was found on main (9c8b347) during the generation of this report, so 98f90f2 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

sylvestre · 2026-10-06T20:11:21Z


+// `rustix::fs::RawMode` is u16 on some targets and u32 on others.
+#[cfg(unix)]
+fn mode_from_umask(mask: u32) -> rustix::fs::Mode {


used once, could be inlined in UmaskGuard::set, no?

sylvestre · 2026-10-06T20:11:42Z

+}
+
+#[cfg(unix)]
+struct UmaskGuard(rustix::fs::Mode);


mkdir.rs and sort/tmp_dir.rs already have their own UmaskGuard, could you please switch them to this in the same PR?
for now it has no caller

done, in two commits: mkdir and the sort test use it now. mkdir's shaped mask is a plain u32, so it drops rustix. its umask tests (15 of them) check the resulting modes; nothing tests two threads on the umask.

sylvestre · 2026-10-06T20:11:56Z

+///
+/// The previous umask is restored when the operation returns or unwinds.
+/// Calls through this module are serialized because the umask is process-wide.
+/// The lock is not reentrant: calling [`get_umask`] or `with_umask` from


a deadlock on a nested get_umask is easy to hit (cp/mkdir call it everywhere), could we make it reentrant instead?

done: the thread holding the lock skips it, and get_umask there returns the mask in effect. test_with_umask_nests hung before and passes now. other threads still wait, so the closure must not wait on one of them.

sylvestre · 2026-10-06T20:11:58Z

+/// Calls through this module are serialized because the umask is process-wide.
+/// The lock is not reentrant: calling [`get_umask`] or `with_umask` from
+/// `operation` deadlocks.
+#[cfg(unix)]


why only unix? get_umask takes the lock on windows too

nothing sets the umask on windows (mkdir's guard was unix-only too), so I dropped the lock there instead.

Copilot

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

abendrothj · 2026-10-07T21:42:33Z

The expand numbers aren't from this PR: expand doesn't enable uucore's mode feature, and its bench binary is identical before and after (checked on macOS arm64). factor is the usual noise.

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.

Copilot

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

Copilot AI balanced review requested due to automatic review settings October 6, 2026 07:27

abendrothj mentioned this pull request Oct 6, 2026

uucore: share replace_link's atomic replace #15122

Open

Copilot AI reviewed Oct 6, 2026

This was referenced Oct 6, 2026

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

Open

mkfifo, mknod: set the umask through mode::with_umask #14768

Open

sylvestre reviewed Oct 6, 2026

View reviewed changes

abendrothj force-pushed the uucore-with-umask branch from 6c97e71 to 28f64a5 Compare October 7, 2026 00:39

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

Copilot AI reviewed Oct 7, 2026

abendrothj mentioned this pull request Oct 7, 2026

mv: recreate special files when moving them across devices #13334

Open

abendrothj added 3 commits October 13, 2026 02:53

mkdir: set the umask through mode::with_umask

782ad28

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

b434a5c

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

Copilot AI balanced review requested due to automatic review settings October 13, 2026 09:56

abendrothj force-pushed the uucore-with-umask branch from 28f64a5 to b434a5c Compare October 13, 2026 09:56

Copilot AI reviewed Oct 13, 2026

Conversation

abendrothj commented Oct 6, 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!

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

Uh oh!

Merging this PR will degrade performance by 33.07%

Performance Changes

Footnotes

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

abendrothj commented Oct 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abendrothj commented Oct 6, 2026 •

edited

Loading

codspeed Bot commented Oct 6, 2026 •

edited

Loading