Distutils doesn't quote Windows command lines properly

[mgiuca]

BPO	8987
Nosy	@warsaw, @jaraco, @ncoghlan, @tarekziade, @merwok, @bitdancer, @zware, @zooba, @dstufft, @eric-wieser
Files	spawn.patch quote-args.patch: _nt_quote_args fix from Intel quote-args-ext.tar.gz: C extension example for quote-args.patch libpath-fix.patch: fix formation of LIBPATH linker args

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2021-02-03.18:07:55.927>
created_at = <Date 2010-06-13.05:22:51.041>
labels = ['3.7', '3.8', 'type-bug', 'library', 'OS-windows']
title = "Distutils doesn't quote Windows command lines properly"
updated_at = <Date 2021-02-03.18:07:55.927>
user = 'https://bugs.python.org/mgiuca'

bugs.python.org fields:

activity = <Date 2021-02-03.18:07:55.927>
actor = 'steve.dower'
assignee = 'none'
closed = True
closed_date = <Date 2021-02-03.18:07:55.927>
closer = 'steve.dower'
components = ['Distutils', 'Windows']
creation = <Date 2010-06-13.05:22:51.041>
creator = 'mgiuca'
dependencies = []
files = ['17653', '40204', '40205', '40221']
hgrepos = []
issue_num = 8987
keywords = ['patch', 'needs review']
message_count = 21.0
messages = ['107722', '107772', '214164', '248785', '248855', '248856', '248858', '248863', '248884', '248892', '248896', '248897', '248898', '248904', '248967', '248984', '249005', '249007', '330070', '330089', '386262']
nosy_count = 12.0
nosy_names = ['barry', 'jaraco', 'ncoghlan', 'tarek', 'eric.araujo', 'mgiuca', 'r.david.murray', 'zach.ware', 'steve.dower', 'dstufft', 'Eric.Wieser', 'christopher.hogan']
pr_nums = []
priority = 'normal'
resolution = 'out of date'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue8987'
versions = ['Python 2.7', 'Python 3.7', 'Python 3.8']

[mgiuca]

I discovered this investigating a bug report that python-cjson doesn't compile properly on Windows (http://pypi.python.org/pypi/python-cjson). Cjson's setup.py asks distutils to compile with the flag '-DMODULE_VERSION="1.0.5"', but distutils.spawn._nt_quote_args is not escaping the quotes correctly.

Specifically, the current behaviour is:
>>> distutils.spawn._nt_quote_args(['-DMODULE_VERSION="1.0.5"'])
['-DMODULE_VERSION="1.0.5"']

I expect the following:
>>> distutils.spawn._nt_quote_args(['-DMODULE_VERSION="1.0.5"'])
['"-DMODULE_VERSION=""1.0.5"""']

Not surprising, since that function contains a big comment:
# XXX this doesn't seem very robust to me -- but if the Windows guys
# say it'll work, I guess I'll have to accept it. (What if an arg
# contains quotes? What other magic characters, other than spaces,
# have to be escaped? Is there an escaping mechanism other than
# quoting?)

It only escapes spaces, and that's it. I've proposed a patch which escapes the following characters properly: "&()<>^| (as far as I can tell, these are the "reserved" characters on Windows).

Note: I did not escape * or ?, the wildcard characters. As far as I can tell, these only have special meaning on the command-line itself, and not when supplied to a program.

