[libcxx] [test] Quote env variables that are set with a shell "export" in ssh.py

[mstorsjo]

Summary

This safeguards against cases if some of the env vars contain chars
that are problematic for shells.

[curdeius]

LGTM.

[• Quuxplusone]

libcxx/utils/ssh.py

116

pipes.quote is gone and/or undocumented in Python 3; the Internet tells me to use shlex.quote instead.

I'm skeptical of "shell escaping" cleanups because it's essentially impossible to do right, and so any given commit is likely to make matters more complicated without making them any more correct. I'd like the commit message to contain a concrete example of something that didn't work before, that does work now.
(I'm guessing this has to do with a PATH containing names-with-spaces; but I want the commit message to tell me explicitly.)

[mstorsjo]

libcxx/utils/ssh.py

116

Yes, but the shebang only specifies python, so depending on distribution, this still can be executed by python2, deprecated or not - and shlex.quote is python3 only. Don't remember what the overall progress of python 2->3 transition/forcing scripts to python3 in llvm, but I didn't want to get sidetracked with that right now.

I can clarify the message regarding what chars are quoted/escaped. Spaces is an obvious one, but the one I tripped on actually is semicolons. (The semicolons aren't right on their own either - they come from trying to set a windows PATH while crosstesting from linux via ssh. No path from the build host make sense on the remote system anyway, regardless of how they're separated, but the cross testing setup works well for statically linked setups anyway.) Tl;dr, unneeded env vars can contain semicolons, this makes sure ssh.py doesn't trip up on them, even if they aren't entirely correct.

[curdeius]

libcxx/utils/ssh.py

116

How about doing this?

try:
    from shlex import quote as cmd_quote
except ImportError:
    from pipes import quote as cmd_quote

[mstorsjo]

libcxx/utils/ssh.py

116

Sure, I can do that

[mstorsjo]

Updated to prefer shlex.quote if available.

Amended the commit message to include this:

This safeguards against cases if some of the env vars contain chars
that are problematic for shells. (In cases of cross testing for
windows, the PATH variable can end up specified with semicolon
separators - even if specifying a PATH overall doesn't even make sense
when cross testing, but this makes ssh.py not break on such a variable.)

[• Quuxplusone]

LGTM, as long as the commit message contains that example! Sounds like an almost-complete example of what I'm looking for is
libcxx/utils/ssh.py --env "X=Y;Z" something

libcxx/utils/ssh.py

28

Let's leave a comment here like # for Python 2 compatibility
I switched from "Python 2 chauvinist" to "Python 3 chauvinist" a year or so ago — IIRC just after Python 2 was officially end-of-lifed in January 2020. So I'm happy not to cater to anyone still stuck on Python 2, but I don't mind (as long as we leave the greppable comment for later cleanup).

libcxx/utils/ssh.py

 import os
 import posixpath
 import shlex
 import subprocess
 import sys
 import tarfile
 import tempfile
 
+try:
+   from shlex import quote as cmd_quote
+except ImportError:
+   # for Python 2 compatibility
+   from pipes import quote as cmd_quote
+
 def ssh(args, command):
     cmd = ['ssh', '-oBatchMode=yes']
     if args.extra_ssh_args is not None:
         cmd.extend(shlex.split(args.extra_ssh_args))
     return cmd + [args.host, command]
 
 
 def scp(args, src, dst):
try:
 
         # Execute the command through SSH in the temporary directory, with the
         # correct environment. We tweak the command line to run it on the remote
         # host by transforming the path of test-executables to their path in the
         # temporary directory on the remote host.
         commandLine = (pathOnRemote(x) if isTestExe(x) else x for x in commandLine)
         remoteCommands.append('cd {}'.format(tmp))
         if args.env:
-            remoteCommands.append('export {}'.format(' '.join(args.env)))
+            remoteCommands.append('export {}'.format(cmd_quote(' '.join(args.env))))
         remoteCommands.append(subprocess.list2cmdline(commandLine))
 
         # Finally, SSH to the remote host and execute all the commands.
         rc = subprocess.call(ssh(args, ' && '.join(remoteCommands)))
         return rc
 
     finally:
         # Make sure the temporary directory is removed when we're done.
         subprocess.check_call(ssh(args, 'rm -r {}'.format(tmp)))
 
 
 if __name__ == '__main__':
     exit(main())