Skip to content

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
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 🙇 !

sbc100 pushed a commit that referenced this pull request Sep 23, 2024
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 cover `umask` functionality.
@sbc100
Copy link
Collaborator

sbc100 commented Jan 3, 2025

This was fixed in #22589

@sbc100 sbc100 closed this Jan 3, 2025
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