launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #30176
[Merge] ~cjwatson/launchpad:avoid-subprocess-shell into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:avoid-subprocess-shell into launchpad:master.
Commit message:
Avoid calling subprocess with shell=True
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/445865
While none of these are actually security vulnerabilities as far as I know (they're all in tests or in utilities that are only used locally), it's generally bad practice to run subprocesses via a shell. Avoid this where it's relatively trivial to do so.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:avoid-subprocess-shell into launchpad:master.
diff --git a/lib/lp/answers/doc/expiration.rst b/lib/lp/answers/doc/expiration.rst
index 18b467e..84ac916 100644
--- a/lib/lp/answers/doc/expiration.rst
+++ b/lib/lp/answers/doc/expiration.rst
@@ -144,8 +144,7 @@ somebody are subject to expiration.
# Run the script.
>>> import subprocess
>>> process = subprocess.Popen(
- ... "cronscripts/expire-questions.py",
- ... shell=True,
+ ... ["cronscripts/expire-questions.py"],
... stdin=subprocess.PIPE,
... stdout=subprocess.PIPE,
... stderr=subprocess.PIPE,
diff --git a/lib/lp/bugs/doc/bugnotification-sending.rst b/lib/lp/bugs/doc/bugnotification-sending.rst
index abf8f70..d254cc3 100644
--- a/lib/lp/bugs/doc/bugnotification-sending.rst
+++ b/lib/lp/bugs/doc/bugnotification-sending.rst
@@ -661,8 +661,7 @@ makes it write out the emails it sends.
>>> import subprocess
>>> process = subprocess.Popen(
- ... "cronscripts/send-bug-notifications.py -v",
- ... shell=True,
+ ... ["cronscripts/send-bug-notifications.py", "-v"],
... stdin=subprocess.PIPE,
... stdout=subprocess.PIPE,
... stderr=subprocess.PIPE,
diff --git a/lib/lp/bugs/doc/bugtask-expiration.rst b/lib/lp/bugs/doc/bugtask-expiration.rst
index 8087a48..abaa762 100644
--- a/lib/lp/bugs/doc/bugtask-expiration.rst
+++ b/lib/lp/bugs/doc/bugtask-expiration.rst
@@ -561,8 +561,7 @@ config.malone.expiration_dbuser.
>>> import subprocess
>>> process = subprocess.Popen(
- ... "cronscripts/expire-bugtasks.py",
- ... shell=True,
+ ... ["cronscripts/expire-bugtasks.py"],
... stdin=subprocess.PIPE,
... stdout=subprocess.PIPE,
... stderr=subprocess.PIPE,
diff --git a/lib/lp/bugs/doc/externalbugtracker-debbugs.rst b/lib/lp/bugs/doc/externalbugtracker-debbugs.rst
index d35b5e3..34861ea 100644
--- a/lib/lp/bugs/doc/externalbugtracker-debbugs.rst
+++ b/lib/lp/bugs/doc/externalbugtracker-debbugs.rst
@@ -706,8 +706,7 @@ tracker.
>>> import subprocess
>>> process = subprocess.Popen(
- ... "scripts/import-debian-bugs.py 237001 322535",
- ... shell=True,
+ ... ["scripts/import-debian-bugs.py", "237001", "322535"],
... stdin=subprocess.PIPE,
... stdout=subprocess.PIPE,
... stderr=subprocess.PIPE,
diff --git a/lib/lp/bugs/model/tests/test_bugtask.py b/lib/lp/bugs/model/tests/test_bugtask.py
index c3eb248..e26983b 100644
--- a/lib/lp/bugs/model/tests/test_bugtask.py
+++ b/lib/lp/bugs/model/tests/test_bugtask.py
@@ -3871,8 +3871,7 @@ class TestTargetNameCache(TestCase):
transaction.commit()
process = subprocess.Popen(
- "cronscripts/update-bugtask-targetnamecaches.py",
- shell=True,
+ ["cronscripts/update-bugtask-targetnamecaches.py"],
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
diff --git a/lib/lp/registry/doc/convert-person-to-team.rst b/lib/lp/registry/doc/convert-person-to-team.rst
index be3ef31..a64e685 100644
--- a/lib/lp/registry/doc/convert-person-to-team.rst
+++ b/lib/lp/registry/doc/convert-person-to-team.rst
@@ -16,8 +16,7 @@ team and the name of the team owner as arguments.
>>> from subprocess import Popen, PIPE
>>> process = Popen(
- ... "scripts/convert-person-to-team.py -q matsubara mark",
- ... shell=True,
+ ... ["scripts/convert-person-to-team.py", "-q", "matsubara", "mark"],
... stdin=PIPE,
... stdout=PIPE,
... stderr=PIPE,
diff --git a/lib/lp/registry/doc/distribution-mirror.rst b/lib/lp/registry/doc/distribution-mirror.rst
index a3bb64d..db50e08 100644
--- a/lib/lp/registry/doc/distribution-mirror.rst
+++ b/lib/lp/registry/doc/distribution-mirror.rst
@@ -741,14 +741,14 @@ First we need to run the http server that's going to answer our requests.
>>> http_server.setUp()
>>> import subprocess
- >>> def run_prober(arguments):
- ... cmd = (
- ... "cronscripts/distributionmirror-prober.py %s "
- ... "--no-remote-hosts" % arguments
- ... )
+ >>> def run_prober(*arguments):
+ ... cmd = [
+ ... "cronscripts/distributionmirror-prober.py",
+ ... "--no-remote-hosts",
+ ... ]
+ ... cmd.extend(arguments)
... prober = subprocess.Popen(
... cmd,
- ... shell=True,
... stdin=subprocess.PIPE,
... stdout=subprocess.PIPE,
... stderr=subprocess.PIPE,
@@ -811,7 +811,9 @@ probed again.
But we can override the default behaviour and tell the prober to check
all official mirrors independently if they were probed recently or not.
- >>> prober, stdout, stderr = run_prober("--content-type=cdimage --force")
+ >>> prober, stdout, stderr = run_prober(
+ ... "--content-type=cdimage", "--force"
+ ... )
>>> print(stdout)
<BLANKLINE>
>>> print(stderr)
@@ -834,7 +836,9 @@ we'll enable them again.
True
>>> cdimage_mirror.enabled = False
>>> transaction.commit()
- >>> prober, stdout, stderr = run_prober("--content-type=cdimage --force")
+ >>> prober, stdout, stderr = run_prober(
+ ... "--content-type=cdimage", "--force"
+ ... )
>>> print(stderr)
INFO Creating lockfile:
/var/lock/launchpad-distributionmirror-prober.lock
diff --git a/lib/lp/registry/doc/person-karma.rst b/lib/lp/registry/doc/person-karma.rst
index 2091cbd..aaf5ace 100644
--- a/lib/lp/registry/doc/person-karma.rst
+++ b/lib/lp/registry/doc/person-karma.rst
@@ -125,8 +125,7 @@ is updated by the foaf-update-karma-cache.py cronscript.
>>> import subprocess
>>> process = subprocess.Popen(
- ... "cronscripts/foaf-update-karma-cache.py",
- ... shell=True,
+ ... ["cronscripts/foaf-update-karma-cache.py"],
... stdin=subprocess.PIPE,
... stdout=subprocess.PIPE,
... stderr=subprocess.PIPE,
diff --git a/lib/lp/registry/doc/person-notification.rst b/lib/lp/registry/doc/person-notification.rst
index 3751d2d..7c36130 100644
--- a/lib/lp/registry/doc/person-notification.rst
+++ b/lib/lp/registry/doc/person-notification.rst
@@ -92,8 +92,7 @@ This includes notifications to teams owned by other teams.
>>> import subprocess
>>> process = subprocess.Popen(
- ... "cronscripts/send-person-notifications.py -q",
- ... shell=True,
+ ... ["cronscripts/send-person-notifications.py", "-q"],
... stdin=subprocess.PIPE,
... stdout=subprocess.PIPE,
... stderr=subprocess.PIPE,
diff --git a/lib/lp/registry/doc/standing.rst b/lib/lp/registry/doc/standing.rst
index 936a2b2..2f13ad6 100644
--- a/lib/lp/registry/doc/standing.rst
+++ b/lib/lp/registry/doc/standing.rst
@@ -248,8 +248,7 @@ standing untouched.
>>> import subprocess
>>> process = subprocess.Popen(
- ... "cronscripts/update-standing.py",
- ... shell=True,
+ ... ["cronscripts/update-standing.py"],
... stdin=subprocess.PIPE,
... stdout=subprocess.PIPE,
... stderr=subprocess.PIPE,
@@ -284,8 +283,7 @@ update-standing script bumps his standing to Good too.
>>> LaunchpadZopelessLayer.txn.commit()
>>> process = subprocess.Popen(
- ... "cronscripts/update-standing.py",
- ... shell=True,
+ ... ["cronscripts/update-standing.py"],
... stdin=subprocess.PIPE,
... stdout=subprocess.PIPE,
... stderr=subprocess.PIPE,
diff --git a/lib/lp/registry/tests/test_karmacache_updater.py b/lib/lp/registry/tests/test_karmacache_updater.py
index fc13a39..26810e2 100644
--- a/lib/lp/registry/tests/test_karmacache_updater.py
+++ b/lib/lp/registry/tests/test_karmacache_updater.py
@@ -34,8 +34,7 @@ class TestKarmaCacheUpdater(unittest.TestCase):
def _runScript(self):
process = subprocess.Popen(
- "cronscripts/foaf-update-karma-cache.py",
- shell=True,
+ ["cronscripts/foaf-update-karma-cache.py"],
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
diff --git a/lib/lp/services/doc/pidfile.rst b/lib/lp/services/doc/pidfile.rst
index 70eabb3..a6b9173 100644
--- a/lib/lp/services/doc/pidfile.rst
+++ b/lib/lp/services/doc/pidfile.rst
@@ -67,8 +67,7 @@ the correct PID is stored in it.
... int(open(pidfile_path('nuts')).read().strip() == str(os.getpid()))
... )
... '''
- >>> cmd = '%s -c "%s"' % (sys.executable, cmd)
- >>> subprocess.call(cmd, shell=True)
+ >>> subprocess.call([sys.executable, "-c", cmd])
1
Moreover, the pidfile has been removed.
diff --git a/lib/lp/services/librarianserver/tests/test_apachelogparser.py b/lib/lp/services/librarianserver/tests/test_apachelogparser.py
index 82b26b4..bfa7f7b 100644
--- a/lib/lp/services/librarianserver/tests/test_apachelogparser.py
+++ b/lib/lp/services/librarianserver/tests/test_apachelogparser.py
@@ -152,8 +152,7 @@ class TestScriptRunning(TestCase):
self.assertEqual(libraryfile_set[3].hits, 0)
process = subprocess.Popen(
- "cronscripts/parse-librarian-apache-access-logs.py",
- shell=True,
+ ["cronscripts/parse-librarian-apache-access-logs.py"],
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
diff --git a/lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py b/lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py
index c62c8b2..69337ec 100644
--- a/lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py
+++ b/lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py
@@ -116,8 +116,7 @@ class TestScriptRunning(TestCaseWithFactory):
)
process = subprocess.Popen(
- "cronscripts/parse-ppa-apache-access-logs.py",
- shell=True,
+ ["cronscripts/parse-ppa-apache-access-logs.py"],
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
diff --git a/lib/lp/translations/doc/poexport-language-pack.rst b/lib/lp/translations/doc/poexport-language-pack.rst
index d4067a3..996cccd 100644
--- a/lib/lp/translations/doc/poexport-language-pack.rst
+++ b/lib/lp/translations/doc/poexport-language-pack.rst
@@ -385,7 +385,6 @@ number of command-line arguments is wrong.
>>> def get_subprocess(command):
... return subprocess.Popen(
... command,
- ... shell=True,
... stdin=subprocess.PIPE,
... stdout=subprocess.PIPE,
... stderr=subprocess.PIPE,
@@ -393,7 +392,7 @@ number of command-line arguments is wrong.
... )
...
- >>> proc = get_subprocess("cronscripts/language-pack-exporter.py")
+ >>> proc = get_subprocess(["cronscripts/language-pack-exporter.py"])
>>> (out, err) = proc.communicate()
>>> print(err)
Traceback (most recent call last):
@@ -416,7 +415,7 @@ into the lockfilename to allow multiple exports to run concurrently for
different distribution and series combinations.
>>> proc = get_subprocess(
- ... "cronscripts/language-pack-exporter.py ubuntu hoary"
+ ... ["cronscripts/language-pack-exporter.py", "ubuntu", "hoary"]
... )
>>> (out, err) = proc.communicate()
>>> print(err)
diff --git a/test_on_merge.py b/test_on_merge.py
index 05fe1cb..8df3dc7 100755
--- a/test_on_merge.py
+++ b/test_on_merge.py
@@ -9,6 +9,7 @@ import _pythonpath # noqa: F401
import os
import select
+import shlex
import sys
import time
from signal import SIGHUP, SIGINT, SIGKILL, SIGTERM
@@ -142,20 +143,18 @@ def run_test_process():
cmd = [
"/usr/bin/xvfb-run",
"--error-file=/var/tmp/xvfb-errors.log",
- "--server-args='-screen 0 1024x768x24'",
+ "--server-args=-screen 0 1024x768x24",
os.path.join(HERE, "bin", "test"),
] + sys.argv[1:]
- command_line = " ".join(cmd)
- print("Running command:", command_line)
+ print("Running command:", " ".join(shlex.quote(arg) for arg in cmd))
# Run the test suite. Make the suite the leader of a new process group
# so that we can signal the group without signaling ourselves.
xvfb_proc = Popen(
- command_line,
+ cmd,
stdout=PIPE,
stderr=STDOUT,
preexec_fn=os.setpgrp,
- shell=True,
)
# This code is very similar to what takes place in Popen._communicate(),
diff --git a/utilities/local-latency b/utilities/local-latency
index dd33c58..65df36a 100755
--- a/utilities/local-latency
+++ b/utilities/local-latency
@@ -6,35 +6,68 @@ import sys
from script_commands import UserError, helps, run_subcommand
-def tc(command):
+def tc(*arguments):
"""Run a tc command under sudo.
- :param tc: The remainder of the command (leaving out tc).
+ :param arguments: The remainder of the command (leaving out tc).
"""
- subprocess.call("sudo tc " + command, shell=True)
+ subprocess.call(["sudo", "tc"] + arguments)
@helps(
- delay="Length of delay in miliseconds (each way).",
+ delay="Length of delay in milliseconds (each way).",
port="Port to induce delay on.",
)
def start(delay=500, port=443):
"""Add artificial latency to the lo interface on the specified port."""
- tc("qdisc add dev lo root handle 1: prio")
- tc("qdisc add dev lo parent 1:3 handle 30: netem delay %dms" % delay)
+ qdisc_add = ["qdisc", "add", "dev", "lo"]
+ tc(*qdisc_add, "root", "handle", "1:", "prio")
tc(
- "filter add dev lo protocol ip parent 1:0 prio 3 u32 match ip"
- " dport %d 0xffff flowid 1:3" % port
+ *qdisc_add,
+ "parent",
+ "1:3",
+ "handle",
+ "30:",
+ "netem",
+ "delay",
+ "%dms" % delay,
)
+ filter_add_ip = ["filter", "add", "dev", "lo", "protocol", "ip"]
tc(
- "filter add dev lo protocol ip parent 1:0 prio 3 u32 match ip"
- " sport %d 0xffff flowid 1:3" % port
+ *filter_add_ip,
+ "parent",
+ "1:0",
+ "prio",
+ "3",
+ "u32",
+ "match",
+ "ip",
+ "dport",
+ str(port),
+ "0xffff",
+ "flowid",
+ "1:3",
+ )
+ tc(
+ *filter_add_ip,
+ "parent",
+ "1:0",
+ "prio",
+ "3",
+ "u32",
+ "match",
+ "ip",
+ "sport",
+ str(port),
+ "0xffff",
+ "flowid",
+ "1:3",
)
def stop():
"""Remove latency from the lo."""
- tc("qdisc del dev lo root")
+ tc("qdisc", "del", "dev", "lo", "root")
subcommands = {
