8000 mkdir: fixed not respecting set umask by pyoky · Pull Request #3150 · uutils/coreutils · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

mkdir: fixed not respecting set umask #3150

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 16 commits into from
Apr 13, 2022
Merged

mkdir: fixed not respecting set umask #3150

merged 16 commits into from
Apr 13, 2022

Conversation

pyoky
Copy link
Contributor
@pyoky pyoky commented Feb 17, 2022

Resolves #3119.

Removed the permissions mode argument's hardcoded default_value() of 755, instead using uutils::mode::get_umask() when argument isn't specified.

Before:

$ umask 000 && ./coreutils mkdir new_dir && ls -l | grep new_dir
drwxr-xr-x  2 pyoky pyoky     4096 Feb 17 08:30 new_dir

After:

$ umask 000 && ./coreutils mkdir new_dir && ls -l | grep new_dir
d---------  2 pyoky pyoky     4096 Feb 17 08:29 new_dir

This mirrors the behavior of GNU mkdir.

pyoky added 2 commits February 17, 2022 07:37
@pyoky
mkdir: permissions respects umask
ea175ce 
- hardcoded default permissions changed to ones defined by umask
@pyoky
Merge branch 'main' of github.com:pyoky/coreutils
65a45ae
@sylvestre
Copy link
Contributor

Looks good but could you please add a test?

@pyoky
Copy link
Contributor Author
pyoky commented Feb 18, 2022

@sylvestre I'm having issues writing a test that would modify the umask, create a new directory, and test if the permissions are expected. umask is a shell builtin, meaning that TestScenario can't execute it as a command. This is what I'm trying instead:

#[test]
#[cfg(not(windows))]
fn test_umask_compliance() {
    let scene = TestScenario::new(util_name!());
    let at = scene.fixtures.clone();
    let umask_out = scene.cmd("sh")
         .arg("-c")
         .arg("umask")
         .arg("000")
         .succeeds();
    let mkdir_out = scene.ucmd().arg(TEST_DIR8).succeeds();
    let perms = at.metadata(TEST_DIR8).permissions().mode();
    assert_eq!(perms, 0o40000);
}

but this has the issue that umask is dependent on the shell, which means as sh exits the modified umask disappears along with it.

I can write one that checks if it respects the already set umask but it won't be effective. What do you suggest?

@pyoky
mkdir: fixed permissions behavior with umask
b8c0a87 
- umask application more closely resembles gnu
@sylvestre
Copy link
Contributor

@pyoky maybe try to do the two operations at the same time?

@tertsdiepraam
Copy link
Member

I think that's a good solution. On my machine, this seems to give the correct umask:

❯ sh -c "umask 000 && mkdir a"
❯ mkdir b
❯ ls -l
total 0
drwxrwxrwx 2 terts terts 40 19 feb 01:48 a
drwxr-xr-x 2 terts terts 40 19 feb 01:48 b

@pyoky
test/mkdir: added testing for umask compliance
8ffe367 
- tests for all permission combinations
@pyoky
Copy link
Contributor Author
pyoky commented Feb 19, 2022 •
edited
Loading

Thanks @sylvestre @tertsdiepraam, though in the end I wrote it to call libc::umask(), since it was similar to test_chmod.rs's permissions testing. Grateful for the kind response - this was my first oss contribution.

@sylvestre
Copy link
Contributor

This fails:

error[E0308]: mismatched types
   --> /Users/runner/work/coreutils/coreutils/tests/by-util/test_mkdir.rs:113:45
    |
113 |         let original_umask = unsafe { umask(umask_set) };
    |                                             ^^^^^^^^^ expected `u16`, found `u32`
    |
help: you can convert a `u32` to a `u16` and panic if the converted value doesn't fit
    |
113 |         let original_umask = unsafe { umask(umask_set.try_into().unwrap()) };
    |                                                      ++++++++++++++++++++

could you please have a look? thanks

@pyoky
Copy link
Contributor Author
pyoky commented Feb 24, 2022 •
edited
Loading

@sylvestre I changed the umask_set to mode_t. Test should work now.

@sylvestre
Copy link
Contributor

A few tests are failing, could you please have a look? Thanks

@sylvestre
Copy link
Contributor

ping?

@pyoky
Copy link
Contributor Author
pyoky commented Mar 22, 2022

@sylvestre Yup I'm working on it. I'll push a commit in a day or two.

@tertsdiepraam
Copy link
Member

There is a small linting error. 746 tests are also suddenly failing, due to permission denied errors. Let's see if they're still around once the linting issue is fixed.

@pyoky
Copy link
Contributor Author
pyoky commented Mar 31, 2022

@tertsdiepraam It seems like my use of libc::umask() messes with permissions of test directories with messes up permissions for tests that come after. The test now only test umasks with a reasonable number of permission combinations. (Testing all 0o0~0o777 was unnecessary anyways.)

While testing I also found a slight bug in test_pwd.rs where the directory name in a test command was not escaped properly. It was a minor bug so I included in the commit with descriptions.

Lints should also be fixed. Sorry for my mistakes - I'm still new to contributions.

@pyoky
tests/mkdir: reduced tested permission combinations
9a76f3b 
tests/pwd: escapted directory paths
@tertsdiepraam
Copy link
Member

No worries! Mistakes are basically expected :) Great work on the fix!

tertsdiepraam
tertsdiepraam reviewed Mar 31, 2022
View reviewed changes
Copy link
Member
@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should fix the last clippy error

@sylvestre @tertsdiepraam
Fix clippy
12ad0f0 
Co-authored-by: Terts Diepraam <terts.diepraam@gmail.com>
@pyoky
Copy link
Contributor Author
pyoky commented Apr 5, 2022

@tertsdiepraam Is there an issue still with checks? I'm seeing the tests are canceled. If there's still issues with merging let me know.

@tertsdiepraam
Copy link
Member

I'm rerunning those checks. If they pass, I think this can get merged.

jfinkels
jfinkels approved these changes Apr 9, 2022
View reviewed changes
Copy link
Collaborator
@jfinkels jfinkels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool

tests/by-util/test_mkdir.rs
Comment on lines +178 to +179
for i in 0o0..0o027 {
// tests all permission combinations
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not all the possible permission settings. That's fine, let's just make sure the comment matches the code.

@sylvestre sylvestre merged commit 10be79c into uutils:main Apr 13, 2022
@pyoky pyoky deleted the mkdir-fix branch June 18, 2022 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tertsdiepraam tertsdiepraam tertsdiepraam left review comments

@jfinkels jfinkels jfinkels approved these changes

Assignees
No one assigned
Labels
awaiting response
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mkdir: does not respect umask
4 participants
@pyoky @sylvestre @tertsdiepraam @jfinkels
0