Re: [PATCH 1/4] Remove all uses of subprocess.call(shell=True)

[Claudius Heine <claudius.heine.ext@siemens.com>]

Hi Harald,

On 12/02/2019 10.20, Harald Seiler wrote:
> Using shell-expansion in subprocess calls has a lot of nasty
> side-effects that are hard to debug should one ever surface.
> This commit changes all uses of subprocess to
> 
> A) not use shell=True by giving a list of args instead of
>     a command-line.
> B) ensure return-codes are handled appropriately.
> 
> Furthermore, in some places the subprocess usage was changed to
> be more idiomatic.
> 
> Co-authored-by: Claudius Heine <ch@denx.de>
> Signed-off-by: Harald Seiler <hws@denx.de>
> ---
>   meta/classes/base.bbclass                  | 10 ++++++----
>   meta/classes/image.bbclass                 |  9 +++++----
>   meta/classes/isar-bootstrap-helper.bbclass |  9 ++++-----
>   meta/classes/isar-events.bbclass           | 13 +++++++------
>   meta/classes/patch.bbclass                 | 16 ++++++++++++----
>   5 files changed, 34 insertions(+), 23 deletions(-)
> 
> diff --git a/meta/classes/base.bbclass b/meta/classes/base.bbclass
> index 6ca93bf..6fcf452 100644
> --- a/meta/classes/base.bbclass
> +++ b/meta/classes/base.bbclass
> @@ -170,15 +170,17 @@ addtask clean
>   do_clean[nostamp] = "1"
>   python do_clean() {
>       import subprocess
> +    import glob
>   
>       for f in (d.getVar('CLEANFUNCS', True) or '').split():
>           bb.build.exec_func(f, d)
>   
> -    dir = d.expand("${WORKDIR}")
> -    subprocess.call('sudo rm -rf ' + dir, shell=True)
> +    workdir = d.expand("${WORKDIR}")
> +    subprocess.check_call(["sudo", "rm", "-rf", workdir])
>   
> -    dir = "%s.*" % bb.data.expand(d.getVar('STAMP', False), d)
> -    subprocess.call('sudo rm -rf ' + dir, shell=True)
> +    stamppath = bb.data.expand(d.getVar('STAMP', False), d)
> +    stampdirs = glob.glob(stamppath + ".*")
> +    subprocess.check_call(["sudo", "rm", "-rf"] + stampdirs)
>   }
>   
>   # Derived from OpenEmbedded Core: meta/classes/base.bbclass
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index be00817..930c245 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -27,16 +27,17 @@ def get_image_name(d, name_link):
>           full = d.getVar("IMAGE_FULLNAME", True) + "." + base
>           return [base, full]
>       if os.path.islink(path_link):
> -        return get_image_name(d, os.path.relpath(os.path.realpath(path_link),
> -                                                 '/'))
> +        return get_image_name(d, os.path.relpath(os.path.realpath(path_link), '/'))
> +
>       return ["", ""]
>   
>   def get_rootfs_size(d):
>       import subprocess
>       rootfs_extra = int(d.getVar("ROOTFS_EXTRA", True))
>   
> -    output = subprocess.check_output(['sudo', 'du', '-s', '--block-size=1k',
> -                                      d.getVar("IMAGE_ROOTFS", True)])
> +    output = subprocess.check_output(
> +        ['sudo', 'du', '-s', '--block-size=1k', d.getVar("IMAGE_ROOTFS", True)]
> +    )
>       base_size = int(output.split()[0])
>   
>       return base_size + rootfs_extra * 1024
> diff --git a/meta/classes/isar-bootstrap-helper.bbclass b/meta/classes/isar-bootstrap-helper.bbclass
> index d780b85..3e11098 100644
> --- a/meta/classes/isar-bootstrap-helper.bbclass
> +++ b/meta/classes/isar-bootstrap-helper.bbclass
> @@ -9,11 +9,10 @@ IMAGE_TRANSIENT_PACKAGES ??= ""
>   
>   def get_deb_host_arch():
>       import subprocess
> -    host_arch = subprocess.Popen("dpkg --print-architecture",
> -                                 shell=True,
> -                                 env=os.environ,
> -                                 stdout=subprocess.PIPE
> -                                ).stdout.read().decode('utf-8').strip()
> +    host_arch = subprocess.check_output(
> +        ["dpkg", "--print-architecture"],
> +        encoding="utf-8",

utf-8 should be the default encoding, or is it? I would remove that if 
it is. Other check_output calls don't have that.

> +    ).strip()
>       return host_arch
>   
>   #Debian Distribution for SDK host
> diff --git a/meta/classes/isar-events.bbclass b/meta/classes/isar-events.bbclass
> index c4a6149..a178d05 100644
> --- a/meta/classes/isar-events.bbclass
> +++ b/meta/classes/isar-events.bbclass
> @@ -16,13 +16,14 @@ python isar_handler() {
>   
>       basepath = tmpdir + '/work/'
>   
> -    with open(os.devnull, 'w') as devnull:
> -        with open('/proc/mounts', 'rU') as f:
> -            lines = f.readlines()
> -        for line in lines:
> +    with open('/proc/mounts') as f:
> +        for line in f.readlines():
>               if basepath in line:
> -                subprocess.call('sudo umount -l ' + line.split()[1],
> -                                stdout=devnull, stderr=devnull, shell=True)
> +                subprocess.call(
> +                    ["sudo", "umount", "-l", line.split()[1]],
> +                    stdout=subprocess.DEVNULL,
> +                    stderr=subprocess.DEVNULL,
> +                )
>   }
>   
>   isar_handler[eventmask] = "bb.runqueue.runQueueExitWait bb.event.BuildCompleted"
> diff --git a/meta/classes/patch.bbclass b/meta/classes/patch.bbclass
> index 0bc449f..9d4441d 100644
> --- a/meta/classes/patch.bbclass
> +++ b/meta/classes/patch.bbclass
> @@ -21,10 +21,18 @@ python do_patch() {
>   
>               striplevel = fetcher.ud[src_uri].parm.get("striplevel") or "1"
>   
> -            cmd = "patch --no-backup-if-mismatch -p " + striplevel + \
> -                  " --directory " + src_dir + " --input " + workdir + basename
> -            bb.note(cmd)
> -            if subprocess.call(cmd, shell=True) != 0:
> +            cmd = [
> +                "patch",
> +                "--no-backup-if-mismatch",
> +                "-p",
> +                striplevel,
> +                "--directory",
> +                src_dir,
> +                "--input",
> +                workdir + basename,
> +            ]

IMO formatting could be improved there. I don't know about you, but I 
can read:

             cmd = [
                 "patch",
                 "--no-backup-if-mismatch",
                 "-p", striplevel,
                 "--directory", src_dir,
                 "--input", workdir + basename,
             ]

better. :) But that is just opinion, so if you differ I will not insist 
on it.

Otherwise great work!

Claudius

> +            bb.note(" ".join(cmd))
> +            if subprocess.call(cmd) != 0:
>                   bb.fatal("patching failed")
>           except bb.fetch2.BBFetchException as e:
>               raise bb.build.FuncFailed(e)
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch@denx.de