-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Change default umask
from 777
to 022
#22589
Conversation
Thanks @sbc100 for taking a look at the PR 🙇 ! It's my first attempt to add unit tests to the repository, so bear with me for any mistakes 😅 . |
Thanks for your contribution! Much appreciated. |
Btw, I picked the value |
test/stat/test_umask.c
Outdated
} | ||
|
||
int main() { | ||
signal(SIGABRT, cleanup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this line, along with the cleanup function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in 9619d38.
test/stat/test_umask.c
Outdated
|
||
int main() { | ||
signal(SIGABRT, cleanup); | ||
test(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I guess you might as well inline the test into main
since that is all it contains now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, good point. I've applied this in 9619d38.
test/test_other.py
Outdated
@also_with_wasmfs | ||
def test_umask(self): | ||
self.set_setting("FORCE_FILESYSTEM") | ||
self.do_runf('stat/test_umask.c', 'success') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this test into test/other
? And then use do_other_test
to run it? You can run it with --rebase
to generate the expected output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, I forgot to relocate the file after b0d7dcf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in 9619d38.
test/stat/test_umask.c
Outdated
// Get the default umask | ||
mode_t default_umask = get_umask(); | ||
printf("default umask: %o\n", default_umask); | ||
assert(default_umask == 027); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think making the default 022 makes sense yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I'll update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this in 2891de1.
umask
from 777
to 027
umask
from 777
to 022
@@ -28,7 +28,7 @@ static int g_pid = 42; | |||
static int g_pgid = 42; | |||
static int g_ppid = 1; | |||
static int g_sid = 42; | |||
static mode_t g_umask = S_IRWXU | S_IRWXG | S_IRWXO; | |||
static mode_t g_umask = S_IWGRP | S_IWOTH; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, here are the definitions of values used here:
emscripten/system/lib/libc/musl/include/sys/stat.h
Lines 60 to 74 in 8eb432e
#define S_ISUID 04000 | |
#define S_ISGID 02000 | |
#define S_ISVTX 01000 | |
#define S_IRUSR 0400 | |
#define S_IWUSR 0200 | |
#define S_IXUSR 0100 | |
#define S_IRWXU 0700 | |
#define S_IRGRP 0040 | |
#define S_IWGRP 0020 | |
#define S_IXGRP 0010 | |
#define S_IRWXG 0070 | |
#define S_IROTH 0004 | |
#define S_IWOTH 0002 | |
#define S_IXOTH 0001 | |
#define S_IRWXO 0007 |
Thanks @sbc100 for reviewing and approving the PR 🙇 ! |
Addresses #17269.
This PR follows the approach made in #17270 but sets default
umask
to a more permissive value (022
). Additionally, it includes a unit test to coverumask
functionality.