Project.git(list/str): reduce reliance on shlex.split()

For convenience, Project.git() supports passing either a list (good) or
a string with whitespaces (bad). The latter is parsed with shlex.split()

This saves some typing but the caller has to be extremely careful to
never use the shlex.split() convenience with unsanitized inputs.

Fixes commit 3ac600acaa ("git: clean west ref space after fetching")
where the caller was not careful and concatenated `update-ref -d ` with
unsanitized input, possibly containing special characters as found in
bug #679. Fix this bug by converting the string to a list.

While at it, look for a few other, frequent and risky invocations and
convert their string argument to a list too. The following test hack was
used to semi-automate the search for these other locations:

```
--- a/src/west/manifest.py
+++ b/src/west/manifest.py
@@ -897,6 +897,8 @@ class Project:
         :param cwd: directory to run git in (default: ``self.abspath``)
         '''
         if isinstance(cmd, str):
+            print(cmd)
+            breakpoint()
             cmd_list = shlex.split(cmd)
         else:
             cmd_list = list(cmd)
```

While at it, also convert to a list a couple non-risky but very frequent
invocations. This speeds up the test hack above.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
This commit is contained in:
Marc Herbert 2023-09-01 17:11:22 +00:00 committed by Martí Bolívar
parent bcd58e0e38
commit 41007648d9
2 changed files with 8 additions and 8 deletions

View File

@ -685,7 +685,7 @@ class Compare(_ProjectCommand):
if self.ignore_branches: if self.ignore_branches:
return False return False
return bool(project.git('branch --show-current', return bool(project.git(['branch', '--show-current'],
capture_stdout=True, capture_stdout=True,
capture_stderr=True).stdout.strip()) capture_stderr=True).stdout.strip())
@ -1226,7 +1226,7 @@ class Update(_ProjectCommand):
# out the new detached HEAD, then print some helpful context. # out the new detached HEAD, then print some helpful context.
if take_stats: if take_stats:
start = perf_counter() start = perf_counter()
project.git('checkout --detach ' + sha) project.git(['checkout', '--detach', sha])
if take_stats: if take_stats:
stats['checkout new manifest-rev'] = perf_counter() - start stats['checkout new manifest-rev'] = perf_counter() - start
self.post_checkout_help(project, current_branch, self.post_checkout_help(project, current_branch,
@ -1318,7 +1318,7 @@ class Update(_ProjectCommand):
# This remote is added as a convenience for the user. # This remote is added as a convenience for the user.
# However, west always fetches project data by URL, not name. # However, west always fetches project data by URL, not name.
# The user is therefore free to change the URL of this remote. # The user is therefore free to change the URL of this remote.
project.git(f'remote add -- {project.remote_name} {project.url}') project.git(['remote', 'add', '--', project.remote_name, project.url])
else: else:
self.small_banner(f'{project.name}: cloning from {cache_dir}') self.small_banner(f'{project.name}: cloning from {cache_dir}')
# Clone the project from a local cache repository. Set the # Clone the project from a local cache repository. Set the
@ -1889,14 +1889,14 @@ def _clean_west_refspace(project):
# Clean the refs/west space to ensure they do not show up in 'git log'. # Clean the refs/west space to ensure they do not show up in 'git log'.
# Get all the ref names that start with refs/west/. # Get all the ref names that start with refs/west/.
list_refs_cmd = ('for-each-ref --format="%(refname)" -- ' + list_refs_cmd = ['for-each-ref', '--format=%(refname)', '--',
QUAL_REFS + '**') QUAL_REFS + '**']
cp = project.git(list_refs_cmd, capture_stdout=True) cp = project.git(list_refs_cmd, capture_stdout=True)
west_references = cp.stdout.decode('utf-8').strip() west_references = cp.stdout.decode('utf-8').strip()
# Safely delete each one. # Safely delete each one.
for ref in west_references.splitlines(): for ref in west_references.splitlines():
delete_ref_cmd = 'update-ref -d ' + ref delete_ref_cmd = ['update-ref', '-d', ref]
project.git(delete_ref_cmd) project.git(delete_ref_cmd)
def _update_manifest_rev(project, new_manifest_rev): def _update_manifest_rev(project, new_manifest_rev):

View File

@ -951,7 +951,7 @@ class Project:
# Though we capture stderr, it will be available as the stderr # Though we capture stderr, it will be available as the stderr
# attribute in the CalledProcessError raised by git() in # attribute in the CalledProcessError raised by git() in
# Python 3.5 and above if this call fails. # Python 3.5 and above if this call fails.
cp = self.git(f'rev-parse {rev}^{{commit}}', capture_stdout=True, cp = self.git(['rev-parse', f'{rev}^{{commit}}'], capture_stdout=True,
cwd=cwd, capture_stderr=True) cwd=cwd, capture_stderr=True)
# Assumption: SHAs are hex values and thus safe to decode in ASCII. # Assumption: SHAs are hex values and thus safe to decode in ASCII.
# It'll be fun when we find out that was wrong and how... # It'll be fun when we find out that was wrong and how...
@ -1023,7 +1023,7 @@ class Project:
# instead, which prints an empty string (i.e., just a newline, # instead, which prints an empty string (i.e., just a newline,
# which we strip) for the top-level directory. # which we strip) for the top-level directory.
_logger.debug(f'{self.name}: checking if cloned') _logger.debug(f'{self.name}: checking if cloned')
res = self.git('rev-parse --show-cdup', check=False, cwd=cwd, res = self.git(['rev-parse', '--show-cdup'], check=False, cwd=cwd,
capture_stderr=True, capture_stdout=True) capture_stderr=True, capture_stdout=True)
return not (res.returncode or res.stdout.strip()) return not (res.returncode or res.stdout.strip())