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

WIP: Correctly detect if executable with os.stat() #7813

Closed
wants to merge 3 commits into from

Conversation

bsolomon1124
Copy link

@bsolomon1124 bsolomon1124 commented Mar 3, 2020

Closes #6364.

Use os.stat() rather than os.access() to correctly determine if srcfile has any executable bits set. os.access() will always return False in cases where the directory is mounted noexec, which can lead to a false negative and the resulting script not being executable.

WIP: working on adding tests as described at #6364 (comment).

This fix does seem to work if modifying the run_test script from the link above, e.g. pip install --upgrade /pip; using this development version of pip including this commit, installing the local version results in the script having correct executable permissions.

Closes Issue pypa#6364: use os.stat() rather than os.access()
to correctly determine if srcfile has any executable bits
set.  os.access() will always return False in cases where
the directory is mounted noexec, which can lead to a false
negative and the resulting script not being executable.
Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

👍

One minor comment, and as you mentioned it would be nice to have a test for this.

src/pip/_internal/operations/install/wheel.py Outdated Show resolved Hide resolved
@bsolomon1124
Copy link
Author

bsolomon1124 commented Mar 26, 2020

I'm taking a look at this again now, but to be perfectly honest am not initially sure how to go about incorporating https://github.com/lovelysystems/lovely-pytest-docker (or similar tool) here. Doing some reading but comments appreciated.

Specifically, I'm not seeing how to properly communicate with the container. The examples in https://github.com/lovelysystems/lovely-pytest-docker and https://github.com/avast/pytest-docker all use Docker web apps that listen on a port and accept requests to check a certain status. In this case, we just want to check the flags of the resulting .venv/bin/testbin file.

I know that the check on the file itself could be done with something like script_path = shutil.which('testbin'); assert os.access(script_path, os.X_OK), but I'm not seeing how that communication can happen in the first place.

Currently I have a fixture and test callable that takes the fixture as its arg in test_wheel.py.

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 11, 2020
@chrahunt
Copy link
Member

I think that our new direct-from-zip wheel installation logic should cover this case, since we derive the file mode from the zip itself. We still don't have a concrete test for it, though.

@pradyunsg
Copy link
Member

Closing since this is fairly out of date and bitrotten. Please feel free to file a new PR for this! ^>^

@pradyunsg pradyunsg closed this Apr 2, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs rebase or merge PR has conflicts with current master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

noexec /tmp causes pip install to miss chmod +x on scripts included in wheel file
5 participants