launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/shhh-no-shell into lp:launchpad.

Commit message:
Rework utilities/shhh.py to invoke the subsidiary command directly rather than via a shell.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/shhh-no-shell/+merge/330155

shell=True delenda est.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/shhh-no-shell into lp:launchpad.

=== modified file 'utilities/shhh.py'
--- utilities/shhh.py	2012-01-01 03:10:25 +0000
+++ utilities/shhh.py	2017-09-04 11:58:53 +0000
@@ -1,14 +1,15 @@
 #! /usr/bin/python -S
 #
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """
-Run a command and suppress output unless it returns a non-zero exit status
+Run a command and suppress output unless it returns a non-zero exit status.
 """
 
 __metaclass__ = type
 
+import os
 from subprocess import (
     PIPE,
     Popen,
@@ -23,49 +24,62 @@
     order may be messed up if the command attempts to control order by
     flushing stdout at points or setting it to unbuffered.
 
-
     To test, we invoke both this method and this script with some commands
-    and examine the output and exitvalue
+    and examine the output and exit status.
 
     >>> python = sys.executable
 
     >>> def shhh_script(cmd):
     ...     from subprocess import Popen, PIPE
-    ...     script = '%s %s' % (python, __file__)
-    ...     cmd = "%s '%s'" % (script, cmd)
-    ...     p = Popen(cmd, shell=True, stdout=PIPE, stderr=PIPE)
+    ...     cmd = [python, __file__] + cmd
+    ...     p = Popen(cmd, stdout=PIPE, stderr=PIPE)
     ...     (out, err) = p.communicate()
     ...     return (out, err, p.returncode)
 
-    >>> cmd = '''%s -c "import sys; sys.exit(%d)"''' % (python, 0)
+    >>> cmd = [python, "-c", "import sys; sys.exit(0)"]
     >>> shhh(cmd)
     0
     >>> shhh_script(cmd)
     ('', '', 0)
 
-    >>> cmd = '''%s -c "import sys; sys.exit(%d)"''' % (python, 1)
+    >>> cmd = [python, "-c", "import sys; sys.exit(1)"]
     >>> shhh(cmd)
     1
     >>> shhh_script(cmd)
     ('', '', 1)
 
-    >>> cmd = '''%s -c "import sys; print 666; sys.exit(%d)"''' % (
-    ...     python, 42)
+    >>> cmd = [python, "-c", "import sys; print 666; sys.exit(42)"]
     >>> shhh(cmd)
     666
     42
     >>> shhh_script(cmd)
     ('666\n', '', 42)
 
-    >>> cmd = (
-    ...     '''%s -c "import sys; print 666; '''
-    ...     '''print >> sys.stderr, 667; sys.exit(42)"''' % python
-    ...     )
+    >>> cmd = [
+    ...     python, "-c",
+    ...     "import sys; print 666; print >> sys.stderr, 667; sys.exit(42)",
+    ...     ]
     >>> shhh_script(cmd)
     ('666\n', '667\n', 42)
+
+    >>> cmd = ["TEST=sentinel value=0", "sh", "-c", 'echo "$TEST"; exit 1']
+    >>> shhh(cmd)
+    sentinel value=0
+    1
+    >>> shhh_script(cmd)
+    ('sentinel value=0\n', '', 1)
     """
 
-    process = Popen(cmd, stdout=PIPE, stderr=PIPE, shell=True)
+    env = dict(os.environ)
+    cmd = list(cmd)
+    while cmd:
+        if "=" in cmd[0]:
+            name, value = cmd[0].split("=", 1)
+            env[name] = value
+            del cmd[0]
+        else:
+            break
+    process = Popen(cmd, stdout=PIPE, stderr=PIPE, env=env)
     (out, err) = process.communicate()
     if process.returncode == 0:
         return 0
@@ -76,6 +90,4 @@
 
 
 if __name__ == '__main__':
-    cmd = ' '.join(sys.argv[1:])
-    sys.exit(shhh(cmd))
-
+    sys.exit(shhh(sys.argv[1:]))