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

escape string in __VIRTUAL_ENV__ path for bash #811

Closed
wants to merge 2 commits into from

Conversation

davidbstein
Copy link

Fixes issue #53 in bash and shell only. Uses the printf %q "shell quote" formatter. Falls back to quoting some characters that universally need to be escaped.

Fallback is needed because while bash's printf command has the %q formatter, the builtin function printf may not.

(edit): removed irrelevant link

Fixes issue pypa#53 in bash and shell by using the printf %q "shell quote" formatter, which escapes as appropriate for a given environment. Falls back to quoting some characters that universally need to be escaped. Fallback is needed because there are several printf implementations, not all of which are guaranteed to have the %q formatter (though bash always should).

uncompressed diff here: davidbstein/virtualenv_decompressed_scripts@0bc6629
@Ivoz
Copy link

Ivoz commented Oct 18, 2015

Hi there, you need to actually commit changes for virtualenv_embedded/activate.sh as well.

Did you understand not to do so from reading the documentation somewhere? If so we need to clarify it for everyone.

@davidbstein
Copy link
Author

Hi Matt -

Completely my fault, have had a fix locally for a while and pushed it
upstream without checking contributing.rst - docs are quite clear. Sorry.

@Ivoz
Copy link

Ivoz commented Oct 20, 2015

Hi @davidbstein upon looking further, I don't see how this solves #53 at all?

The #! line is interpreted by the system kernel, not bash.

@pfmoore
Copy link
Member

pfmoore commented Oct 20, 2015

Regardless of whether this PR does fix the problem, would it be worth adding a test that demonstrates the issue, so we'd at least be able to see the problem via a test?

@davidbstein
Copy link
Author

thanks for the quick review!

@Ivoz - AFAICT, #53 is caused by the bad entry in PATH, not the malformed shebang - easy_install and pip work again with this change (testing on Ubuntu and OSX - escaping the PATH fixes the issue, escaping the shebang line does not). Fixing the shebang line seems like a good idea - I'll try to dig into a good way to do that.

@pfmoore - tests seem like a great idea. Will add one.

@Ivoz
Copy link

Ivoz commented Jan 18, 2016

@davidbstein so after manually applying your patch to test,

I created a new virtualenv called "h a", confirm the patch is in the new virtualenv:

(h\ a) ivo·ivosung ~/code/pypa/virtualenv/h a/bin develop $ cat activate | grep 'VIRTUAL_ENV='
# VIRTUAL_ENV="/home/ivo/code/pypa/virtualenv/h a"
VIRTUAL_ENV=$(printf %q "/home/ivo/code/pypa/virtualenv/h a") || VIRTUAL_ENV=$(echo "/home/ivo/code/pypa/virtualenv/h a" | sed 's/[ ()$;]/\\&/g')

Yes, we've managed to escape it in the $PATH:

(h\ a) ivo·ivosung ~/code/pypa/virtualenv/h a/bin develop $ echo $PATH
/home/ivo/code/pypa/virtualenv/h\ a/bin:/home/ivo/bin:/hom...

But, We can't escape the hashbang problem:

(h\ a) ivo·ivosung ~/code/pypa/virtualenv/h a/bin develop $ pip -h
/home/ivo/.pyenv/plugins/pyenv-pip-rehash/libexec/pip: ./pip: "/home/ivo/code/pypa/virtualenv/h\: bad interpreter: No such file or directory
 126 (h\ a) ivo·ivosung ~/code/pypa/virtualenv/h a/bin develop $ easy_install -h
/home/ivo/.pyenv/plugins/pyenv-pip-rehash/libexec/easy_install: ./easy_install: "/home/ivo/code/pypa/virtualenv/h: bad interpreter: No such file or directory

So we've put an escape in the $VIRTUAL_ENV env var & $PATH, but paths could be resolved without that anyway; and we haven't fixed execution of scripts as shown above. So we still haven't fixed scripts being broken, as is title of #53

@Ivoz
Copy link

Ivoz commented Jan 18, 2016

I'm pretty sure that escaping paths in $PATH is in fact incorrect. A) They don't get evaluated, hence no need or reason to process an escape, and B) bash / shell is not the only thing interacting with environment variables. A bash $PATH is split by colons, not by anything else.

I'm guessing that this is only working for you (or, appearing to work), because now that your patch with the backslash escape has made the virtualenv path invalid, when you type $ pip, $ easy_install etc you are finding working executable, but they are the global executables (I'd guess $ which pip will give you /usr/bin/pip) - we obviously don't want those. If you type $ ./bin/pip you will still find the command breaking from the space in the hashbang.

@Ivoz Ivoz closed this Jan 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants