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

Add support for handling long shebang lines #4237

Conversation

HaraldNordgren
Copy link

@HaraldNordgren HaraldNordgren commented Jan 16, 2017

We need this for the virtualenv project.

When creating a new virtualenv with a long name on Linux (more than 127 characters) and/or spaces in the name, the virtualenv becomes unusable. The problem is that on Linux there are limitations on the shebangs, meaning that a script starting with the following line does not work:

 #!/very/loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong/path/possibly/containing spaces/bin/python

This pull request alters the executable of the ScriptMaker when need to give a script with an sh shebang which then executes itself with the specified Python. Clever quoting makes sure that the same code is both valid Shell and Python.

This is what flask installed with virtualenv running this code looks like:

#!/usr/bin/env sh
'''exec' '/path/to/env/bin/python' "$0" "$@"
' '''

# -*- coding: utf-8 -*-
import re
import sys

from flask.cli import main

if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw?|\.exe)?$', '', sys.argv[0])
    sys.exit(main())

The implementation is based on https://hg.mozilla.org/mozilla-central/file/tip/mach.


This change is Reviewable

…xecute as sh script and then re-execute with specified Python
@pfmoore
Copy link
Member

pfmoore commented Jan 16, 2017

As noted on the equivalent PR you raised against distlib, I really think this needs stronger justification, in terms of examples of where it's causing issues for people. We get a small but regular trickle of issues raised here, but it's not exactly a flood - and generally the fact that it's an OS limitation is enough explanation. There's never really been a case where it's been reported as a showstopper. Add that to the fact that (if as suggested on the distlib tracker) this needs to be added into pip, it'll be either (1) quite complex and OS-dependent code to detect when the workaround is needed, or (2) a complex and potentially fragile bit of wrapper code that 99% of the time isn't needed, and I think we need a really solid justification for taking this forward.

shebang_max_lengths = {
'linux': 127,
'darwin': 512,
}
Copy link
Member

Choose a reason for hiding this comment

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

I couldn't see the limits you are using here confirmed in the URL you mention, and there are a lot of other different limits mentioned there. It seems to me that there will be plenty of systems that could still have the issue even with this patch. I can't comment on how obscure those systems might be, of course.

new_executable = b'/usr/bin/env sh\n'
new_executable += b"'''exec' '" + executable + b"'" + b' "$0" "$@"\n'
new_executable += b"' '''"
return new_executable
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't an executable name containing ''' break this? I don't know if that could be used to produce an exploit, but it should be considered.

Copy link
Author

@HaraldNordgren HaraldNordgren Jan 16, 2017

Choose a reason for hiding this comment

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

Indeed it does. Not good.

@pfmoore
Copy link
Member

pfmoore commented Jan 16, 2017

There probably need to be tests for this. Maybe create a long directory and (as a separate test) a directory with spaces, and confirm the wrappers work.

@HaraldNordgren
Copy link
Author

HaraldNordgren commented Jan 17, 2017

Some tests would probably be a good idea. But I am having problems with tox. It's breaking in a lot of different places even on master.

Is there a guide to get the test suite up and running with pip?

# re-executed with the specified Python executable. Proper quoting
# makes sure that the same code is valid as both. See
# https://hg.mozilla.org/mozilla-central/file/tip/mach
new_executable = b'/usr/bin/env sh\n'
Copy link
Member

Choose a reason for hiding this comment

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

Note that /usr/bin/env is not the correct location on every system.

@pradyunsg pradyunsg changed the title For shebang lines that are too long or contain spaces, add logic to execute as sh script and then re-execute with specified Python Add support for handling long shebang lines Jul 22, 2017
@pradyunsg
Copy link
Member

Is there a guide to get the test suite up and running with pip?

I don't think so, feel free to ask questions here.


Maybe we could add it to the development section of pip's documentation, if there is a need for such a guide.

@pradyunsg
Copy link
Member

Also, ScriptMaker got some improvements in distlib master -- you might wanna have a look at them. A passing look makes me think that pip won't need to do a dance with the shebangs (this PR) once the code in the current master of distlib is released.

@pradyunsg pradyunsg added S: awaiting response Waiting for a response/more information type: enhancement Improvements to functionality labels Aug 18, 2017
@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 eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 1, 2017
@pradyunsg pradyunsg removed the S: awaiting response Waiting for a response/more information label Sep 30, 2017
@HaraldNordgren HaraldNordgren deleted the workaround_for_long_executables branch September 18, 2018 13:55
@lock
Copy link

lock bot commented Jun 1, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation needs rebase or merge PR has conflicts with current master type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants