Merge lp:~evfool/software-properties/lp1060192 into lp:software-properties
Proposed by
Robert Roth
Status: | Merged |
---|---|
Merged at revision: | 830 |
Proposed branch: | lp:~evfool/software-properties/lp1060192 |
Merge into: | lp:software-properties |
Diff against target: |
14 lines (+2/-2) 1 file modified
softwareproperties/MirrorTest.py (+2/-2) |
To merge this branch: | bzr merge lp:~evfool/software-properties/lp1060192 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ubuntu Core Development Team | Pending | ||
Review via email: mp+148210@code.launchpad.net |
Commit message
- Do not use Shell=True unnecessarily in subprocess.Popen (bug #1060192)
Description of the change
As the python subprocess Popen documentation states: "the use of shell=True is strongly discouraged in cases where the command string is constructed from external input".
We construct the command to execute from mostly well-known parameters, unless the hostname of the mirror to test, which comes from /usr/share/
As bug #1060192 states, it does not really make a difference in this context whether we use shell=True or False, but maybe it is safer to use shell=False.
To post a comment you must log in.
On Wed, Feb 13, 2013 at 03:24:18PM -0000, Robert Roth wrote:
> Robert Roth has proposed merging lp:~evfool/software-properties/lp1060192 into lp:software-properties.
>
> Commit message:
> - Do not use Shell=True unnecessarily in subprocess.Popen (bug #1060192)
[..]
Great! Thanks for working on this!
> self.parent. report_ action( "Pinging %s..." % host) Popen(" ping -q -c 2 -W 1 -i 0.5 %s" % host, subprocess. PIPE, Popen([ "ping", "-q", "-c 2", "-W 1", "-i 0.5", host], subprocess. PIPE, subprocess. STDOUT, newlines= True).stdout
> - commando = subprocess.
> - shell=True, stdout=
> + commando = subprocess.
> + stdout=
> stderr=
> universal_
> while True:
You can also go one step further and do ["ping", "-c", "2", "-W",
"1"...].
But ping is fine with both. Thanks again for getting rid of the
shell=True.
Cheers,
Michael