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.

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

Respect umask #65

Merged
merged 4 commits into from
Jul 13, 2023
Merged

Respect umask #65

merged 4 commits into from
Jul 13, 2023

Conversation

dholth
Copy link
Contributor

@dholth dholth commented Jun 23, 2023 •
edited by kenodegard

Description

This extends tarfile to apply umask (before ever creating the file with tar's default chown).

This should work if the program does not change its own umask. It is probably necessary to check umask with a global lock such as the one used by module level globals in the import system for thread safety.

Resolves conda/conda#12829

dholth
dholth commented Jun 23, 2023
View reviewed changes
Copy link
Contributor Author

@dholth dholth left a comment

Choose a reason for hiding this comment

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

I would add an option to set the umask in the TarFile subclass' constructor to be completely general, for mad programs that want to change the umask while calling us multiple times. I would add a unit test.

@kenodegard kenodegard mentioned this pull request Jun 30, 2023
Closed
3 tasks
@jezdez
Copy link
Member

jezdez commented Jun 30, 2023

@awwad Do you have opinions on the way this was solved?

varlackc
varlackc approved these changes Jul 13, 2023
View reviewed changes
Copy link

@varlackc varlackc left a comment

Choose a reason for hiding this comment

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

LGTM

kenodegard
kenodegard reviewed Jul 13, 2023
View reviewed changes
Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

minor edit

conda_package_streaming/package_streaming.py Outdated Show resolved Hide resolved
conda_package_streaming/package_streaming.py Outdated Show resolved Hide resolved
@dholth @kenodegard
Apply suggestions from code review
2aa2abb 
Co-authored-by: Ken Odegard <kodegard@anaconda.com>
kenodegard
kenodegard reviewed Jul 13, 2023
View reviewed changes
tests/test_extract.py Outdated Show resolved Hide resolved
@dholth @kenodegard
Update tests/test_extract.py
3aa8a66 
Co-authored-by: Ken Odegard <kodegard@anaconda.com>
@dholth dholth merged commit add2774 into main Jul 13, 2023
6 checks passed
@dholth dholth deleted the respect-umask branch July 13, 2023 20:10
@dholth dholth mentioned this pull request Jul 28, 2023
Open
2 tasks
@dholth
Copy link
Contributor Author

dholth commented Jul 28, 2023

Does this affect the umask on implictly-created directories? (When we see an a/b/c/d.pywithout seeing directory entries for a/b/c in the tarfile?)

@j3mdamas j3mdamas mentioned this pull request Oct 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jezdez jezdez jezdez approved these changes

@kenodegard kenodegard kenodegard approved these changes

@varlackc varlackc varlackc approved these changes

@chenghlee chenghlee Awaiting requested review from chenghlee

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

Successfully merging this pull request may close these issues.

conda should respect umask when installing package files (world-writable files)
4 participants
@dholth @jezdez @kenodegard @varlackc