-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
mkdir: create directories atomically with correct permissions #10036
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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: |
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.