* [PATCH 1/3] fetcher: quote filenames given in commands
@ 2012-04-13 14:06 Enrico Scholz
2012-04-13 14:06 ` [PATCH 2/3] runfetchcmd(): allow to use a sequence as cmd Enrico Scholz
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Enrico Scholz @ 2012-04-13 14:06 UTC (permalink / raw)
To: bitbake-devel; +Cc: Enrico Scholz
Filenames were given as-is to shell commands. This causes problems
when names contain characters which have a special meaning. E.g. when
having a local systemd unit
| SRC_URI = "file://etc-machine\20id.mount"
the fetcher failed with
| NOTE: Unpacking .../etc-machine\20id.mount to ...
| cp: cannot stat `.../etc-machine20id.mount': No such file or directory
Patch uses the pipes.quote() function to apply shell escaping. These
changes are not really complete; they are modifying __init__.py only
but most fetchers need similar adaptations too.
Signed-off-by: Enrico Scholz <enrico.scholz@informatik.tu-chemnitz.de>
---
lib/bb/fetch2/__init__.py | 38 +++++++++++++++++++++-----------------
1 files changed, 21 insertions(+), 17 deletions(-)
diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index 414cc2b..9afbc4f 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -29,6 +29,7 @@ from __future__ import print_function
import os, re
import logging
import bb.persist_data, bb.utils
+import pipes
from bb import data
__version__ = "2"
@@ -712,38 +713,39 @@ class FetchMethod(object):
cmd = None
if unpack:
+ qfile = pipes.quote(file)
if file.endswith('.tar'):
- cmd = 'tar x --no-same-owner -f %s' % file
+ cmd = ['tar', 'x', '--no-same-owner', '-f', file]
elif file.endswith('.tgz') or file.endswith('.tar.gz') or file.endswith('.tar.Z'):
- cmd = 'tar xz --no-same-owner -f %s' % file
+ cmd = ['tar', 'xz', '--no-same-owner', '-f', file]
elif file.endswith('.tbz') or file.endswith('.tbz2') or file.endswith('.tar.bz2'):
- cmd = 'bzip2 -dc %s | tar x --no-same-owner -f -' % file
+ cmd = 'bzip2 -dc %s | tar x --no-same-owner -f -' % qfile
elif file.endswith('.gz') or file.endswith('.Z') or file.endswith('.z'):
- cmd = 'gzip -dc %s > %s' % (file, efile)
+ cmd = 'gzip -dc %s > %s' % (qfile, pipes.quote(efile))
elif file.endswith('.bz2'):
- cmd = 'bzip2 -dc %s > %s' % (file, efile)
+ cmd = 'bzip2 -dc %s > %s' % (qfile, pipes.quote(efile))
elif file.endswith('.tar.xz'):
- cmd = 'xz -dc %s | tar x --no-same-owner -f -' % file
+ cmd = 'xz -dc %s | tar x --no-same-owner -f -' % qfile
elif file.endswith('.xz'):
- cmd = 'xz -dc %s > %s' % (file, efile)
+ cmd = 'xz -dc %s > %s' % (qfile, pipes.quote(efile))
elif file.endswith('.zip') or file.endswith('.jar'):
try:
dos = bb.utils.to_boolean(urldata.parm.get('dos'), False)
except ValueError as exc:
bb.fatal("Invalid value for 'dos' parameter for %s: %s" %
(file, urldata.parm.get('dos')))
- cmd = 'unzip -q -o'
+ cmd = ['unzip', '-q', '-o']
if dos:
- cmd = '%s -a' % cmd
- cmd = "%s '%s'" % (cmd, file)
+ cmd.append('-a')
+ cmd.append(file)
elif file.endswith('.src.rpm') or file.endswith('.srpm'):
if 'extract' in urldata.parm:
unpack_file = urldata.parm.get('extract')
- cmd = 'rpm2cpio.sh %s | cpio -i %s' % (file, unpack_file)
+ cmd = 'rpm2cpio.sh %s | cpio -i %s' % (qfile, pipes.quote(unpack_file))
iterate = True
iterate_file = unpack_file
else:
- cmd = 'rpm2cpio.sh %s | cpio -i' % (file)
+ cmd = 'rpm2cpio.sh %s | cpio -i' % qfile
if not unpack or not cmd:
# If file == dest, then avoid any copies, as we already put the file into dest!
@@ -759,8 +761,8 @@ class FetchMethod(object):
destdir = "."
elif not os.access("%s/%s" % (rootdir, destdir), os.F_OK):
os.makedirs("%s/%s" % (rootdir, destdir))
- cmd = 'cp -pPR %s %s/%s/' % (file, rootdir, destdir)
- #cmd = 'tar -cf - -C "%d" -ps . | tar -xf - -C "%s/%s/"' % (file, rootdir, destdir)
+ cmd = ['cp', '-pPR', file, '%s/%s/' % (rootdir, destdir)]
+ #cmd = 'tar -cf - -C "%d" -ps . | tar -xf - -C "%s/%s/"' % (pipes.quote(file), pipes.quote(rootdir), pipes.quote(destdir))
else:
# The "destdir" handling was specifically done for FILESPATH
# items. So, only do so for file:// entries.
@@ -769,7 +771,7 @@ class FetchMethod(object):
else:
destdir = "."
bb.utils.mkdirhier("%s/%s" % (rootdir, destdir))
- cmd = 'cp %s %s/%s/' % (file, rootdir, destdir)
+ cmd = ['cp', file, '%s/%s/' % (rootdir, destdir)]
if not cmd:
return
@@ -782,9 +784,11 @@ class FetchMethod(object):
bb.utils.mkdirhier(newdir)
os.chdir(newdir)
- cmd = "PATH=\"%s\" %s" % (data.getVar('PATH', True), cmd)
+ new_env = os.environ.copy()
+ new_env['PATH'] = data.getVar('PATH', True)
bb.note("Unpacking %s to %s/" % (file, os.getcwd()))
- ret = subprocess.call(cmd, preexec_fn=subprocess_setup, shell=True)
+ ret = subprocess.call(cmd, preexec_fn=subprocess_setup,
+ shell=isinstance(cmd, basestring), env=new_env)
os.chdir(save_cwd)
--
1.7.7.6
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/3] runfetchcmd(): allow to use a sequence as cmd 2012-04-13 14:06 [PATCH 1/3] fetcher: quote filenames given in commands Enrico Scholz @ 2012-04-13 14:06 ` Enrico Scholz 2012-04-13 14:06 ` [PATCH 3/3] fetch2/git: quote arguments Enrico Scholz 2012-04-14 10:21 ` [PATCH 1/3] fetcher: quote filenames given in commands Richard Purdie 2 siblings, 0 replies; 5+ messages in thread From: Enrico Scholz @ 2012-04-13 14:06 UTC (permalink / raw) To: bitbake-devel; +Cc: Enrico Scholz Patch removes the explicit 'shell=True' from the bb.process.run() call and modifies the environment directly. This allows to use a sequence as the cmd option which in turn, removes the need for quoting filenames manually. Unfortunately, modifying only runfetchmod() does not allow code like runfetchcmd([ud.basecmd, 'remote', 'add', ..., repourl]) because 'ud.basecmd' (expanded e.g. from ${FETCHCMD_git}) might contain a command plus shell escaped options. To cope with such situations, a runfetchcmd2() function was added which quotes the arguments but takes the cmd as-is. Having basecmd splitted off from cmdline string reduces information printed out by check_network_access(). To retain the old behavior, an optional argument was added which allows to specify the basecmd. Signed-off-by: Enrico Scholz <enrico.scholz@informatik.tu-chemnitz.de> --- lib/bb/fetch2/__init__.py | 26 +++++++++++++++++++++----- 1 files changed, 21 insertions(+), 5 deletions(-) diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py index 9afbc4f..168efb5 100644 --- a/lib/bb/fetch2/__init__.py +++ b/lib/bb/fetch2/__init__.py @@ -407,10 +407,11 @@ def runfetchcmd(cmd, d, quiet = False, cleanup = []): 'SSH_AUTH_SOCK', 'SSH_AGENT_PID', 'HOME', 'GIT_PROXY_IGNORE', 'SOCKS5_USER', 'SOCKS5_PASSWD'] + new_env = os.environ.copy() for var in exportvars: val = d.getVar(var, True) if val: - cmd = 'export ' + var + '=\"%s\"; %s' % (val, cmd) + new_env[var] = val logger.debug(1, "Running %s", cmd) @@ -418,7 +419,7 @@ def runfetchcmd(cmd, d, quiet = False, cleanup = []): error_message = "" try: - (output, errors) = bb.process.run(cmd, shell=True, stderr=subprocess.PIPE) + (output, errors) = bb.process.run(cmd, stderr=subprocess.PIPE, env=new_env) success = True except bb.process.NotFoundError as e: error_message = "Fetch command %s" % (e.command) @@ -438,14 +439,29 @@ def runfetchcmd(cmd, d, quiet = False, cleanup = []): return output -def check_network_access(d, info = "", url = None): +def runfetchcmd2(cmd, args, d, **opts): + cmd += ' ' + cmd += ' '.join(map(lambda a: pipes.quote(a), args)) + return runfetchcmd(cmd, d, **opts) + +def check_network_access(d, info = "", url = None, base_cmd = None): """ log remote network access, and error if BB_NO_NETWORK is set """ + if base_cmd: + info_str = base_cmd + ' ' + else: + info_str = '' + + if isinstance(info, list) or isinstance(info, tuple): + info_str += ' '.join(map(lambda a: pipes.quote(a), info)) + else: + info_str += '%s' % info + if d.getVar("BB_NO_NETWORK", True) == "1": - raise NetworkAccess(url, info) + raise NetworkAccess(url, info_str) else: - logger.debug(1, "Fetcher accessed the network with the command %s" % info) + logger.debug(1, "Fetcher accessed the network with the command %s" % info_str) def try_mirrors(d, origud, mirrors, check = False): """ -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] fetch2/git: quote arguments 2012-04-13 14:06 [PATCH 1/3] fetcher: quote filenames given in commands Enrico Scholz 2012-04-13 14:06 ` [PATCH 2/3] runfetchcmd(): allow to use a sequence as cmd Enrico Scholz @ 2012-04-13 14:06 ` Enrico Scholz 2012-04-14 10:21 ` [PATCH 1/3] fetcher: quote filenames given in commands Richard Purdie 2 siblings, 0 replies; 5+ messages in thread From: Enrico Scholz @ 2012-04-13 14:06 UTC (permalink / raw) To: bitbake-devel; +Cc: Enrico Scholz Patch makes sure that arguments given to 'git' are quoted properly. It uses the newly added runfetchcmd2() function or quotes them manually when special shell functionality (pipes, io redirections) are used. Signed-off-by: Enrico Scholz <enrico.scholz@informatik.tu-chemnitz.de> --- lib/bb/fetch2/git.py | 53 +++++++++++++++++++++++++++---------------------- 1 files changed, 29 insertions(+), 24 deletions(-) diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py index 61fdc4b..52adb9a 100644 --- a/lib/bb/fetch2/git.py +++ b/lib/bb/fetch2/git.py @@ -65,8 +65,9 @@ import os import bb from bb import data from bb.fetch2 import FetchMethod -from bb.fetch2 import runfetchcmd +from bb.fetch2 import runfetchcmd, runfetchcmd2 from bb.fetch2 import logger +import pipes class Git(FetchMethod): """Class to fetch a module or modules from git repositories""" @@ -115,6 +116,7 @@ class Git(FetchMethod): ud.branches[name] = branch ud.basecmd = data.getVar("FETCHCMD_git", d, True) or "git" + ud.runfetch = lambda args,d,**opts: runfetchcmd2(ud.basecmd, args, d, **opts) ud.write_tarballs = ((data.getVar("BB_GENERATE_MIRROR_TARBALLS", d, True) or "0") != "0") or ud.rebaseable @@ -177,15 +179,15 @@ class Git(FetchMethod): if not os.path.exists(ud.clonedir) and os.path.exists(ud.fullmirror): bb.utils.mkdirhier(ud.clonedir) os.chdir(ud.clonedir) - runfetchcmd("tar -xzf %s" % (ud.fullmirror), d) + runfetchcmd(['tar', '-xzf', ud.fullmirror], d) repourl = "%s://%s%s%s" % (ud.proto, username, ud.host, ud.path) # If the repo still doesn't exist, fallback to cloning it if not os.path.exists(ud.clonedir): - clone_cmd = "%s clone --bare --mirror %s %s" % (ud.basecmd, repourl, ud.clonedir) - bb.fetch2.check_network_access(d, clone_cmd) - runfetchcmd(clone_cmd, d) + clone_cmd = ['clone', '--bare', '--mirror', repourl, ud.clonedir] + bb.fetch2.check_network_access(d, clone_cmd, base_cmd = ud.basecmd, url = repourl) + ud.runfetch(clone_cmd, d) os.chdir(ud.clonedir) # Update the checkout if needed @@ -200,10 +202,10 @@ class Git(FetchMethod): except bb.fetch2.FetchError: logger.debug(1, "No Origin") - runfetchcmd("%s remote add --mirror=fetch origin %s" % (ud.basecmd, repourl), d) - fetch_cmd = "%s fetch -f --prune %s refs/*:refs/*" % (ud.basecmd, repourl) - bb.fetch2.check_network_access(d, fetch_cmd, ud.url) - runfetchcmd(fetch_cmd, d) + ud.runfetch(['remote', 'add', '--mirror=fetch', 'origin', repourl], d) + fetch_cmd = ['fetch', '-f', '--prune', repourl, 'refs/*:refs/*'] + bb.fetch2.check_network_access(d, fetch_cmd, base_cmd = ud.basecmd, url = ud.url) + ud.runfetch(fetch_cmd, d) runfetchcmd("%s prune-packed" % ud.basecmd, d) runfetchcmd("%s pack-redundant --all | xargs -r rm" % ud.basecmd, d) ud.repochanged = True @@ -213,8 +215,8 @@ class Git(FetchMethod): if ud.write_tarballs and (ud.repochanged or not os.path.exists(ud.fullmirror)): os.chdir(ud.clonedir) logger.info("Creating tarball of git repository") - runfetchcmd("tar -czf %s %s" % (ud.fullmirror, os.path.join(".") ), d) - runfetchcmd("touch %s.done" % (ud.fullmirror), d) + runfetchcmd(['tar', '-czf', ud.fullmirror, os.path.join(".")], d) + runfetchcmd(['touch', ud.fullmirror + '.done'], d) def unpack(self, ud, destdir, d): """ unpack the downloaded src to destdir""" @@ -232,18 +234,20 @@ class Git(FetchMethod): if os.path.exists(destdir): bb.utils.prunedir(destdir) - cloneflags = "-s -n" + cloneflags = ['clone', '-s', '-n'] if ud.bareclone: - cloneflags += " --mirror" + cloneflags.append("--mirror") + + cloneflags.extend(["%s/" % ud.clonedir, destdir]) + ud.runfetch(cloneflags, d) - runfetchcmd("git clone %s %s/ %s" % (cloneflags, ud.clonedir, destdir), d) if not ud.nocheckout: os.chdir(destdir) if subdir != "": - runfetchcmd("%s read-tree %s%s" % (ud.basecmd, ud.revisions[ud.names[0]], readpathspec), d) + ud.runfetch(['read-tree', '%s%s' % ud.revisions[ud.names[0]], readpathspec]) runfetchcmd("%s checkout-index -q -f -a" % ud.basecmd, d) else: - runfetchcmd("%s checkout %s" % (ud.basecmd, ud.revisions[ud.names[0]]), d) + ud.runfetch(['checkout', ud.revisions[ud.names[0]]], d) return True def clean(self, ud, d): @@ -257,7 +261,7 @@ class Git(FetchMethod): def _contains_ref(self, tag, d): basecmd = data.getVar("FETCHCMD_git", d, True) or "git" - cmd = "%s log --pretty=oneline -n 1 %s -- 2> /dev/null | wc -l" % (basecmd, tag) + cmd = "%s log --pretty=oneline -n 1 %s -- 2> /dev/null | wc -l" % (basecmd, pipes.quote(tag)) output = runfetchcmd(cmd, d, quiet=True) if len(output.split()) > 1: raise bb.fetch2.FetchError("The command '%s' gave output with more then 1 line unexpectedly, output: '%s'" % (cmd, output)) @@ -279,10 +283,12 @@ class Git(FetchMethod): username = "" basecmd = data.getVar("FETCHCMD_git", d, True) or "git" - cmd = "%s ls-remote %s://%s%s%s %s" % \ - (basecmd, ud.proto, username, ud.host, ud.path, ud.branches[name]) - bb.fetch2.check_network_access(d, cmd) - output = runfetchcmd(cmd, d, True) + cmd = ['ls-remote', + '%s://%s%s%s' % (ud.proto, username, ud.host, ud.path), + ud.branches[name]] + + bb.fetch2.check_network_access(d, cmd, base_cmd = ud.basecmd) + output = ud.runfetch(cmd, d, quiet=True) if not output: raise bb.fetch2.FetchError("The command %s gave empty output unexpectedly" % cmd, url) return output.split()[0] @@ -315,7 +321,7 @@ class Git(FetchMethod): if not self._contains_ref(rev, d): self.download(None, ud, d) - output = runfetchcmd("%s rev-list %s -- 2> /dev/null | wc -l" % (ud.basecmd, rev), d, quiet=True) + output = runfetchcmd("%s rev-list %s -- 2> /dev/null | wc -l" % (ud.basecmd, pipes.quote(rev)), d, quiet=True) os.chdir(cwd) buildindex = "%s" % output.split()[0] @@ -323,9 +329,8 @@ class Git(FetchMethod): return buildindex def checkstatus(self, uri, ud, d): - fetchcmd = "%s ls-remote %s" % (ud.basecmd, uri) try: - runfetchcmd(fetchcmd, d, quiet=True) + ud.runfetch(['ls-remote', uri], d, quiet=True) return True except FetchError: return False -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] fetcher: quote filenames given in commands 2012-04-13 14:06 [PATCH 1/3] fetcher: quote filenames given in commands Enrico Scholz 2012-04-13 14:06 ` [PATCH 2/3] runfetchcmd(): allow to use a sequence as cmd Enrico Scholz 2012-04-13 14:06 ` [PATCH 3/3] fetch2/git: quote arguments Enrico Scholz @ 2012-04-14 10:21 ` Richard Purdie 2012-04-14 13:20 ` Enrico Scholz 2 siblings, 1 reply; 5+ messages in thread From: Richard Purdie @ 2012-04-14 10:21 UTC (permalink / raw) To: Enrico Scholz; +Cc: bitbake-devel On Fri, 2012-04-13 at 16:06 +0200, Enrico Scholz wrote: > Filenames were given as-is to shell commands. This causes problems > when names contain characters which have a special meaning. E.g. when > having a local systemd unit > > | SRC_URI = "file://etc-machine\20id.mount" > > the fetcher failed with > > | NOTE: Unpacking .../etc-machine\20id.mount to ... > | cp: cannot stat `.../etc-machine20id.mount': No such file or directory > > Patch uses the pipes.quote() function to apply shell escaping. These > changes are not really complete; they are modifying __init__.py only > but most fetchers need similar adaptations too. > > Signed-off-by: Enrico Scholz <enrico.scholz@informatik.tu-chemnitz.de> > --- > lib/bb/fetch2/__init__.py | 38 +++++++++++++++++++++----------------- > 1 files changed, 21 insertions(+), 17 deletions(-) Whilst I understand what you're trying to fix, I'm afraid I don't like the patch. I'd prefer to have one way of building these commands, rather than two different ones with the shell=isinstance(cmd, basestring) check. We're stabilising for release at the moment and I don't have enough confidence in these patches at this point in the cycle, particularly since you change runfetchcmd but not other users in other fetch methods other than git. I'm planning to hold off these until after that and even then, I'd like to find a cleaner approach that doesn't involve runfetchcmd2. Cheers, Richard ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] fetcher: quote filenames given in commands 2012-04-14 10:21 ` [PATCH 1/3] fetcher: quote filenames given in commands Richard Purdie @ 2012-04-14 13:20 ` Enrico Scholz 0 siblings, 0 replies; 5+ messages in thread From: Enrico Scholz @ 2012-04-14 13:20 UTC (permalink / raw) To: Richard Purdie; +Cc: bitbake-devel Richard Purdie <richard.purdie@linuxfoundation.org> writes: >> Filenames were given as-is to shell commands. This causes problems >> when names contain characters which have a special meaning. > ... > Whilst I understand what you're trying to fix, I'm afraid I don't like > the patch. I'd prefer to have one way of building these commands, rather > than two different ones with the shell=isinstance(cmd, basestring) > check. ok; one way would be calling of 'pipes.quote()' on every variable substituting %s. But that's boring, error prone and makes the code ugly. Another way might be to declare every command as a sequence with special objects for shell operations; e.g. | class ShellOp: | def __init__(self, op): | self.op = op | | cmd = ['bunzip2', '-d', file, ShellOp('|'), 'tar', 'xf', '-'] In simple case (no ShellOp objects), sequence will be given directly to Popen() with shell=False. Else, pipes.quote() will be applied on every string element of the sequence while ShellOp() objects will be kept as as-is. > I'd like to find a cleaner approach that doesn't involve runfetchcmd2. As I wrote in the comment, runfetchcmd2() exists because ${FETCHCMD} might be defined as git -c 'url.file://${WORKSPACE_DIR}/avrprog.insteadOf=git://git.somewhere/avrprog.git' There is no simple way to split it into a sequence (note: there might be quoted whitespaces in an option). Enrico ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-04-14 13:34 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-04-13 14:06 [PATCH 1/3] fetcher: quote filenames given in commands Enrico Scholz 2012-04-13 14:06 ` [PATCH 2/3] runfetchcmd(): allow to use a sequence as cmd Enrico Scholz 2012-04-13 14:06 ` [PATCH 3/3] fetch2/git: quote arguments Enrico Scholz 2012-04-14 10:21 ` [PATCH 1/3] fetcher: quote filenames given in commands Richard Purdie 2012-04-14 13:20 ` Enrico Scholz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.