Patchwork [bitbake-devel,1/3] fetcher: quote filenames given in commands

login
register
mail settings
Submitter Enrico Scholz
Date April 13, 2012, 2:06 p.m.
Message ID <1334325973-15760-1-git-send-email-enrico.scholz@informatik.tu-chemnitz.de>
Download mbox | patch
Permalink /patch/25757/
State New
Headers show

Comments

Enrico Scholz - April 13, 2012, 2:06 p.m.
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(-)
Richard Purdie - April 14, 2012, 10:21 a.m.
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
Enrico Scholz - April 14, 2012, 1:20 p.m.
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

Patch

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)