Use shell=False where possible with utils.runcmd()

It's best practice for security reasons to use shell=False and pass
command line arguments as a list; it also avoids some pain with
escaping, so let's use it everywhere we can (in fact we're only left
with one place in layerindex/tasks.py where we now pass shell=True).

Signed-off-by: Paul Eggleton <paul.eggleton@linux.intel.com>

layerindex/tasks.py

@@ -50,7 +50,7 @@ def run_update_command(self, branch_name, update_command):
     retcode = 0
     erroutput = None
     try:
-        output = utils.runcmd(update_command, os.path.dirname(os.path.dirname(__file__)), outfile=logfile)
+        output = utils.runcmd(update_command, os.path.dirname(os.path.dirname(__file__)), outfile=logfile, shell=True)
     except subprocess.CalledProcessError as e:
         output = e.output
         erroutput = output

layerindex/tools/import_layer.py

@@ -302,9 +302,9 @@ def main():
             out = None
             try:
                 if not os.path.exists(repodir):
-                    out = utils.runcmd("git clone %s %s" % (layer.vcs_url, urldir), fetchdir, logger=logger)
+                    out = utils.runcmd(['git', 'clone', layer.vcs_url, urldir], fetchdir, logger=logger)
                 else:
-                    out = utils.runcmd("git fetch", repodir, logger=logger)
+                    out = utils.runcmd(['git', 'fetch'], repodir, logger=logger)
             except Exception as e:
                 logger.error("Fetch failed: %s" % str(e))
                 sys.exit(1)
@@ -313,10 +313,10 @@ def main():
             if (options.actual_branch):
                 actual_branch = options.actual_branch
             try:
-                out = utils.runcmd("git checkout origin/%s" % actual_branch, repodir, logger=logger)
+                out = utils.runcmd(['git', 'checkout', 'origin/%s' % actual_branch], repodir, logger=logger)
             except subprocess.CalledProcessError:
                 actual_branch = None
-                branches = utils.runcmd("git branch -r", repodir, logger=logger)
+                branches = utils.runcmd(['git', 'branch', '-r'], repodir, logger=logger)
                 for line in branches.splitlines():
                     if 'origin/HEAD ->' in line:
                         actual_branch = line.split('-> origin/')[-1]
@@ -324,7 +324,7 @@ def main():
                 if not actual_branch:
                     logger.error("Repository has no master branch nor origin/HEAD")
                     sys.exit(1)
-                out = utils.runcmd("git checkout origin/%s" % actual_branch, repodir, logger=logger)
+                out = utils.runcmd(['git', 'checkout', 'origin/%s' % actual_branch], repodir, logger=logger)
 
             layer_paths = []
             if options.subdir:

layerindex/tools/import_project.py

@@ -77,7 +77,7 @@ class ImportProject:
     def add_layer(self, layer):
         self.logger.debug("Processing layer %s" % layer)
         try:
-            git_dir = utils.runcmd("git rev-parse --show-toplevel", destdir=layer, logger=self.logger)
+            git_dir = utils.runcmd(['git', 'rev-parse', '--show-toplevel'], destdir=layer, logger=self.logger)
         except Exception as e:
             self.logger.error("Cannot get root dir for layer %s: %s - Skipping." % (layer, str(e)))
             return 1
@@ -93,20 +93,20 @@ class ImportProject:
         layer_name = self.get_layer_name(layer)
 
         for i in [1, 2, 3]:
-            remote = utils.runcmd("git remote", destdir=git_dir, logger=self.logger)
+            remote = utils.runcmd(['git', 'remote'], destdir=git_dir, logger=self.logger)
             if not remote:
                 self.logger.warning("Cannot find remote git for %s" % layer_name)
                 return 1
 
             try:
-                git_url = utils.runcmd("git config --get remote.%s.url" % remote, destdir=git_dir, logger=self.logger)
+                git_url = utils.runcmd(['git', 'config', '--get', 'remote.%s.url' % remote], destdir=git_dir, logger=self.logger)
             except Exception as e:
                 self.logger.info("Cannot get remote.%s.url for git dir %s: %s" % (remote, git_dir, str(e)))
 
             if not os.path.exists(git_url):
                 # Assume this is remote.
                 self.logger.debug("Found git url = %s" % git_url)
