'run' does not handle quote properly #191

Closed
mijikai opened this Issue Nov 9, 2014 · 4 comments

Projects

None yet

2 participants

@mijikai
mijikai commented Nov 9, 2014

Trying to run this:

import invoke

string_in_quote = "      hello\t\t\nworld with spaces"
invoke.run(""" eval 'echo "{}" ' """.format(string_in_quote))

will work as expected but when I run this with pty=True, it throws a parsing error.

This was due to the string passed as it is without the quotes escaped.

@bitprophet
Member

I don't think it's technically run's own fault...I dug a bit after reproducing the issue:

  • With pty=False, we end up running a straightforward subprocess.Popen(command, shell=True, ...) execution. Clearly this works fine.

  • With pty=True, we use a vendored pexpect module which ends up chopping up the command you give it and handing it to os.execv().

  • Because of how execv works, we have to give pexpect a wrapped version of the command (currently /bin/bash -c "command" though I expect we'll make that configurable sometime), but I don't think this is what's doing it.

  • Instead I suspect something in how pexpect is chopping up the string is breaking it, because when I print out the args it's giving to execve we get this with your input:

    ['/bin/bash', '-c', " eval 'echo ", 'hello', 'world', 'with', "spaces ' "]

    which feels pretty incorrect to me.

  • Indeed, if I do run("echo 'hi' how are 'you'?", pty=True) - which is still a nontrivial quoted multi-word string, but not one quite as wacky as yours - the same variable looks like:

    ['/bin/bash', '-c', "echo 'hi' how are 'you'?"]

    wherein the entirety of the substring given to -c is preserved as a single Python string.

Will poke a bit more and see what's happening exactly. We vendor pexpect but I'm not above patching it if necessary.

@bitprophet
Member

Yup, it does its own handrolled parsing here: https://github.com/pyinvoke/invoke/blob/902683a2496c2a6d4c2f51de6df9afe41ffd9ba2/invoke/vendor/pexpect.py#L1848

Wondering if we can simply replace that with shlex or something.

The good news is that pexpect doesn't run things through that function if we hand it an explicit args - so we can do our own splitting beforehand if we cannot determine a good way to modify that state machine, or if doing so seems worse than just doing pexpect.spawn(command, shlex(command)) or similar.

@bitprophet
Member

Sadly, shlex.split yields identical results to pexpect's own homegrown stuff. So whatever logic they're both using, really does not like the nesting going on here, despite sh/bash apparently supporting it. Gotta think a little harder to see what we can realistically do here.

@bitprophet
Member

Turns out it was way simpler than that; the entire point here is not to split the command given to bash! So simply changing our pexpect call to pexpect.spawn('/bin/bash', ['-c', command]) makes the example work fine as far as I can tell.

@bitprophet bitprophet added a commit that referenced this issue Nov 13, 2014
@bitprophet bitprophet Test proving #191 2c97b4a
@bitprophet bitprophet added a commit that referenced this issue Nov 13, 2014
@bitprophet bitprophet Changelog re #191 74f332c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment