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

Fix permissions on dist-info files created by adjacent_temp_file #8144

Merged
merged 8 commits into from
Apr 27, 2020
Merged

Fix permissions on dist-info files created by adjacent_temp_file #8144

merged 8 commits into from
Apr 27, 2020

Conversation

uranusjr
Copy link
Member

Fix #8139. This does not contain the fix (yet) because I want to see the tests fail first; I can’t do that on Windows :p

@uranusjr uranusjr force-pushed the wheel-install-permissions branch 6 times, most recently from b4b0d9d to 15fa666 Compare April 26, 2020 10:12
@uranusjr
Copy link
Member Author

Alright, fix coming.

@uranusjr uranusjr marked this pull request as ready for review April 26, 2020 10:26
pradyunsg
pradyunsg approved these changes Apr 27, 2020
View reviewed changes
news/8139.bugfix Outdated Show resolved Hide resolved
@pradyunsg pradyunsg added this to the 20.1 milestone Apr 27, 2020
@uranusjr @pradyunsg
Wording on news fragment
a24fbd9 
Co-Authored-By: Pradyun Gedam <[email protected]>
sbidoul
sbidoul requested changes Apr 27, 2020
View reviewed changes
src/pip/_internal/operations/install/wheel.py Outdated
# type: (str, **Any) -> Iterator[NamedTemporaryFileResult]
with adjacent_tmp_file(path, **kwargs) as f:
yield f
os.chmod(f.name, 0o644)
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be 0o666 - current_umask().

Copy link
Member

Choose a reason for hiding this comment

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

TBH this is becoming complex for little benefits. How about

with open(path + ".pip", **kwargs) as f:
    yield f
replace(f.name, path)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I was wondering whether this needs to be more generic, but didn’t know how.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sbidoul Oops, I missed your last comment (stale GitHub tab). Why were we using adjacent_tmp_file() in the first place?

Copy link
Member

Choose a reason for hiding this comment

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

@uranusjr it came from a suggestion of @chrahunt.
It made sense at first glance but it has generated so much trouble...

Copy link
Member Author

Choose a reason for hiding this comment

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

I traced the NamedTemporaryFile() implementation and I think there is a point to use it instead of straight up open, but I’m not sure for how much benefit. I’d vote for using the tempfile-chmod method for now (we need to fix this in the 20.1 release), and let @chrahunt chime in before trying to remove the temp file call. At least it would be simple since the calls are now wrapped in a function.

@hroncok
Copy link
Contributor

hroncok commented Apr 27, 2020

Testing this in Fedora.

@hroncok
Copy link
Contributor

hroncok commented Apr 27, 2020

Before:

drwxr-xr-x 2 root root     0 apr 27 16:24 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info
-rw------- 1 root root     4 apr 27 16:24 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/INSTALLER
-rw-r--r-- 1 root root  1090 apr 27 16:24 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/LICENSE.txt
-rw-r--r-- 1 root root  3634 apr 27 16:24 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/METADATA
-rw------- 1 root root 57396 apr 27 16:24 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/RECORD
-rw-r--r-- 1 root root   110 apr 27 16:24 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/WHEEL
-rw------- 1 root root   107 apr 27 16:24 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/direct_url.json
-rw-r--r-- 1 root root   125 apr 27 16:24 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/entry_points.txt
-rw-r--r-- 1 root root     4 apr 27 16:24 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/top_level.txt

After:

drwxr-xr-x 2 root root     0 apr 27 17:59 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info
-rw-r--r-- 1 root root     4 apr 27 17:59 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/INSTALLER
-rw-r--r-- 1 root root  1090 apr 27 17:59 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/LICENSE.txt
-rw-r--r-- 1 root root  3634 apr 27 17:59 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/METADATA
-rw-r--r-- 1 root root 57396 apr 27 17:59 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/RECORD
-rw-r--r-- 1 root root   110 apr 27 17:59 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/WHEEL
-rw-r--r-- 1 root root   107 apr 27 17:59 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/direct_url.json
-rw-r--r-- 1 root root   125 apr 27 17:59 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/entry_points.txt
-rw-r--r-- 1 root root     4 apr 27 17:59 /usr/lib/python3.8/site-packages/pip-20.1b1.dist-info/top_level.txt

Hence, fixes the problem.

@pradyunsg
Copy link
Member

Hurray! Thanks for the testing @hroncok! Much appreciated! ^>^

@pradyunsg
Copy link
Member

Gonna go ahead and merge this, since it has approvals from multiple folks and has a positive report that this fixes the issue from someone who's reported the issue.

@pradyunsg pradyunsg merged commit c4c2870 into pypa:master Apr 27, 2020
@sbidoul

This comment has been minimized.

Sign in to view
@mmarchetti
Copy link

Running with umask = 027 causes the permission calculation here to go awry:

https://github.com/pypa/pip/pull/8144/files#diff-81eaeaa2196a8c5382958f2d9f22b593R570

    generated_file_mode = 0o666 - current_umask()

>>> oct(0o666 - 0o027)
'0637'

I'd have expected a bitwise AND so the result would be 0640.

@uranusjr uranusjr deleted the wheel-install-permissions branch May 21, 2020 05:12
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pradyunsg pradyunsg pradyunsg approved these changes

@sbidoul sbidoul sbidoul approved these changes

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

Successfully merging this pull request may close these issues.

PEP 610 support (git packages) can break pip freeze
5 participants
@uranusjr @hroncok @pradyunsg @sbidoul @mmarchetti