Alternatively, it could call subprocess.list2cmdline (but there seem to be issues with that: http://bugs.python.org/issue8972).

[bitdancer]

Note that list2cmdline does correct quoting (which includes the "s) if you are passing the string directly to a program. In that case cmd.exe's metacharacters aren't special. (As I noted in bpo-8972, I believe that list2cmdline's current quoting of '|' characters is in error).

Perhaps a canonical list2cmdline actually belongs in shutil?

[merwok]

I’m more inclined to use the proposed patch rather than list2cmdline, given the issue linked.

I would not like a fix for this to break people’s setup.py scripts; it would be good to know if people work around this bug by monkey-patching the _nt_quote_args function, escaping quotes themselves, or something else.

If the proposed patch is the right solution, unit tests for _nt_quote_args would be needed, plus a high-level test that builds an extension module with a flag like '-DMODULE_VERSION="1.0.5"', so that the windows buildbots can confirm this works.

[christopherhogan]

At Intel, we've run into problems with external modules giving paths to _nt_quote_args that contain trailing backslashes, which escapes the final quote and breaks the command. This fix takes care of special characters, trailing backslashes, and embedded quotes as in Matt's example. I've also added unit tests for these cases, as well as a high level test that builds an extension and defines some macros that contain quotes and special characters.

setup.py - Includes a library directory with a trailing backslash. I compiled a simple library, test_module.lib, and put it in that directory to test the linking. This fails without my fix. The macro definitions also fail without this fix.

testmodule.c - A simple C extension. It just makes sure everything worked and returns the string "Success!"

As for current workarounds in setup scripts, I know numpy uses their own distutils. They wrote a quote_args() function that they use instead of _nt_quote_args(). It checks for spaces, but only if the argument isn't already quoted. It doesn't account for other special characters or trailing backslashes.

[bitdancer]

So I think the only objection to committing this as a bug fix would be the unfortunately real possibility that doing so will break someone's workaround. My *guess* is that such a workaround would most likely take the form of replacing _nt_quote_args as the one example we have in had did. So I'm in favor of fixing this.

I've added some people to nosy who might have an opinion and even perhaps some way to look for other examples of people coping with this (in open source code bases that also target windows).

I haven't reviewed the patch yet but I will.

[bitdancer]

There are two features of this I have questions about. If I'm understanding correctly, if passed a quoted string you are not re-quoting it, but you are always stripping a trailing slash even if it is inside quotes.

For the first, that seems wrong. Either we should be quoting the quotes, or we should assume that if passed a string that starts and ends with quotes, that it is already fully quoted, and not change it.

For the second, doesn't that potentially change the semantics of the string passed in? You can't actually know that the backslash is part of a path, nor can you be sure that a trailing backslash is not significant even if it is a path. And in fact if there is a backslash before a double quote, arguably that is supposed to be a double quote that doesn't end the string (see point one above).

I tried list2cmdline, and what it does is to backslash escape appropriately, including preserving blackslashes that are in front of double quotes, and does not add surrounding quotes if there are no internal blanks. I think list2cmdline is correct here (and it has certainly seen a lot of use), and I think we should either replicate its logic or just use it.

Now, subprocess calls CreateProcess, while distutils calls os.spawnv, but my understanding is that the quoting rules are the same (that is, that the CRT's _spawnv ultimately calls CreateProcess with the arguments joined by blanks).

Since bpo-8972 has been resolved by fixing the broken behavior, I think we should just use list2cmdline. We could leave _nt_quote_args alone and replace the call to it in _spawn_nt with:

  cmd = [list2cmdline([arg]) for arg in cmd]

or we could put the fix in _nt_quote_args. The backward compatibility calculus is...not obvious :(

[BreamoreBoy]

As far as I'm concerned distutils on Windows is a farce so do what you like with it. It usually can't pick up the version of VS that you know is correct and is installed, the error messages are less than useless, so the only damage that I see is something that is even more broken than it all ready is. Alternatively it gets slightly further before bombing with yet another useless message.

[zooba]

Switching distutils.spawn to simply use subprocess.Popen (probably keeping the explicit path search) looks good to me.

Quoting rules are different when you call with shell=True (aka "cmd.exe /c"), since then you also need to escape ^, | and > (with a ^), but if it's being passed directly to the process (via CreateProcess) you don't need to do this. Popen doesn't appear to handle this correctly either.

Because of the backwards compatibility concerns, realistically I think we can only fix this in 3.6 at this stage. The issue has existed for too long to convince Larry to take it for 3.5.0, and changing the behaviour of either distutils or subprocess between .0 and .1 feels risky to me.

(Adding Brett in case he's motivated to take the opportunity to definitively learn and implement Windows's quoting rules and becoming the first person in the world to achieve it. Sounds like his kind of thing :) )

[bitdancer]

*I'm* not suggesting using Popen. That would be a more significant change to the code, and I don't see a motivation for it. I'm just suggesting using list2cmdline for the quoting. (If we were rewriting distutils it would be a different story, but we're not.)

And no, Popen with shell=True does *not* handle quoting. If you use shell=True, *you* are responsible for doing correct quoting (and is what makes using shell=True so dangerous). That applies to both unix and windows, and the confusion about that fact was what led to the need for the "fix" backout in bpo-8972.

If a fix can't go in as a bug fix (whether or not it gets into 3.5.0), then we should perhaps instead consider the rewrite for 3.6: provide a *new* distutils function that does use Popen and does things "right" (based on everything we've learned since distutils was written), leaving the old function in place, deprecated, for backward compatibility.

That doesn't solve the problem for people running in to this bug trying to build extensions for 2.7, 3.4, and 3.5, though.

[zooba]

Maybe a better starting point is to monkey-patch via setuptools? I know it's nasty, but it'll give us a chance to actually iterate on this before committing to a distutils change. (IMO the biggest problem with distutils is that it gets basically no testing before a final release, other than people who have already worked around its issues.) CCompiler.spawn is fairly easy to patch too.

Adding Jason in case he's still so upset from last time I did this (finding VC compiler for 2.7) he wants to shut the suggestion down immediately :)

As an aside, escaping quotes by doubling them (as in the OP) works fine for MSVC, but is not necessarily standard across all applications. Escaping with a backslash is probably more common, but (e.g.) batch files don't need them escaped if they aren't followed by a space and can't escape them at all if they are followed by a space (then again, we all agree that cmd.exe is fundamentally broken here, so using https://msdn.microsoft.com/en-us/library/windows/desktop/bb776391.aspx (CommandLineToArgvW API) as the benchmark is reasonable).