-                remote_branch = utils.runcmd( "git rev-parse --abbrev-ref --symbolic-full-name @\{u\}", destdir=git_dir, logger=self.logger)
+                remote_branch = utils.runcmd(['git', 'rev-parse', '--abbrev-ref', '--symbolic-full-name', '@{u}'], destdir=git_dir, logger=self.logger)
                 if remote_branch.startswith(remote):
                     actual_branch = remote_branch[len(remote) + 1:]
                 break

layerindex/update.py

@@ -103,9 +103,9 @@ def fetch_repo(vcs_url, repodir, urldir, fetchdir, layer_name):
     logger.info("Fetching remote repository %s" % vcs_url)
     try:
         if not os.path.exists(repodir):
-            utils.runcmd(['git', 'clone', vcs_url, urldir], fetchdir, logger=logger, printerr=False, shell=False)
+            utils.runcmd(['git', 'clone', vcs_url, urldir], fetchdir, logger=logger, printerr=False)
         else:
-            utils.runcmd("git fetch -p", repodir, logger=logger, printerr=False)
+            utils.runcmd(['git', 'fetch', '-p'], repodir, logger=logger, printerr=False)
         return (vcs_url, None)
     except subprocess.CalledProcessError as e:
         logger.error("Fetch of layer %s failed: %s" % (layer_name, e.output))

layerindex/utils.py

@@ -231,19 +231,19 @@ def checkout_repo(repodir, commit, logger, force=False):
             # git clone <url>
             # warning: remote HEAD refers to nonexistent ref, unable to checkout.
             # So check and avoid that
-            currentref = runcmd("git rev-parse HEAD", repodir, logger=logger).strip()
+            currentref = runcmd(['git', 'rev-parse', 'HEAD'], repodir, logger=logger).strip()
         except Exception as esc:
             logger.warn(esc)
             currentref = ''
     if currentref != commit:
         # Reset in case there are added but uncommitted changes
-        runcmd("git reset --hard", repodir, logger=logger)
+        runcmd(['git', 'reset', '--hard'], repodir, logger=logger)
         # Drop any untracked files in case these cause problems (either because
         # they will exist in the revision we're checking out, or will otherwise
         # interfere with operation, e.g. stale pyc files)
-        runcmd("git clean -qdfx", repodir, logger=logger)
+        runcmd(['git', 'clean', '-qdfx'], repodir, logger=logger)
         # Now check out the revision
-        runcmd(['git', 'checkout', commit], repodir, logger=logger, shell=False)
+        runcmd(['git', 'checkout', commit], repodir, logger=logger)
 
 def checkout_layer_branch(layerbranch, repodir, logger=None):
     branchname = layerbranch.get_checkout_branch()
@@ -289,7 +289,7 @@ def parse_layer_conf(layerdir, data, logger=None):
     data.expandVarref('LAYERDIR')
 
 child_pid = 0
