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

Incorrect file permissions in dist-info directory after installing a package #8164

Closed
mmarchetti opened this issue Apr 28, 2020 · 9 comments · Fixed by #8166
Closed

Incorrect file permissions in dist-info directory after installing a package #8164

mmarchetti opened this issue Apr 28, 2020 · 9 comments · Fixed by #8166
Labels
type: bug

Comments

@mmarchetti
Copy link

mmarchetti commented Apr 28, 2020

Environment

  • pip version: 20.1
  • Python version: 3.8.1
  • OS: ubuntu 18.04 (xenial)

Description
After a pip install with umask set to 027, files in the environment have incorrect permissions.

I expected the files to have permissions 640, but they ended up as 637. This is both too restrictive at the group level and too permissive at the world level.

Expected behavior
Correct file permissions.

I think the cause is this fix:

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.

How to Reproduce
virtualenv ./env
source env/bin/activate
umask 027
pip install six
ls -lR env/lib/python3.8/site-packages/six-1.14.0.dist-info/

Output

root@674aabd90334:~# virtualenv ./env
Using base prefix '/opt/Python/3.8.1'
New python executable in /root/env/bin/python3.8
copying /opt/Python/3.8.1/bin/python3.8 => /root/env/bin/python3.8
Also creating executable in /root/env/bin/python
Installing setuptools, pip, wheel...
done.
root@674aabd90334:~# source env/bin/activate
(env) root@674aabd90334:~# umask 027
(env) root@674aabd90334:~# pip install six
Collecting six
  Using cached six-1.14.0-py2.py3-none-any.whl (10 kB)
Installing collected packages: six
Successfully installed six-1.14.0
(env) root@674aabd90334:~# ls -lR env/lib/python3.8/site-packages/six-1.14.0.dist-info/
env/lib/python3.8/site-packages/six-1.14.0.dist-info/:
total 24
-rw--wxrwx 1 root root    4 Apr 28 21:13 INSTALLER
-rw-r----- 1 root root 1066 Apr 28 21:13 LICENSE
-rw-r----- 1 root root 1795 Apr 28 21:13 METADATA
-rw--wxrwx 1 root root  560 Apr 28 21:13 RECORD
-rw-r----- 1 root root  110 Apr 28 21:13 WHEEL
-rw-r----- 1 root root    4 Apr 28 21:13 top_level.txt
@triage-new-issues triage-new-issues bot added the S: needs triage label Apr 28, 2020
@sbidoul
Copy link
Member

sbidoul commented Apr 28, 2020

Indeed 0o666 & ~current_umask() should be better.

@sbidoul sbidoul added the type: bug label Apr 28, 2020
@triage-new-issues triage-new-issues bot removed the S: needs triage label Apr 28, 2020
@pradyunsg pradyunsg added this to the 20.1 milestone Apr 28, 2020
@deveshks
Copy link
Contributor

deveshks commented Apr 29, 2020

We also use - instead of &~ at

os.chmod(fn, (0o777 - current_umask() | 0o111))

and

os.chmod(path, (0o777 - current_umask() | 0o111))

Do we need to fix those as well?

@sbidoul
Copy link
Member

sbidoul commented Apr 29, 2020

@deveshks these were the source of bad inspiration that led to this bug indeed.
These work because we subtract to 0o777, but I'd change them yes. In a separate PR though, because that change would be for 20.2, not for a 20.1 bugfix release.

@pradyunsg
Copy link
Member

pradyunsg commented Apr 29, 2020

>>> oct(0o777 - 0o027 | 0o111)
'0o751'
>>> oct(0o666 & ~0o027)
'0o640'

IIUC, the 0o777 - ... | 0o111 form would be the better form, to ensure everyone can read. It'd likely make sense to move this chmod call into a helper, and use it everywhere (to avoid such issues in the future).

@pradyunsg
Copy link
Member

pradyunsg commented Apr 29, 2020

Oh wait, I don't understand correctly. The order of the bits is rwx, not xwr. Those other locations are for setting permissions on executable scripts, and don't actually do the right things as OP (and @sbidoul) have pointed out. Hah.

@deveshks
Copy link
Contributor

deveshks commented Apr 29, 2020

It'd likely make sense to move this chmod call into a helper, and use it everywhere (to avoid such issues in the future).

So do we only move these two os.chmod calls in pip/src/pip/_internal/utils/unpacking.py into a helper function in another PR right, along with changing it to os.chmod(path, (0o777 & ~current_umask() | 0o111)) as per @sbidoul right?

@pradyunsg
Copy link
Member

pradyunsg commented Apr 29, 2020

Nah, not now -- let's get the specific bugfix merged and then think about the broader fixes. :)

@deveshks
Copy link
Contributor

deveshks commented Apr 29, 2020

Nah, not now -- let's get the specific bugfix merged and then think about the broader fixes. :)

Agreed, I will create another issue about fixing them and we can discuss there. BTW I have addressed the review comments, and I think that PR is ready to be approved/merged 😊

@deveshks
Copy link
Contributor

deveshks commented May 1, 2020

Filed #8179 for fixing file permission in utils/unpacking.py

@pradyunsg pradyunsg removed this from the 20.1.1 milestone May 5, 2020
bors bot added a commit to duckinator/emanate that referenced this issue May 19, 2020
126: Update pip to 20.1.1 r=duckinator a=pyup-bot


This PR updates [pip](https://pypi.org/project/pip) from **20.1** to **20.1.1**.



<details>
  <summary>Changelog</summary>
  
  
   ### 20.1.1
   ```
   ===================

Deprecations and Removals
-------------------------

- Revert building of local directories in place, restoring the pre-20.1
  behaviour of copying to a temporary directory. (`7555 &lt;https://github.com/pypa/pip/issues/7555&gt;`_)
- Drop parallelization from ``pip list --outdated``. (`8167 &lt;https://github.com/pypa/pip/issues/8167&gt;`_)

Bug Fixes
---------

- Fix metadata permission issues when umask has the executable bit set. (`8164 &lt;https://github.com/pypa/pip/issues/8164&gt;`_)
- Avoid unnecessary message about the wheel package not being installed
  when a wheel would not have been built. Additionally, clarify the message. (`8178 &lt;https://github.com/pypa/pip/issues/8178&gt;`_)
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pip
  - Changelog: https://pyup.io/changelogs/pip/
  - Homepage: https://pip.pypa.io/
</details>



Co-authored-by: pyup-bot <github-bot@pyup.io>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants