Finally making distlib handle spaces
Hi all, Since at least 2011, virtualenv has not supported spaces in paths. This has bitten many people, including myself, and caused numerous issues over the years [1] [2] [3] [4] [5] [6] [7]. However, as was discussed in [8], the issue lies not with virtualenv but with distlib, via pip. It would be possible for pip to use the existing distlib interface to hack around the problem, but I believe the current behavior of distlib is erroneous when it comes to spaces in paths. I therefore believe it would be more appropriate to fix the problem in distlib. Two separate patches [9] [10] that solve the problem in distlib were posted in January by Harald Nordgren. However, they were declined pending a discussion on distutils-sig [11]. As far as I can tell, no such discussion was ever started. However, the issue remains, and we have a clear solution proposal to consider, so I'd like to kick it off now. In the remainder of this email, I'll explain the problem and surrounding context in detail, and why I think the solution proposed by Harald (or some variation) is a good path forward for distlib. I look forward to hearing your thoughts on the matter. ========================== The behavior of virtualenv ========================== The following is written for: $ python --version Python 2.7.13 $ pip --version pip 9.0.1 from /usr/local/lib/python2.7/site-packages (python 2.7) $ virtualenv --version 15.1.0 Creating a virtualenv is done as follows: $ virtualenv venv New python executable in /private/tmp/path with spaces/venv/bin/python2.7 Also creating executable in /private/tmp/path with spaces/venv/bin/python Installing setuptools, pip, wheel...done. This creates a directory structure looking as follows under the venv directory: ├── bin │ ├── activate │ ├── activate.csh │ ├── activate.fish │ ├── activate_this.py │ ├── easy_install │ ├── easy_install-2.7 │ ├── pip │ ├── pip2 │ ├── pip2.7 │ ├── python -> ./bin/python2.7 │ ├── python-config │ ├── python2 -> ./bin/python2.7 │ ├── python2.7 │ └── wheel ├── include │ └── ... ├── lib │ └── ... └── ... The idea is that one can call the pip and python executables inside the virtualenv, instead of the system ones. Like so: $ venv/bin/python --version Python 2.7.13 $ venv/bin/pip --version zsh: venv/bin/pip: bad interpreter: "/private/tmp/path: no such file or directory Unfortunately, as you can see, pip doesn't work at all! Why does this happen? While the python executable is a native binary, pip is actually just a Python script, which is specified to be run by the accompanying virtualenv python executable. Here are the contents: #!"/private/tmp/path with spaces/venv/bin/python2.7" # -*- coding: utf-8 -*- import re import sys from pip import main if __name__ == '__main__': sys.argv[0] = re.sub(r'(-script\.pyw?|\.exe)?$', '', sys.argv[0]) sys.exit(main()) The issue is that the python binary is specified using a shebang, which is known [12] [13] to be fragile, OS-dependent, and error-prone regarding paths that are long or that contain spaces or non-ASCII characters. In particular, the quoting of the shebang does not work on most operating systems, including macOS, which is what I ran this test on. ====================== virtualenv and distlib ====================== The issue is complicated by the fact that there are several different libraries at play. To perform the installation of pip and wheel into the virtualenv, virtualenv calls into pip [14]. The 'pip install' command then uses the subroutine library 'wheel.py', which generates the stub scripts using distlib's ScriptMaker [15]. It is actually distlib which generates the shebang, although this can be overridden by setting the 'executable' property of the ScriptMaker object [16] [17]. Any patch to fix the virtualenv problem would therefore need to be in either pip (as a consumer of the shebang-generation interface) or distlib (as the provider of that interface). The problem cannot be addressed by virtualenv without doing something like using pip/distlib to generate the scripts and then fixing them after the fact (this has been proposed [26] [29], but I consider it a hack). ================== Proposed solutions ================== There has been extended discussion about this issue, especially in [2]. Essentially, the solutions proposed fall into four categories: (1) Don't change anything; end users can work around the issue. For example, they can place their virtualenvs in a different directory than their project, or change their username to avoid having spaces or non-ASCII characters. (2) Don't fix the bug, but add a warning to virtualenv. If we absolutely can't fix the bug (which I strongly believe not to be the case), then this would be the next best thing to do. See [27] [30] [31]. (3) Attempt to escape the shebang, for example by using backslashes or quotes. The latter has already been implemented. Unfortunately, this does not work on most operating systems, since shebangs are interpreted by the kernel in a rather primitive way. (4) Patch either pip or distlib to use a different strategy for dispatching the python binary to be used. For example, make the pip script into a shell script with '#!/usr/bin/env sh' that then invokes the real pip script using the appropriate python binary [9]; this works because even POSIX sh supports escaping arbitrary characters in executable paths. Alternatively, use a clever hack to make the pip script executable as both a Python script and as a shell script [10]. This idea originated from [19]. I argue that approaches (1), (2), and (3) are inadequate, and that (4) or some variation thereof is the best path forward for distlib. Regarding (4), note that in addition to the second pull request [10], there is another, possibly cleaner implementation of the same idea at [18], by the same author. Since this issue has been discussed since 2011 without being fixed, I obviously need to justify my position. =================================== Justifications and counterarguments ===================================
There's an easy workaround for the end user, so there's no need to change things on the pip/distlib side.
The workaround may not necessarily be "easy". For example, there have been reports of this problem being unavoidable on macOS in certain circumstances, since the drive name ("Macintosh HD") has a space in it [1]. People working in corporate environments may not be able to simply move their virtualenvs to a different directory (possibly one outside their home directory, if their username has a space or non-ASCII character in it). These issues really do happen, and really do inconvenience people [20]. However, setting these cases aside, the workaround is indeed not too hard for most people. The question is: do we really want virtualenv to have this behavior? To me, it honestly seems embarrassing that such an important tool, and a cornerstone of the Python ecosystem, can't handle something as simple as spaces in directory names, even more than six years after the bug was first reported. If there were a bug in the development version of Git that prevented it from working at all when you had your repository in a path with spaces, nobody would even consider shipping a release! In 2017, people use Unicode and spaces in their filesystems, and they expect their tools to handle this. In my opinion, end-user workarounds should be reserved for those cases where it is absolutely impossible for the software to solve the problem itself without introducing even worse problems. This is simply not the case with the distlib/pip path bug. Patches that present elegant ways to solve the issue without breaking backward compatibility are available [9] [10]. There is no risk of breaking programs that rely on the previous behavior, since the previous behavior was a complete inability for virtualenv to function *at all*, even for 'pip --version'!
People who use long paths and/or paths with spaces and/or paths with non-ASCII characters in them are rare.
No, that's not true [1] [2] [3] [4] [5] [6] [7] [23] [24] [28]. Considering the tiny fraction of people running into bugs who actually report them, I'd say quite a few people run into this issue. It's pretty rare that I'm able to scrounge up this number of issues all leading to a single bug. (I've also run into it, on multiple occasions over several years, by the way.)
The issue with spaces is a limitation of the way in which the kernel parses shebangs, and this can't be fixed.
This is technically true [13], but irrelevant [21]. The implications of this fact are that virtualenv cannot solve this issue *if it continues to directly use shebangs for interpreter dispatch*. However, there are many other ways to dispatch interpreters [9] [10], none of which have the same limitations of the shebang. Thus the limitations of shebangs have no bearing on this issue, unless it turns out that it's completely impossible to dispatch scripts in any other way while maintaining compatibility with all supported platforms, which seems extremely unlikely to me.
We don't want to make the code more complicated unless there's a good reason for it.
I agree. In my opinion, the complete failure of virtualenv to operate in a common real-world use case is a good reason. However, beyond this, we can be even more conservative. In Harald's alternate patch [18], the polyglot script hack is only used in situations where the path is too long for a shebang (conditional on the OS) or it contains spaces. Thus, it's only possible for the patch to improve things, since it is only activated in cases where virtualenv was completely nonfunctional before. Look at software like Bash. It has implicit wordsplitting, which makes dealing with spaces in paths difficult. However, it is *extremely* well-understood that it is a best practice to disable wordsplitting by quoting arguments [22]. That is, modern software is expected to handle spaces and other special characters in paths. Failing to do so leads to hard-to-trace bugs and potential security vulnerabilities.
We don't want to change the default behavior of distlib. pip should handle its shebang needs using the ScriptMaker.executable property, which was provided for this purpose.
Now we are getting somewhere. This is one of two solutions proposed which would actually fix the bug. However, I feel that it is the wrong way to go about it. In my opinion, distlib (and specifically ScriptMaker) are providing an interface. This interface specifies that when you use distlib to generate a script, and then run the script, it will invoke the executable that you told it to. This is not currently the case when there are spaces in the path leading to the executable. In my opinion, this is a *bug* in that distlib fails to fulfill its specification. Yes, other applications that use distlib could work around this bug, but it is fundamentally a bug in distlib, and should be fixed there. The ability to support spaces in paths should not be a special feature that must be hacked in (separately, resulting in code duplication and increased maintenance burden) to every application that wants to use distlib to generate executables. If we really want to go that way, though, Harald has made pull requests for both pip [25] and virtualenv [26]. ========== Next steps ========== I'd love to hear people's thoughts on the issues raised here. If we decide to go forward with patching distlib, I think the logical next step would be to look at Harald's patches and see if they need to modified to work correctly on all the systems that Python/pip support. As I mentioned earlier, the last comment on [9] was a request for a discussion to be started on distutils-sig, as this email intends. What do people think about [9], [10], [18]? Best regards, Radon Rosborough [1]: http://stackoverflow.com/q/15472430/3538165 [2]: https://github.com/pypa/virtualenv/issues/53 [3]: https://github.com/pypa/virtualenv/issues/994 [4]: https://github.com/pypa/pip/issues/923 [5]: https://bugs.python.org/issue20622 [6]: https://groups.google.com/forum/#!topic/virtualenvwrapper/vqW3fedTgKc [7]: https://bugs.launchpad.net/virtualenv/+bug/241581 [8]: https://github.com/pypa/virtualenv/issues/53#issuecomment-302005361 [9]: https://bitbucket.org/pypa/distlib/pull-requests/31 [10]: https://bitbucket.org/pypa/distlib/pull-requests/32 [11]: https://bitbucket.org/pypa/distlib/pull-requests/31/#comment-29795586 [12]: https://bitbucket.org/pypa/distlib/pull-requests/31/#comment-29734103 [13]: https://lists.gnu.org/archive/html/bug-bash/2008-05/msg00052.html [14]: https://github.com/pypa/virtualenv/blob/c1ef9e29bfda9f5b128476d0c6d865ffe681... [15]: https://github.com/pypa/pip/blob/7c4353e6834a38b36a34234db865e13a589c9e5f/pi... [16]: https://bitbucket.org/pypa/distlib/src/216bd857431686493d37eb081614d91c1356f... [17]: http://pythonhosted.org/distlib/tutorial.html#specifying-a-custom-executable... [18]: https://bitbucket.org/pypa/distlib/pull-requests/33 [19]: https://hg.mozilla.org/mozilla-central/file/tip/mach [20]: https://bitbucket.org/pypa/distlib/pull-requests/31/#comment-29735496 [21]: https://github.com/pypa/virtualenv/issues/53#issuecomment-302019457 [22]: http://mywiki.wooledge.org/BashGuide/Practices#Quoting [23]: https://codedump.io/share/7xYRbcZCNihE/1/using-virtualenv-with-spaces-in-a-p... [24]: https://github.com/pypa/virtualenv/issues/997 [25]: https://github.com/pypa/pip/pull/4237 [26]: https://github.com/pypa/virtualenv/pull/1004 [27]: https://github.com/pypa/virtualenv/issues/1039 [28]: https://github.com/pypa/virtualenv/issues/1014 [29]: https://github.com/pypa/virtualenv/pull/910 [30]: http://bugs.python.org/issue28446 [31]: https://github.com/pypa/virtualenv/issues/53#issuecomment-282466994
On 20 May 2017 at 15:45, Radon Rosborough <radon.neon@gmail.com> wrote:
=================================== Justifications and counterarguments =================================== [...]
While you make a good case countering the issues you quote, I think there's an important issue you haven't addressed, and I'd like to hear your comments on it: The current behaviour fails, as you note, but it does so in a "standard" way - shebang behaviour (and its limitations) is well-known. At the moment, your proposal is just to use "an alternative" launch process. Without a specific proposal, it's impossible to judge whether the solution is better than the current situation. And indeed, any proposed solution needs to demonstrate that it doesn't have security vulnerabilities or other issues that make it just as much a problem as the status quo (if not more). For example, I would have thought that "#!/usr/bin/env sh" runs the risk of picking up a malicious sh executable injected into the user's PATH. Also, different systems use different sh implementations - so care would need to be taken to only code in the lowest common denominator syntax. I suggest that the next step needs to be to propose a specific implementation of the wrapper. The specifics can then be debated, but you'd need a pretty solid proposal - otherwise the discussion will descend into multiple proposals and bikeshedding, and is likely to stall without achieving anything. One other point, which I'd hope is obvious. On Windows (where shebang processing is handled by the wrapper exe, and is well defined and robust) there should be no change to the current behaviour. Paul
The current behaviour fails, as you note, but it does so in a "standard" way - shebang behaviour (and its limitations) is well-known.
I agree with this, but in my opinion the shebang is simply an implementation detail of virtualenv. I would like to quote @JDLH from [1]: "There is nothing about the value provided by virtualenv that demands it use the shebang." If the shebang were fundamentally necessary to provide the functionality of virtualenv, it would make sense for virtualenv to inherit the shebang's restrictions. But this is not the case, so I think that the shebang should be considered an implementation detail that the end user should not need to be aware of.
At the moment, your proposal is just to use "an alternative" launch process. Without a specific proposal, it's impossible to judge whether the solution is better than the current situation.
We already have three specific patches which provide alternative launch processes: [2], [3], and [4]. I feel like that should be specific enough to start a discussion. In fact, Vinay specifically requested a discussion about [2] be raised on distutils-sig [5]. The only reason that no action has been taken is that nobody started that discussion (until now).
I would have thought that "#!/usr/bin/env sh" runs the risk of picking up a malicious sh executable injected into the user's PATH.
That's certainly a valid concern. Does this happen in the real world? I feel like if you have a malicious sh executable on your PATH, you're going to have a lot more problems than just from virtualenv. But this is a good reason that we may want to restrict the patch to only take effect when using the shebang directly would fail.
Also, different systems use different sh implementations - so care would need to be taken to only code in the lowest common denominator syntax.
Can we assume POSIX compatibility? Even if not, we're not doing anything complicated, only passing some arguments to a command. Surely that can be done in pretty much any shell one can find.
multiple proposals and bikeshedding
Although we have three implementations, my personal preference is for [4]. This is because: * It avoids the need for creating new files. * It only takes effect when necessary (i.e. when the shebang won't work). * The code is fairly clean.
On Windows (where shebang processing is handled by the wrapper exe, and is well defined and robust) there should be no change to the current behaviour.
Agreed. [1]: https://github.com/pypa/virtualenv/issues/53#issuecomment-302019457 [2]: https://bitbucket.org/pypa/distlib/pull-requests/31 [3]: https://bitbucket.org/pypa/distlib/pull-requests/32 [4]: https://bitbucket.org/pypa/distlib/pull-requests/33 [5]: https://bitbucket.org/pypa/distlib/pull-requests/31#comment-29795586
* Radon Rosborough <radon.neon@gmail.com>, 2017-05-20, 14:29:
I would have thought that "#!/usr/bin/env sh" runs the risk of picking up a malicious sh executable injected into the user's PATH. That's certainly a valid concern. Does this happen in the real world? I feel like if you have a malicious sh executable on your PATH, you're going to have a lot more problems than just from virtualenv.
Right. It's safe to assume that all elements of PATH are trusted. If they're not, all bets are off. That said, I'm surprised that "#!/usr/bin/env sh" was proposed instead of the more conventional "#!/bin/sh". I don't really see any advantages of the former. -- Jakub Wilk
On 21 May 2017 at 07:29, Radon Rosborough <radon.neon@gmail.com> wrote:
The current behaviour fails, as you note, but it does so in a "standard" way - shebang behaviour (and its limitations) is well-known.
I agree with this, but in my opinion the shebang is simply an implementation detail of virtualenv. I would like to quote @JDLH from [1]: "There is nothing about the value provided by virtualenv that demands it use the shebang." If the shebang were fundamentally necessary to provide the functionality of virtualenv, it would make sense for virtualenv to inherit the shebang's restrictions. But this is not the case, so I think that the shebang should be considered an implementation detail that the end user should not need to be aware of.
I agree with this way of looking at the problem, so my perspective would be: 1. Don't change anything on Windows (since that already uses the custom 'py' dispatcher) 2. Don't change anything for cases where platform provided shebang dispatch is trusted to be correct (i.e. no quoting of the shebang line is needed) 3. Change the cases that currently quote the shebang line to instead invoke a custom dispatch script running in /bin/sh I also agree that distlib is the right level to implement the change - this isn't about people wanting custom behaviour, it's about distlib's default dispatch mechanism having been found not to work in certain cases, so it makes sense to automatically switch to an alternative that *does* work. The custom dispatcher doesn't need to solve 100% of the currently failing cases - it just needs to solve some of them, and provide a foundation for future iterations on the design and implementation to handle more. Cheers, Nick. P.S. I was originally going to ask "Can we use Python to implement the dispatch instead?", but then realised there are actually lots of messy complications with that around getting program metadata and environmental details right that sh deals with natively. So /bin/sh is a better way to go. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On May 20, 2017 9:30 PM, "Nick Coghlan" <ncoghlan@gmail.com> wrote: On 21 May 2017 at 07:29, Radon Rosborough <radon.neon@gmail.com> wrote:
The current behaviour fails, as you note, but it does so in a "standard" way - shebang behaviour (and its limitations) is well-known.
I agree with this, but in my opinion the shebang is simply an implementation detail of virtualenv. I would like to quote @JDLH from [1]: "There is nothing about the value provided by virtualenv that demands it use the shebang." If the shebang were fundamentally necessary to provide the functionality of virtualenv, it would make sense for virtualenv to inherit the shebang's restrictions. But this is not the case, so I think that the shebang should be considered an implementation detail that the end user should not need to be aware of.
I agree with this way of looking at the problem, so my perspective would be: 1. Don't change anything on Windows (since that already uses the custom 'py' dispatcher) 2. Don't change anything for cases where platform provided shebang dispatch is trusted to be correct (i.e. no quoting of the shebang line is needed) 3. Change the cases that currently quote the shebang line to instead invoke a custom dispatch script running in /bin/sh I agree that using /bin/sh seems more sensible. I don't understand why the proposal used /use/bin/env, and would be curious to hear if there's a rationale... I also agree that distlib is the right level to implement the change - this isn't about people wanting custom behaviour, it's about distlib's default dispatch mechanism having been found not to work in certain cases, so it makes sense to automatically switch to an alternative that *does* work. +1 The custom dispatcher doesn't need to solve 100% of the currently failing cases - it just needs to solve some of them, and provide a foundation for future iterations on the design and implementation to handle more. Cheers, Nick. P.S. I was originally going to ask "Can we use Python to implement the dispatch instead?", but then realised there are actually lots of messy complications with that around getting program metadata and environmental details right that sh deals with natively. So /bin/sh is a better way to go. And using python is a non starter anyway because we may not have access to any python interpreter at a path that can be mentioned in a shebang line! -n
participants (5)
-
Jakub Wilk -
Nathaniel Smith -
Nick Coghlan -
Paul Moore -
Radon Rosborough