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:
parent
bcd58e0e38
commit
41007648d9
|
@ -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):
|
||||||
|
|
|
@ -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())
|
||||||
|
|
Loading…
Reference in New Issue