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

Overzealous string quoting / space escaping #124

Closed
AndydeCleyre opened this issue May 13, 2014 · 16 comments
Closed

Overzealous string quoting / space escaping #124

AndydeCleyre opened this issue May 13, 2014 · 16 comments

Comments

@AndydeCleyre
Copy link
Contributor

Hi,

I'm using plumbum 1.4.1 to call reflector on Arch Linux. I'm not sure what the problem is, but somewhere along the way, way too much string/space escaping happens.

Here is code to reproduce the problem:

#!/usr/bin/env python3
from itertools import chain
from plumbum.cmd import reflector, sudo

def update_mirrorlist(countries=['United States', 'Canada']):
    countries = list(chain(*[('--country', country) for country in countries]))
    print(sudo[reflector['--save', '/etc/pacman.d/mirrorlist', '--sort', 'score', '--latest', '30', countries]])
    sudo(reflector['--save', '/etc/pacman.d/mirrorlist', '--sort', 'score', '--latest', '30', countries])

if __name__ == '__main__':
    update_mirrorlist()

This yields the output

/usr/bin/sudo /usr/bin/reflector --save /etc/pacman.d/mirrorlist --sort score --latest 30 --country 'United States' --country Canada

which looks perfect. If I run that printed command exactly, I get the desired result. However, that is not what gets run by the final line in update_mirrorlist. The generated mirrorlist does not include anything from the United States, and the comments report that the command actually run was

reflector --save /etc/pacman.d/mirrorlist --sort score --latest 30 --country ''"'"'United States'"'"'' --country Canada
@AndydeCleyre
Copy link
Contributor Author

I've confirmed this bug with plumbum 1.4.2 as well.

@tomerfiliba
Copy link
Owner

you mustn't use sudo(cmd) -- always use sudo[cmd]()
e.g., sudo[reflector['--save', '/etc/pacman.d/mirrorlist', '--sort', 'score', '--latest', '30', countries]]() should do the trick

@tomerfiliba
Copy link
Owner

please close the issue if it does

@AndydeCleyre
Copy link
Contributor Author

Thanks, but exactly the same results with the suggested syntax.

@arunaugustine
Copy link

Is there a view to see the exact string that is executed by plumbum ()?

May not be related but I am having some trouble with adb shell commands with:

/Library/Python/2.7/site-packages/plumbum/commands/processes.pyc in run_proc(proc, retcode, timeout)
205 elif proc.returncode != retcode:
206 raise ProcessExecutionError(getattr(proc, "argv", None), proc.returncode,
--> 207 stdout, stderr)
208 return proc.returncode, stdout, stderr
209

ProcessExecutionError: Command line: ['/Users/redacted/adb', '-s', "'redacted device id'", "install -r 'someapkname.apk'"]
Exit code: 1

Am I missing something or is this related to string quoting or string formatting?

@arunaugustine
Copy link

i realized i was doing:

adb_ = local["adb"]
adb_device_sepecific = adb_["-s", deviceid]
adb_install = adb_device_sepecific["-r install", apk_path]

The string "-r install" was the problem. When broken up, all was well.
Sorry for hijacking this issue.

@AndydeCleyre
Copy link
Contributor Author

@tomerfiliba, do you have any tips on further debugging this?

@tomerfiliba
Copy link
Owner

i found that changing the order of brackets solves the issue for me. e.g. sudo[foo]["bar"] as opposed to sudo[foo["bar"]]. can't think of anything else that might help.

@AndydeCleyre
Copy link
Contributor Author

Thanks, it did help. There's definitely something fishy going on, though.

print(sudo[reflector]['--save', '/etc/pacman.d/mirrorlist', '--sort', 'score', '--latest', '30', countries])

Incorrectly prints

/usr/bin/sudo /usr/bin/reflector --save /etc/pacman.d/mirrorlist --sort score --latest 30 --country United States --country Canada

while actually running

sudo[reflector]['--save', '/etc/pacman.d/mirrorlist', '--sort', 'score', '--latest', '30', countries]()

works, and the output file reports that the command was run as

reflector --save /etc/pacman.d/mirrorlist --sort score --latest 30 --country 'United States' --country Canada

@tomerfiliba
Copy link
Owner

print can be misleading, you should use .formulate() to see the actual command line:

>>> ssh["foo.bar", ls["-la"]].formulate()
['C:\\Program Files (x86)\\Git\\bin\\ssh.exe', 'foo.bar', 'C:\\Program Files (x86)\\Git\\bin\\ls.exe', '-la']
>>> ssh["foo.bar", ls["-la", '"xxx"']].formulate()
['C:\\Program Files (x86)\\Git\\bin\\ssh.exe', 'foo.bar', 'C:\\Program Files (x86)\\Git\\bin\\ls.exe', '-la', '\'"xxx"\'']

@khorn
Copy link
Contributor

khorn commented Nov 12, 2014

I'm seeing what I think is this same problem, in my case calling mysqldump and passing the password on the command line. The password contains an asterisk, which triggers shell escaping. This would be fine, except that the mysqldump command is nested in a nice command so that it doesn't slow my db server to a crawl. This makes the password get escaped a second time, essentially injecting quotes into my password. This of course, makes the command fail, as I'm not sending the wrong password.

I think this issue should be reopened, as it looks like auto-escaping is broken for any nested commands.

Here's an example of what I'm trying to do:

r_nice["-n", "20", r_mysqldump["--password"]["1234567890*"]]

note that I have to use separate bindings to add --password and the password itself, rather than using --password=1234567890*, because that would escape the entire string. That's kind of annoying, but not really what I'm complaining about.

@tomerfiliba
Copy link
Owner

Same trick would work here too:

r_nice["-n", "20", r_mysqldump]["--password", "1234567890*"]

@khorn
Copy link
Contributor

khorn commented Nov 12, 2014

Unfortunately it doesn't work, for a couple of reasons:

  • my command is actually a lot more complex than that
  • splitting --password and 1234567890* doesn't actually work...mysqldump treats the password as the database name...apparently you have to use the --password=1234567890* syntax, so I don't know what to do about that
  • that trick means that the password is only escaped once, but I think that still might be too much, especially considering the second point above

@khorn
Copy link
Contributor

khorn commented Nov 12, 2014

What would be nice (feature request!) would be a way to turn off auto-escaping for a given command. Maybe a flag that could be set that formulate() could use to determine whether or not to do any quoting, This could be a class attribute, or maybe a keyword arg to __call__()? I don't grok the code well enough yet to know what the best way to implement this would be.

@khorn
Copy link
Contributor

khorn commented Nov 13, 2014

For future searchers, I was able to work around my issue by using the MYSQL_PWD environment variable, thus avoiding passing the password on the command line.

I'd still like to see a way to disable (or otherwise control) plumbum's auto-escaping behavior in future though.

@tomerfiliba
Copy link
Owner

yep, that's a good idea. please open a different issue on that.

-tomer


Tomer Filiba
tomerfiliba.com http://www.facebook.com/tomerfiliba
http://il.linkedin.com/in/tomerfiliba

On Thu, Nov 13, 2014 at 2:18 AM, Kevin Horn notifications@github.com
wrote:

For future searchers, I was able to work around my issue by using the
MYSQL_PWD environment variable, thus avoiding passing the password on the
command line.

I'd still like to see a way to disable (or otherwise control) plumbum's
auto-escaping behavior in future though.


Reply to this email directly or view it on GitHub
#124 (comment).

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

No branches or pull requests

4 participants