-def runcmd(cmd, destdir=None, printerr=True, outfile=None, logger=None, shell=True):
+def runcmd(cmd, destdir=None, printerr=True, outfile=None, logger=None, shell=False):
     """
         execute command, raise CalledProcessError if fail
         return output if succeed

layerindex/views.py

@@ -277,7 +277,7 @@ def bulk_change_patch_view(request, pk):
     # FIXME this couples the web server and machine running the update script together,
     # but given that it's a separate script the way is open to decouple them in future
     try:
-        ret = utils.runcmd('%s bulkchange.py %d %s' % (sys.executable, int(pk), settings.TEMP_BASE_DIR), os.path.dirname(__file__))
+        ret = utils.runcmd([sys.executable, 'bulkchange.py', str(int(pk)), settings.TEMP_BASE_DIR], os.path.dirname(__file__), shell=False)
         if ret:
             fn = ret.splitlines()[-1]
             if os.path.exists(fn):

rrs/tools/rrs_maintainer_history.py

@@ -78,8 +78,8 @@ def maintainers_inc_history(options, logger, maintplan, layerbranch, repodir, la
 
     logger.debug('Checking maintainers.inc history for %s' % layerbranch)
 
-    commits = utils.runcmd("git log --format='%%H' --reverse --date=rfc origin/master %s"
+    commits = utils.runcmd(['git', 'log', '--format=%H', '--reverse', '--date=rfc', 'origin/master',
-                        % os.path.join(layerbranch.vcs_subdir, MAINTAINERS_INCLUDE_PATH),
+                        os.path.join(layerbranch.vcs_subdir, MAINTAINERS_INCLUDE_PATH)],
                         repodir, logger=logger)
 
     no_maintainer, _ = Maintainer.objects.get_or_create(name='No maintainer')
@@ -93,7 +93,7 @@ def maintainers_inc_history(options, logger, maintplan, layerbranch, repodir, la
                 logger.debug("Analysing commit %s ..." % (commit))
 
                 (author_name, author_email, date, title) = \
-                    get_commit_info(utils.runcmd("git show " + commit, repodir,
+                    get_commit_info(utils.runcmd(['git', 'show', commit], repodir,
                         logger=logger), logger)
 
                 author = Maintainer.create_or_update(author_name, author_email)
@@ -101,7 +101,7 @@ def maintainers_inc_history(options, logger, maintplan, layerbranch, repodir, la
                         sha1=commit, layerbranch=layerbranch)
                 rms.save()
 
-                utils.runcmd("git checkout %s -f" % commit,
+                utils.runcmd(['git', 'checkout', commit, '-f'],
                         repodir, logger=logger)
 
                 with open(maintainers_full_path, 'r') as f:

rrs/tools/rrs_upgrade_history.py

@@ -54,7 +54,7 @@ def run_internal(maintplanlayerbranch, commit, commitdate, options, logger, bitb
         else:
             cmdprefix = 'python3'
 
-    bitbake_rev = utils.runcmd('git rev-list -1 --before="%s" origin/master' % str(commitdate),
+    bitbake_rev = utils.runcmd(['git', 'rev-list', '-1', '--before=%s' % str(commitdate), 'origin/master'],
                     bitbakepath, logger=logger)
     check_rev = bitbake_map.get(bitbake_rev, None)
     if check_rev:
@@ -128,29 +128,29 @@ def upgrade_history(options, logger):
                 if options.commit:
                     initial = False
                     since = options.commit
-                    since_option = '%s^..%s' % (options.commit, options.commit)
+                    since_option = ['%s^..%s' % (options.commit, options.commit)]
                 elif maintplanbranch.upgrade_rev and not options.fullreload:
                     initial = False
                     since = maintplanbranch.upgrade_date
-                    since_option = '%s..origin/master' % maintplanbranch.upgrade_rev
+                    since_option = ['%s..origin/master' % maintplanbranch.upgrade_rev]
                 else:
                     initial = True
                     since = options.since
-                    since_option = '--since="%s" origin/master' % since
+                    since_option = ['--since=%s' % since, 'origin/master']
 
                 repo = git.Repo(repodir)
                 if repo.bare:
                     logger.error('Repository %s is bare, not supported' % repodir)
                     continue
 
-                commits = utils.runcmd("git log %s --format='%%H %%ct' --reverse" % since_option,
+                commits = utils.runcmd(['git', 'log'] + since_option + ['--format=%H %ct', '--reverse'],
                                     repodir,
                                     logger=logger)
                 commit_list = commits.split('\n')
 
                 bitbake_map = {}
                 # Filter out some bad commits
-                bitbake_commits = utils.runcmd("git rev-list fef18b445c0cb6b266cd939b9c78d7cbce38663f^..39780b1ccbd76579db0fc6fb9369c848a3bafa9d^",
+                bitbake_commits = utils.runcmd(['git', 'rev-list', 'fef18b445c0cb6b266cd939b9c78d7cbce38663f^..39780b1ccbd76579db0fc6fb9369c848a3bafa9d^'],
                                     bitbakepath,
                                     logger=logger)
                 bitbake_commit_list = bitbake_commits.splitlines()

rrs/tools/rrs_upstream_email.py

@@ -71,7 +71,7 @@ def send_email(maintplan, recipes, options):
         urldir = layer.get_fetch_dir()
         repodir = os.path.join(fetchdir, urldir)
         # FIXME this assumes the correct branch is checked out
-        topcommitdesc = utils.runcmd("git log -1 --oneline", repodir).strip()
+        topcommitdesc = utils.runcmd(['git', 'log', '-1', '--oneline'], repodir).strip()
         commits.append('%s: %s' % (layerbranch.layer.name, topcommitdesc))
 
     # Render the subject as a template (to allow a bit of flexibility)

rrs/tools/upgrade_history_internal.py

@@ -122,7 +122,7 @@ def _get_recipes_filenames(ct, repodir, layerdir, logger):
     ct_files = []
     layerdir_start = os.path.normpath(layerdir) + os.sep
 
-    files = utils.runcmd("git log --name-only --format='%n' -n 1 " + ct,
+    files = utils.runcmd(['git', 'log', '--name-only', '--format=%n', '-n', '1', ct],
                             repodir, logger=logger)
 
     incdirs = []
@@ -161,7 +161,7 @@ def checkout_layer_deps(layerbranch, commit, fetchdir, logger):
         repodir = os.path.join(fetchdir, urldir)
         if not repodir in done_repos:
             if not lcommit:
-                lcommit = utils.runcmd('git rev-list -1 --before="%s" origin/master' % lcommitdate, repodir, logger=logger).strip()
+                lcommit = utils.runcmd(['git', 'rev-list', '-1', '--before=%s' % lcommitdate, 'origin/master'], repodir, logger=logger).strip()
             utils.checkout_repo(repodir, lcommit, logger, force)
             done_repos.append(repodir)
 
@@ -170,7 +170,7 @@ def checkout_layer_deps(layerbranch, commit, fetchdir, logger):
     checkout_layer(layerbranch, commit, force=True)
     layer_urldir = str(layerbranch.layer.get_fetch_dir())
     layer_repodir = os.path.join(fetchdir, layer_urldir)
-    commitdate = utils.runcmd("git show -s --format=%ci", layer_repodir, logger=logger)
+    commitdate = utils.runcmd(['git', 'show', '-s', '--format=%ci'], layer_repodir, logger=logger)
 
     for dep in layerbranch.get_recursive_dependencies():
         checkout_layer(dep, lcommitdate=commitdate)
@@ -210,7 +210,7 @@ def generate_history(options, layerbranch_id, commit, logger):
             # Branch name, need to check out detached
             bitbake_rev = 'origin/%s' % bitbake_rev
     else:
-        bitbake_rev = utils.runcmd('git rev-list -1 --before="%s" origin/master' % commitdate, bitbakepath, logger=logger).strip()
+        bitbake_rev = utils.runcmd(['git', 'rev-list', '-1', '--before=%s' % commitdate, 'origin/master'], bitbakepath, logger=logger).strip()
     utils.checkout_repo(bitbakepath, bitbake_rev, logger)
     sys.path.insert(0, os.path.join(bitbakepath, 'lib'))
 
@@ -221,14 +221,12 @@ def generate_history(options, layerbranch_id, commit, logger):
 
         if options.initial:
             title = options.initial
-            info = 'No maintainer;;' + utils.runcmd("git log  --format='%ad;%cd' --date=rfc -n 1 " \
+            info = 'No maintainer;;' + utils.runcmd(['git', 'log', '--format=%ad;%cd', '--date=rfc', '-n', '1', commit], destdir=repodir, logger=logger)
-                            + commit, destdir=repodir, logger=logger)
             recordcommit = ''
         else:
-            title = utils.runcmd("git log --format='%s' -n 1 " + commit,
+            title = utils.runcmd(['git', 'log', '--format=%s', '-n', '1', commit],
                                             repodir, logger=logger)
-            info = utils.runcmd("git log  --format='%an;%ae;%ad;%cd' --date=rfc -n 1 " \
+            info = utils.runcmd(['git', 'log', '--format=%an;%ae;%ad;%cd', '--date=rfc', '-n', '1', commit], destdir=repodir, logger=logger)
-                            + commit, destdir=repodir, logger=logger)
             recordcommit = commit
 
         try: