Skip to content
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 0777 to 0027 #17270

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tiran
Copy link
Contributor

@tiran tiran commented Jun 19, 2022

New files are accessible by user.

Fixes: #17269

New files are accessible by user.

Fixes: emscripten-core#17269
@@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doing a little googling it seems that most linux distros default to 022. I wonder if we should do that same?

Also, I'm surprised to see now tests failing.. do our filesystems not honor this, or are there just no tests for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Several tests in CPython test suite are failing on a new buildbot. The same tests are not failing in containerized test environment. The buildbot uses an user account with fewer privileges while the container runs the tests as root with DAC override capability.

The failing code applies the umask directly to permission bits, for example https://github.com/python/cpython/blob/476d30250811e185615dfb971c6a810cac2093bd/Lib/dbm/dumb.py#L306-L317

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't doubt the fix, I'm just surprised we don't have any tests that depend on this in emscripten itself (we should probably add one).

@pradyunsg
Copy link

Anything I can do, to help move this forward?

@sbc100
Copy link
Collaborator

sbc100 commented Oct 24, 2022

Anything I can do, to help move this forward?

IIUC this seems like a good change but we are waiting a test case.

@fluiddot
Copy link
Contributor

👋 I see there's no update since 2022. In order to continue with the effort, I created a new one: #22589. Let me know if that works for you, thanks 🙇 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default umask 0o777 is too strict
4 participants