Launchpad itself

Merge ~cjwatson/launchpad:avoid-subprocess-shell into launchpad:master

  1. Git
  2. lp:~cjwatson/launchpad
  3. avoid-subprocess-shell
  4. Merge into master
Proposed by Colin Watson
Status: Merged
Approved by: Colin Watson
Approved revision: acb71e6db63ce6db92b02466bf78f3a39fbdecf4
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/launchpad:avoid-subprocess-shell
Merge into: launchpad:master
Diff against target: 389 lines (+76/-55)
17 files modified
lib/lp/answers/doc/expiration.rst (+1/-2)
lib/lp/bugs/doc/bugnotification-sending.rst (+1/-2)
lib/lp/bugs/doc/bugtask-expiration.rst (+1/-2)
lib/lp/bugs/doc/externalbugtracker-debbugs.rst (+1/-2)
lib/lp/bugs/model/tests/test_bugtask.py (+1/-2)
lib/lp/registry/doc/convert-person-to-team.rst (+1/-2)
lib/lp/registry/doc/distribution-mirror.rst (+12/-8)
lib/lp/registry/doc/person-karma.rst (+1/-2)
lib/lp/registry/doc/person-notification.rst (+1/-2)
lib/lp/registry/doc/standing.rst (+2/-4)
lib/lp/registry/tests/test_karmacache_updater.py (+1/-2)
lib/lp/services/doc/pidfile.rst (+1/-2)
lib/lp/services/librarianserver/tests/test_apachelogparser.py (+1/-2)
lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py (+1/-2)
lib/lp/translations/doc/poexport-language-pack.rst (+2/-3)
test_on_merge.py (+4/-5)
utilities/local-latency (+44/-11)
Reviewer Review Type Date Requested Status
Guruprasad Approve
Review via email: mp+445865@code.launchpad.net

Commit message

Avoid calling subprocess with shell=True

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Guruprasad (lgp171188) wrote :

LGTM 👍

