All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.