Skip to content

Commit

Permalink
Project.git(list/str): reduce reliance on shlex.split()
Browse files Browse the repository at this point in the history
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 3ac600a ("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>
  • Loading branch information
marc-hb committed Sep 1, 2023
1 parent 92f093d commit 6325366
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 8 deletions.
12 changes: 6 additions & 6 deletions src/west/app/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,7 @@ def has_checked_out_branch(project):
if self.ignore_branches:
return False

return bool(project.git('branch --show-current',
return bool(project.git(['branch', '--show-current'],
capture_stdout=True,
capture_stderr=True).stdout.strip())

Expand Down Expand Up @@ -1228,7 +1228,7 @@ def update(self, project):
# out the new detached HEAD, then print some helpful context.
if take_stats:
start = perf_counter()
project.git('checkout --detach ' + sha)
project.git(['checkout', '--detach', sha])
if take_stats:
stats['checkout new manifest-rev'] = perf_counter() - start
self.post_checkout_help(project, current_branch,
Expand Down Expand Up @@ -1320,7 +1320,7 @@ def init_project(self, project):
# This remote is added as a convenience for the user.
# However, west always fetches project data by URL, not name.
# 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:
self.small_banner(f'{project.name}: cloning from {cache_dir}')
# Clone the project from a local cache repository. Set the
Expand Down Expand Up @@ -1891,14 +1891,14 @@ def _clean_west_refspace(project):
# 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/.
list_refs_cmd = ('for-each-ref --format="%(refname)" -- ' +
QUAL_REFS + '**')
list_refs_cmd = ['for-each-ref', '--format=%(refname)', '--',
QUAL_REFS + '**']
cp = project.git(list_refs_cmd, capture_stdout=True)
west_references = cp.stdout.decode('utf-8').strip()

# Safely delete each one.
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)

def _update_manifest_rev(project, new_manifest_rev):
Expand Down
4 changes: 2 additions & 2 deletions src/west/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -951,7 +951,7 @@ def sha(self, rev: str, cwd: Optional[PathType] = None) -> str:
# Though we capture stderr, it will be available as the stderr
# attribute in the CalledProcessError raised by git() in
# 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)
# 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...
Expand Down Expand Up @@ -1023,7 +1023,7 @@ def is_cloned(self, cwd: Optional[PathType] = None) -> bool:
# instead, which prints an empty string (i.e., just a newline,
# which we strip) for the top-level directory.
_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)

return not (res.returncode or res.stdout.strip())
Expand Down

0 comments on commit 6325366

Please sign in to comment.