review: Approve
Revision history for this message
Colin Watson (cjwatson) :
Revision history for this message
Guruprasad (lgp171188) :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/answers/doc/expiration.rst b/lib/lp/answers/doc/expiration.rst
2index 18b467e..84ac916 100644
3--- a/lib/lp/answers/doc/expiration.rst
4+++ b/lib/lp/answers/doc/expiration.rst
5@@ -144,8 +144,7 @@ somebody are subject to expiration.
6 # Run the script.
7 >>> import subprocess
8 >>> process = subprocess.Popen(
9- ... "cronscripts/expire-questions.py",
10- ... shell=True,
11+ ... ["cronscripts/expire-questions.py"],
12 ... stdin=subprocess.PIPE,
13 ... stdout=subprocess.PIPE,
14 ... stderr=subprocess.PIPE,
15diff --git a/lib/lp/bugs/doc/bugnotification-sending.rst b/lib/lp/bugs/doc/bugnotification-sending.rst
16index abf8f70..d254cc3 100644
17--- a/lib/lp/bugs/doc/bugnotification-sending.rst
18+++ b/lib/lp/bugs/doc/bugnotification-sending.rst
19@@ -661,8 +661,7 @@ makes it write out the emails it sends.
20
21 >>> import subprocess
22 >>> process = subprocess.Popen(
23- ... "cronscripts/send-bug-notifications.py -v",
24- ... shell=True,
25+ ... ["cronscripts/send-bug-notifications.py", "-v"],
26 ... stdin=subprocess.PIPE,
27 ... stdout=subprocess.PIPE,
28 ... stderr=subprocess.PIPE,
29diff --git a/lib/lp/bugs/doc/bugtask-expiration.rst b/lib/lp/bugs/doc/bugtask-expiration.rst
30index 8087a48..abaa762 100644
31--- a/lib/lp/bugs/doc/bugtask-expiration.rst
32+++ b/lib/lp/bugs/doc/bugtask-expiration.rst
33@@ -561,8 +561,7 @@ config.malone.expiration_dbuser.
34
35 >>> import subprocess
36 >>> process = subprocess.Popen(
37- ... "cronscripts/expire-bugtasks.py",
38- ... shell=True,
39+ ... ["cronscripts/expire-bugtasks.py"],
40 ... stdin=subprocess.PIPE,
41 ... stdout=subprocess.PIPE,
42 ... stderr=subprocess.PIPE,
43diff --git a/lib/lp/bugs/doc/externalbugtracker-debbugs.rst b/lib/lp/bugs/doc/externalbugtracker-debbugs.rst
44index d35b5e3..34861ea 100644
45--- a/lib/lp/bugs/doc/externalbugtracker-debbugs.rst
46+++ b/lib/lp/bugs/doc/externalbugtracker-debbugs.rst
47@@ -706,8 +706,7 @@ tracker.
48
49 >>> import subprocess
50 >>> process = subprocess.Popen(
51- ... "scripts/import-debian-bugs.py 237001 322535",
52- ... shell=True,
53+ ... ["scripts/import-debian-bugs.py", "237001", "322535"],
54 ... stdin=subprocess.PIPE,
55 ... stdout=subprocess.PIPE,
56 ... stderr=subprocess.PIPE,
57diff --git a/lib/lp/bugs/model/tests/test_bugtask.py b/lib/lp/bugs/model/tests/test_bugtask.py
58index c3eb248..e26983b 100644
59--- a/lib/lp/bugs/model/tests/test_bugtask.py
60+++ b/lib/lp/bugs/model/tests/test_bugtask.py
61@@ -3871,8 +3871,7 @@ class TestTargetNameCache(TestCase):
62 transaction.commit()
63
64 process = subprocess.Popen(
65- "cronscripts/update-bugtask-targetnamecaches.py",
66- shell=True,
67+ ["cronscripts/update-bugtask-targetnamecaches.py"],
68 stdin=subprocess.PIPE,
69 stdout=subprocess.PIPE,
70 stderr=subprocess.PIPE,
71diff --git a/lib/lp/registry/doc/convert-person-to-team.rst b/lib/lp/registry/doc/convert-person-to-team.rst
72index be3ef31..a64e685 100644
73--- a/lib/lp/registry/doc/convert-person-to-team.rst
74+++ b/lib/lp/registry/doc/convert-person-to-team.rst
75@@ -16,8 +16,7 @@ team and the name of the team owner as arguments.
76
77 >>> from subprocess import Popen, PIPE
78 >>> process = Popen(
79- ... "scripts/convert-person-to-team.py -q matsubara mark",
80- ... shell=True,
81+ ... ["scripts/convert-person-to-team.py", "-q", "matsubara", "mark"],
82 ... stdin=PIPE,
83 ... stdout=PIPE,
84 ... stderr=PIPE,
85diff --git a/lib/lp/registry/doc/distribution-mirror.rst b/lib/lp/registry/doc/distribution-mirror.rst
86index a3bb64d..db50e08 100644
87--- a/lib/lp/registry/doc/distribution-mirror.rst
88+++ b/lib/lp/registry/doc/distribution-mirror.rst
89@@ -741,14 +741,14 @@ First we need to run the http server that's going to answer our requests.
90 >>> http_server.setUp()
91
92 >>> import subprocess
93- >>> def run_prober(arguments):
94- ... cmd = (
95- ... "cronscripts/distributionmirror-prober.py %s "
96- ... "--no-remote-hosts" % arguments
97- ... )
98+ >>> def run_prober(*arguments):
99+ ... cmd = [
100+ ... "cronscripts/distributionmirror-prober.py",
101+ ... "--no-remote-hosts",
102+ ... ]
103+ ... cmd.extend(arguments)
104 ... prober = subprocess.Popen(
105 ... cmd,
106- ... shell=True,
107 ... stdin=subprocess.PIPE,
108 ... stdout=subprocess.PIPE,
109 ... stderr=subprocess.PIPE,
110@@ -811,7 +811,9 @@ probed again.
111 But we can override the default behaviour and tell the prober to check
112 all official mirrors independently if they were probed recently or not.
113
114- >>> prober, stdout, stderr = run_prober("--content-type=cdimage --force")
115+ >>> prober, stdout, stderr = run_prober(
116+ ... "--content-type=cdimage", "--force"
117+ ... )
118 >>> print(stdout)
119 <BLANKLINE>
120 >>> print(stderr)
121@@ -834,7 +836,9 @@ we'll enable them again.
122 True
123 >>> cdimage_mirror.enabled = False
124 >>> transaction.commit()
125- >>> prober, stdout, stderr = run_prober("--content-type=cdimage --force")
126+ >>> prober, stdout, stderr = run_prober(
127+ ... "--content-type=cdimage", "--force"
128+ ... )
129 >>> print(stderr)
130 INFO Creating lockfile:
131 /var/lock/launchpad-distributionmirror-prober.lock
132diff --git a/lib/lp/registry/doc/person-karma.rst b/lib/lp/registry/doc/person-karma.rst
133index 2091cbd..aaf5ace 100644
134--- a/lib/lp/registry/doc/person-karma.rst
135+++ b/lib/lp/registry/doc/person-karma.rst
136@@ -125,8 +125,7 @@ is updated by the foaf-update-karma-cache.py cronscript.
137
138 >>> import subprocess
139 >>> process = subprocess.Popen(
140- ... "cronscripts/foaf-update-karma-cache.py",
141- ... shell=True,
142+ ... ["cronscripts/foaf-update-karma-cache.py"],
143 ... stdin=subprocess.PIPE,
144 ... stdout=subprocess.PIPE,
145 ... stderr=subprocess.PIPE,
146diff --git a/lib/lp/registry/doc/person-notification.rst b/lib/lp/registry/doc/person-notification.rst
147index 3751d2d..7c36130 100644
148--- a/lib/lp/registry/doc/person-notification.rst
149+++ b/lib/lp/registry/doc/person-notification.rst
150@@ -92,8 +92,7 @@ This includes notifications to teams owned by other teams.
151
152 >>> import subprocess
153 >>> process = subprocess.Popen(
154- ... "cronscripts/send-person-notifications.py -q",
155- ... shell=True,
156+ ... ["cronscripts/send-person-notifications.py", "-q"],
157 ... stdin=subprocess.PIPE,
158 ... stdout=subprocess.PIPE,
159 ... stderr=subprocess.PIPE,
160diff --git a/lib/lp/registry/doc/standing.rst b/lib/lp/registry/doc/standing.rst
161index 936a2b2..2f13ad6 100644
162--- a/lib/lp/registry/doc/standing.rst
163+++ b/lib/lp/registry/doc/standing.rst
164@@ -248,8 +248,7 @@ standing untouched.
165
166 >>> import subprocess
167 >>> process = subprocess.Popen(
168- ... "cronscripts/update-standing.py",
169- ... shell=True,
170+ ... ["cronscripts/update-standing.py"],
171 ... stdin=subprocess.PIPE,
172 ... stdout=subprocess.PIPE,
173 ... stderr=subprocess.PIPE,
174@@ -284,8 +283,7 @@ update-standing script bumps his standing to Good too.
175 >>> LaunchpadZopelessLayer.txn.commit()
176
177 >>> process = subprocess.Popen(
178- ... "cronscripts/update-standing.py",
179- ... shell=True,
180+ ... ["cronscripts/update-standing.py"],
181 ... stdin=subprocess.PIPE,
182 ... stdout=subprocess.PIPE,
183 ... stderr=subprocess.PIPE,
184diff --git a/lib/lp/registry/tests/test_karmacache_updater.py b/lib/lp/registry/tests/test_karmacache_updater.py
185index fc13a39..26810e2 100644
186--- a/lib/lp/registry/tests/test_karmacache_updater.py
187+++ b/lib/lp/registry/tests/test_karmacache_updater.py
188@@ -34,8 +34,7 @@ class TestKarmaCacheUpdater(unittest.TestCase):
189
190 def _runScript(self):
191 process = subprocess.Popen(
192- "cronscripts/foaf-update-karma-cache.py",
193- shell=True,
194+ ["cronscripts/foaf-update-karma-cache.py"],
195 stdin=subprocess.PIPE,
196 stdout=subprocess.PIPE,
197 stderr=subprocess.PIPE,
198diff --git a/lib/lp/services/doc/pidfile.rst b/lib/lp/services/doc/pidfile.rst
199index 70eabb3..a6b9173 100644
200--- a/lib/lp/services/doc/pidfile.rst
201+++ b/lib/lp/services/doc/pidfile.rst
202@@ -67,8 +67,7 @@ the correct PID is stored in it.
203 ... int(open(pidfile_path('nuts')).read().strip() == str(os.getpid()))
204 ... )
205 ... '''
206- >>> cmd = '%s -c "%s"' % (sys.executable, cmd)
207- >>> subprocess.call(cmd, shell=True)
208+ >>> subprocess.call([sys.executable, "-c", cmd])
209 1
210
211 Moreover, the pidfile has been removed.
212diff --git a/lib/lp/services/librarianserver/tests/test_apachelogparser.py b/lib/lp/services/librarianserver/tests/test_apachelogparser.py
213index 82b26b4..bfa7f7b 100644
214--- a/lib/lp/services/librarianserver/tests/test_apachelogparser.py
215+++ b/lib/lp/services/librarianserver/tests/test_apachelogparser.py
216@@ -152,8 +152,7 @@ class TestScriptRunning(TestCase):
217 self.assertEqual(libraryfile_set[3].hits, 0)
218
219 process = subprocess.Popen(
220- "cronscripts/parse-librarian-apache-access-logs.py",
221- shell=True,
222+ ["cronscripts/parse-librarian-apache-access-logs.py"],
223 stdin=subprocess.PIPE,
224 stdout=subprocess.PIPE,
225 stderr=subprocess.PIPE,
226diff --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
227index c62c8b2..69337ec 100644
228--- a/lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py
229+++ b/lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py
230@@ -116,8 +116,7 @@ class TestScriptRunning(TestCaseWithFactory):
231 )
232
233 process = subprocess.Popen(
234- "cronscripts/parse-ppa-apache-access-logs.py",
235- shell=True,
236+ ["cronscripts/parse-ppa-apache-access-logs.py"],
237 stdin=subprocess.PIPE,
238 stdout=subprocess.PIPE,
239 stderr=subprocess.PIPE,
240diff --git a/lib/lp/translations/doc/poexport-language-pack.rst b/lib/lp/translations/doc/poexport-language-pack.rst
241index d4067a3..996cccd 100644
242--- a/lib/lp/translations/doc/poexport-language-pack.rst
243+++ b/lib/lp/translations/doc/poexport-language-pack.rst
244@@ -385,7 +385,6 @@ number of command-line arguments is wrong.
245 >>> def get_subprocess(command):
246 ... return subprocess.Popen(
247 ... command,
248- ... shell=True,
249 ... stdin=subprocess.PIPE,
250 ... stdout=subprocess.PIPE,
251 ... stderr=subprocess.PIPE,
252@@ -393,7 +392,7 @@ number of command-line arguments is wrong.
253 ... )
254 ...
255
256- >>> proc = get_subprocess("cronscripts/language-pack-exporter.py")
257+ >>> proc = get_subprocess(["cronscripts/language-pack-exporter.py"])
258 >>> (out, err) = proc.communicate()
259 >>> print(err)
260 Traceback (most recent call last):
261@@ -416,7 +415,7 @@ into the lockfilename to allow multiple exports to run concurrently for
262 different distribution and series combinations.
263
264 >>> proc = get_subprocess(
265- ... "cronscripts/language-pack-exporter.py ubuntu hoary"
266+ ... ["cronscripts/language-pack-exporter.py", "ubuntu", "hoary"]
267 ... )
268 >>> (out, err) = proc.communicate()
269 >>> print(err)
270diff --git a/test_on_merge.py b/test_on_merge.py
271index 05fe1cb..8df3dc7 100755
272--- a/test_on_merge.py
273+++ b/test_on_merge.py
274@@ -9,6 +9,7 @@ import _pythonpath # noqa: F401
275
276 import os
277 import select
278+import shlex
279 import sys
280 import time
281 from signal import SIGHUP, SIGINT, SIGKILL, SIGTERM
282@@ -142,20 +143,18 @@ def run_test_process():
283 cmd = [
284 "/usr/bin/xvfb-run",
285 "--error-file=/var/tmp/xvfb-errors.log",
286- "--server-args='-screen 0 1024x768x24'",
287+ "--server-args=-screen 0 1024x768x24",
288 os.path.join(HERE, "bin", "test"),
289 ] + sys.argv[1:]
290- command_line = " ".join(cmd)
291- print("Running command:", command_line)
292+ print("Running command:", " ".join(shlex.quote(arg) for arg in cmd))
293
294 # Run the test suite. Make the suite the leader of a new process group
295 # so that we can signal the group without signaling ourselves.
296 xvfb_proc = Popen(
297- command_line,
298+ cmd,
299 stdout=PIPE,
300 stderr=STDOUT,
301 preexec_fn=os.setpgrp,
302- shell=True,
303 )
304
305 # This code is very similar to what takes place in Popen._communicate(),
306diff --git a/utilities/local-latency b/utilities/local-latency
307index dd33c58..65df36a 100755
308--- a/utilities/local-latency
309+++ b/utilities/local-latency
310@@ -6,35 +6,68 @@ import sys
311 from script_commands import UserError, helps, run_subcommand
312
313
314-def tc(command):
315+def tc(*arguments):
316 """Run a tc command under sudo.
317
318- :param tc: The remainder of the command (leaving out tc).
319+ :param arguments: The remainder of the command (leaving out tc).
320 """
321- subprocess.call("sudo tc " + command, shell=True)
322+ subprocess.call(["sudo", "tc"] + arguments)
323
324
325 @helps(
326- delay="Length of delay in miliseconds (each way).",
327+ delay="Length of delay in milliseconds (each way).",
328 port="Port to induce delay on.",
329 )
330 def start(delay=500, port=443):
331 """Add artificial latency to the lo interface on the specified port."""
332- tc("qdisc add dev lo root handle 1: prio")
333- tc("qdisc add dev lo parent 1:3 handle 30: netem delay %dms" % delay)
334+ qdisc_add = ["qdisc", "add", "dev", "lo"]
335+ tc(*qdisc_add, "root", "handle", "1:", "prio")
336 tc(
337- "filter add dev lo protocol ip parent 1:0 prio 3 u32 match ip"
338- " dport %d 0xffff flowid 1:3" % port
339+ *qdisc_add,
340+ "parent",
341+ "1:3",
342+ "handle",
343+ "30:",
344+ "netem",
345+ "delay",
346+ "%dms" % delay,
347 )
348+ filter_add_ip = ["filter", "add", "dev", "lo", "protocol", "ip"]
349 tc(
350- "filter add dev lo protocol ip parent 1:0 prio 3 u32 match ip"
351- " sport %d 0xffff flowid 1:3" % port
352+ *filter_add_ip,
353+ "parent",
354+ "1:0",
355+ "prio",
356+ "3",
357+ "u32",
358+ "match",
359+ "ip",
360+ "dport",
361+ str(port),
362+ "0xffff",
363+ "flowid",
364+ "1:3",
365+ )
366+ tc(
367+ *filter_add_ip,
368+ "parent",
369+ "1:0",
370+ "prio",
371+ "3",
372+ "u32",
373+ "match",
374+ "ip",
375+ "sport",
376+ str(port),
377+ "0xffff",
378+ "flowid",
379+ "1:3",
380 )
381
382
383 def stop():
384 """Remove latency from the lo."""
385- tc("qdisc del dev lo root")
386+ tc("qdisc", "del", "dev", "lo", "root")
387
388
389 subcommands = {

Subscribers

People subscribed via source and target branches

to status/vote changes: