mkdir: create directories atomically with correct permissions#10036
Conversation
Fix uutils#10022: mkdir -m MODE was creating directories with umask-based permissions first, then calling chmod afterward. This left a brief window where the directory existed with wrong permissions. Now we match GNU mkdir behavior by temporarily setting umask to 0 before the mkdir syscall, passing the exact requested mode to the kernel, then restoring the original umask. The directory is created atomically with the correct permissions. Before (two syscalls, race condition): mkdir("dir", 0777) -> created with 0755 (umask applied) chmod("dir", 0700) -> fixed afterward After (single syscall, atomic): umask(0) mkdir("dir", 0700) -> created with exact mode umask(original) Also added tests to verify -m MODE bypasses umask correctly.
|
Is coreutils supposed to be used as a multithreaded lib? Umask changes the whole process, all threads. If safety is needed adding Drop + mutex can make it more robust |
|
GNU testsuite comparison: |
Yes and no. We focus on the cli first but we have some users like nushell. |
|
I can add mutex and Drop but I wonder how much it can help. If a downstream user users uutils/coreutils as a library then their own umask or fork calls can't be protected by any of our mutexes. |
|
One approach is to fork a subprocess that does umask, mkdir and exit. But I wonder if this is an overkill and a mutex + drop guard is enough? |
|
@sylvestre can you provide some directions on whether I should go with:
|
|
for now, just focus on the binary itself, we will see for the lib later |
Add RAII guard to ensure umask is restored even on panic. Encapsulate unsafe umask calls in UmaskGuard::set() for a safe interface.
|
updated with the drop guard |
- Add RAII to spell-checker ignore list - Narrow chmod cfg to Linux only (only used for ACL bits)
|
GNU testsuite comparison: |
|
some jobs are failing |
FromIo is only used in chmod, which is now Linux-only.
|
GNU testsuite comparison: |
|
Looks like the benchmark failure is from flaky test on |
|
well done |
|
seems like this also fixed a bug in gid bit not getting inherited, i bisected with the following test: #!/usr/bin/env nu
def main [] {
cargo b -p uu_mkdir
alias mkdir = ./target/debug/mkdir
if ("test-dir" | path exists) {
rm -rv test-dir
}
mkdir test-dir
chgrp agent test-dir
chmod g+rwxs test-dir
mkdir -p test-dir/foo/bar/baz
let perms = ls -lD test-dir/foo/bar/baz | get mode | str substring 5..5
print $"perms: ($perms)"
print ($perms == "s")
}fails for d72d6f7 although, seems like an unrelated side effect from this pr. |
4 participants
Fix #10022: mkdir -m MODE was creating directories with umask-based permissions first, then calling chmod afterward. This left a brief window where the directory existed with wrong permissions.
Now we match GNU mkdir behavior by temporarily setting umask to 0 before the mkdir syscall, passing the exact requested mode to the kernel, then restoring the original umask. The directory is created atomically with the correct permissions.
Before (two syscalls, race condition):
mkdir("dir", 0777) -> created with 0755 (umask applied)
chmod("dir", 0700) -> fixed afterward
After (single syscall, atomic):
umask(0)
mkdir("dir", 0700) -> created with exact mode
umask(original)
Also added tests to verify -m MODE bypasses umask correctly.