more mounts in mock
Michael E Brown
Michael_E_Brown at dell.com
Fri May 26 05:42:42 UTC 2006
Comments below...
On Thu, 2006-05-25 at 21:43 -0400, Mike McLean wrote:
> Attached are a couple of patches that expand the mounts created in
> the
> chroot by mock. These are mounts that we've used for builds within
> Red
> Hat for years and some packages need them to compile properly.
>
> more_mounts.patch is the larger patch, it refactors _mount() so that
> the
> mounts to be created are specified in a list and looped over. I've
> also
> changed to the unmounting code to make it more paranoid. In order to
> allow these mounts, I had to make some changes to mock-helper.
>
> bind_dev.patch builds on the the previous patch and provides an
> option
> to have /dev bind mounted in the chroot (instead of the skeletal /dev
> that mock sets up). The patch makes this an option with the original
> behavior as the default.
>
>
>
>
>
>
>
>
> differences
> between files
> attachment
> (more_mounts.patch)
>
> ? more_mounts.patch
> Index: mock.py
> ===================================================================
> RCS file: /cvs/fedora/mock/mock.py,v
> retrieving revision 1.51
> diff -u -r1.51 mock.py
> --- mock.py 24 May 2006 15:15:19 -0000 1.51
> +++ mock.py 26 May 2006 01:21:35 -0000
> @@ -177,11 +177,9 @@
> self.state("clean")
>
> self.root_log('Cleaning Root')
> - if os.path.exists('%s/%s' % (self.rootdir, 'proc')):
> - self._umount('proc')
> - if os.path.exists('%s/%s' % (self.rootdir, 'dev/pts')):
> - self._umount('dev/pts')
> -
> + self._umount_by_proc()
> + self._umount_by_file()
> +
> if os.path.exists(self.basedir):
> cmd = '%s -rfv %s' % (self.config['rm'], self.basedir)
> (retval, output) = self.do(cmd)
> @@ -381,6 +379,7 @@
> """unmount things and clean up a bit"""
> self.root_log("Cleaning up...")
> self.state("ending")
> + self._umount_by_proc()
> self._umount_by_file()
> self._build_log.close()
> self.state("done")
> @@ -403,44 +402,49 @@
>
> def _mount(self):
> """mount proc and devpts into chroot"""
> +
> + #dev,path(relative),type,opts
> + mounts = [('proc','proc','proc',None)]
> + if os.path.exists('/selinux'):
> + mounts.append(('/selinux','selinux','none','bind'))
How will this affect builds of eg. fedora development pkgs on build
systems that dont have selinux, or do not have it enabled?
> + #these have to come after /dev
> + mounts.extend([
> + ('pts','dev/pts','devpts', 'gid=5,mode=620'),
> + ('shm','dev/shm','tmpfs',None),])
tmpfs filesystems can have args specifying max mem usage. Would that be
useful?
> +
> + #read existing mounts
> + current = self._find_mounts()
> mf = os.path.join(self.statedir, 'mounted-locations')
> track = open(mf, 'w+')
>
> - # make the procdir if we don't have it
> - # mount up proc
> - procdir = os.path.join(self.rootdir, 'proc')
> - self._ensure_dir(procdir)
> -
> - self.debug("mounting proc in %s" % procdir)
> - command = '%s -t proc proc %s/proc' % (self.config['mount'],
> - self.rootdir)
> - track.write('proc\n')
> - (retval, output) = self.do(command)
> - track.flush()
> -
> - if retval != 0:
> - if output.find('already mounted') == -1: # probably won't
> work in other LOCALES
> - error("could not mount proc error was: %s" % output)
> -
> - # devpts
> - #
> - devptsdir = os.path.join(self.rootdir, 'dev/pts')
> - self._ensure_dir(devptsdir)
> - self.debug("mounting devpts in %s" % devptsdir)
> - command = '%s -t devpts devpts %s' % (self.config['mount'],
> devptsdir)
> - track.write('dev/pts\n')
> - (retval, output) = self.do(command)
> - track.flush()
> + # mount all of the items listed in mounts
> + for dev,path,type,opts in mounts:
> + if path in current:
> + self.debug("Already mounted: %s" % path)
> + continue
> + mount_dir = os.path.normpath('/'.join([self.rootdir,
> path]))
> + self._ensure_dir(mount_dir)
> + self.debug("mounting %s in %s" % (path, mount_dir))
> + command = [self.config['mount'], '-t', type]
> + if opts is not None:
> + command.extend(['-o', opts])
> + command.extend([dev, mount_dir])
> +
> + track.write('%s\n' % path)
> + (retval, output) = self.do(' '.join(command))
> + track.flush()
> + if retval != 0:
> + if output.find('already mounted') == -1: # probably
> won't work in other LOCALES
> + raise RootError, "could not mount %s error was: %
> s" % (mount_dir, output)
> track.close()
>
> - if retval != 0:
> - if output.find('already mounted') == -1: # probably won't
> work in other LOCALES
> - raise RootError, "could not mount /dev/pts error was:
> %s" % output
> -
> -
> def _umount(self, path):
>
> item = '%s/%s' % (self.rootdir, path)
> + if not os.path.exists(item):
> + #skip
> + self.debug("skipping umount: no such path: %s" % item)
> + return
superfluous comment.
> command = '%s %s' % (self.config['umount'], item)
> (retval, output) = self.do(command)
>
> @@ -448,7 +452,36 @@
> if output.find('not mounted') == -1: # this probably
> won't work in other LOCALES
> raise RootError, "could not umount %s error was: %s"
> % (path, output)
>
> -
> + def _find_mounts(self):
> + #read existing mounts
> + ret = []
> + rootdir = os.path.normpath(self.rootdir)
> + fo = file('/proc/mounts','r')
> + for line in fo.readlines():
> + path = line.split()[1]
> + if not path.startswith(rootdir):
> + continue
> + path = path[len(rootdir):]
> + if path[:1] == '/':
> + path = path[1:]
> + if path:
> + ret.append(path)
> + fo.close()
> + #reverse sort so deeper mounts come first
> + ret.sort()
> + ret.reverse()
> + self.debug("Current mounts: %r" % ret)
> + return ret
> +
> + def _umount_by_proc(self):
> + for path in self._find_mounts():
> + self.debug("ummounting /proc/mounts entry: %r" % path)
> + self._umount(path)
> + #check
> + remain = self._find_mounts()
> + if remain:
> + raise RootError, "mounts remain: %r" % remain
> +
> def _umount_by_file(self):
>
> mf = os.path.join(self.statedir, 'mounted-locations')
> @@ -459,11 +492,14 @@
> lines = track.readlines()
> track.close()
>
> + lines.sort()
> + lines.reverse()
> for item in lines:
> item = item.replace('\n','')
> if len(item.strip()) < 1:
> continue
>
> + self.debug("ummounting file entry: %r" % item)
> self._umount(item)
>
> # poof, no more file
> Index: src/config.h
> ===================================================================
> RCS file: /cvs/fedora/mock/src/config.h,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 config.h
> --- src/config.h 16 May 2005 02:44:00 -0000 1.1.1.1
> +++ src/config.h 26 May 2006 01:21:35 -0000
> @@ -44,7 +44,7 @@
> #define PACKAGE "mock"
>
> /* Location of mock roots */
> -#define ROOTSDIR "/var/lib/mock"
> +#define ROOTSDIR "/var/lib/mock/"
>
> /* Define to 1 if you have the ANSI C header files. */
> #define STDC_HEADERS 1
> Index: src/mock-helper.c
> ===================================================================
> RCS file: /cvs/fedora/mock/src/mock-helper.c,v
> retrieving revision 1.9
> diff -u -r1.9 mock-helper.c
> --- src/mock-helper.c 24 May 2006 15:15:20 -0000 1.9
> +++ src/mock-helper.c 26 May 2006 01:21:35 -0000
> @@ -32,6 +32,28 @@
>
> #define ALLOWED_ENV_SIZE (sizeof (ALLOWED_ENV) / sizeof
> (ALLOWED_ENV[0]))
>
> +/* allowed values to mount type */
> +static char const * const ALLOWED_MTYPE[] =
> +{
> + "proc",
> + "devpts",
> + "tmpfs",
> + "none",
> +};
> +
> +#define ALLOWED_MTYPE_SIZE (sizeof (ALLOWED_MTYPE) / sizeof
> (ALLOWED_MTYPE[0]))
> +
> +/* allowed mount options */
> +static char const * const ALLOWED_MOPT[] =
> +{
> + "bind",
> + "gid=5",
> + "mode=620",
> +};
Why is gid hard-coded here?
> +
> +#define ALLOWED_MOPT_SIZE (sizeof (ALLOWED_MOPT) / sizeof
> (ALLOWED_MOPT[0]))
> +#define MOPT_BUFLEN 128
> +
> /*
> * helper functions
> */
> @@ -58,6 +80,54 @@
> }
>
> /*
> + * check mount type against allowed list
> + */
> +void check_mount_type(const char *type)
> +{
> + size_t i;
> + for (i = 0; i < ALLOWED_MTYPE_SIZE; ++i)
> + {
> + char const *ptr = ALLOWED_MTYPE[i];
> + if (strcmp (type, ptr) == 0) {
> + return;
> + }
> + }
> + error ("mount type not allowed: %s", type);
> +}
> +
> +/*
> + * check mount options against allowed list
> + */
> +void check_mount_opts(const char *optstring)
> +{
> + size_t i,j;
> + char *buf = strndup(optstring, MOPT_BUFLEN);
> + char *opt;
> + if (ALLOWED_MOPT_SIZE == 0)
> + /* allowed option list empty */
> + error ("no mount options allowed");
> + /* iterate over comma-separated list */
> + opt = strtok(buf,",");
> + while (opt)
> + {
> + int allowed = 0;
> + /* iterate over allowed opts */
> + for (j = 0; j < ALLOWED_MOPT_SIZE; ++j)
> + {
> + char const *ptr = ALLOWED_MOPT[j];
> + if (strcmp (opt, ptr) == 0) {
> + allowed = 1;
> + break;
> + }
> + }
> + if (!allowed)
> + error ("mount option not allowed: %s", opt);
> + opt = strtok(NULL,",");
> + }
> + free(buf);
> +}
> +
> +/*
> * perform checks on the given dir
> * - is the given dir under the allowed hierarchy ?
> * - is it an actual dir ?
> @@ -204,40 +274,92 @@
> }
>
> /*
> - * allow proc mounts:
> - * mount -t proc proc (root)/proc
> - * allow devpts mounts:
> - * mount -t devpts devpts (root)/dev/pts
> + * check against: ALLOWED_MTYPE, ALLOWED_MOPT
> + * check that mountpoint lies under rootsdir
> + * verify that needed params are present
> */
> void
> do_mount (int argc, char *argv[])
probably just me, but this function seems a bit long.
> {
> - /* see if we have enough arguments for it to be what we want, ie. 5
> */
> - if (argc < 5)
> - error ("not enough arguments");
> -
> - /* see if it's -t proc or -t devpts */
> - if ((strncmp ("-t", argv[2], 2) == 0) &&
> - (strncmp ("proc", argv[3], 4) == 0))
> + char *mopts = NULL;
> + char *mtype = NULL;
> + char *dev = NULL;
> + char *dir = NULL;
> + char *cmd[10];
> + int i;
> +
> + if (argc > 8)
> + error("too many arguments");
> +
> + /* scan args */
> + char opt = '\0';
> + int count = 0;
> + /* note we skip the first two args, which should be ('mock-helper',
> 'mount') */
> + for (i=2; i<argc; i++)
> {
> - /* see if we're mounting proc to somewhere in rootsdir */
> - if (strncmp (rootsdir, argv[5], strlen (rootsdir)) != 0)
> - error ("proc: mount not allowed on %s", argv[5]);
> + if (strcmp(argv[i], "-t") == 0)
> + {
> + if (++i >= argc)
> + error("incomplete type specification");
> + mtype = argv[i];
why not check_mount_type() here? Saves conditional later.
> + }
> + else if (strcmp(argv[i], "-o") == 0)
> + {
> + if (++i >= argc)
> + error("incomplete option specification");
> + mopts = argv[i];
why not check_mount_opts() here? saves conditional later.
> + }
> + else if (*argv[i] == '-')
> + error ("option not allowed: %s", strndup(argv[i],10));
> + else
> + {
> + /* normal parameter */
> + if (count == 0)
> + {
> + dev = argv[i];
> + count++;
> + }
> + else if (count == 1)
> + {
> + dir = argv[i];
> + count++;
> + }
> + else
> + error("Too many parameters");
> + }
> }
> - else if ((strncmp ("-t", argv[2], 2) == 0) &&
> - (strncmp ("devpts", argv[3], 6) == 0))
> +
> + if (!dev)
> + error("No device provided");
> + if (!dir)
> + error("No dir given");
> + if (!mtype)
> + error("No type given");
> + /* verify that type is allowed */
> + check_mount_type(mtype);
> + if (mopts)
> + /* verify mount options are allowed */
> + check_mount_opts(mopts);
> +
> + /* see if we're mounting to somewhere in rootsdir */
> + if (strncmp (rootsdir, dir, strlen(rootsdir)) != 0)
> + error ("mount not allowed on %s", strndup(dir,80));
looks like a security problem here. You should use check_dir_allowed()
here to prevent /../ and the like.
> +
> + i = 0;
> + cmd[i++] = "/bin/mount";
> + cmd[i++] = "-t";
> + cmd[i++] = mtype;
> + if (mopts)
> {
> - if (argc < 5)
> - error ("devpts: not enough mount arguments");
> - /* see if we're mounting devpts to somewhere in rootsdir */
> - else if (strncmp (rootsdir, argv[5], strlen (rootsdir)) != 0)
> - error ("devpts: mount not allowed on %s", argv[5]);
> + cmd[i++] = "-o";
> + cmd[i++] = mopts;
> }
> - else
> - error ("unallowed mount type");
> + cmd[i++] = dev;
> + cmd[i++] = dir;
> + cmd[i++] = NULL;
>
> /* all checks passed, execute */
> - do_command ("/bin/mount", &(argv[1]), 0);
> + do_command ("/bin/mount", cmd, 0);
> }
>
> /* clean out a chroot dir */
>
>
>
>
>
>
>
> differences
> between files
> attachment
> (bind_dev.patch)
>
> diff -u mock.py mock.py
> --- mock.py 26 May 2006 01:12:29 -0000
> +++ mock.py 26 May 2006 01:09:15 -0000
> @@ -407,6 +409,8 @@
> mounts = [('proc','proc','proc',None)]
> if os.path.exists('/selinux'):
> mounts.append(('/selinux','selinux','none','bind'))
> + if self.config['bind_dev']:
> + mounts.append(('/dev','dev','none','bind'))
> #these have to come after /dev
> mounts.extend([
> ('pts','dev/pts','devpts', 'gid=5,mode=620'),
> @@ -601,24 +605,9 @@
> break
>
> return rpmUtils.miscutils.unique(reqlist)
> -
> - def _prep_install(self):
> - """prep chroot for installation"""
> - # make chroot dir
> - # make /dev, mount /proc
> - #
> - for item in [self.basedir, self.rootdir, self.statedir,
> self.resultdir,
> - os.path.join(self.rootdir, 'var/lib/rpm'),
> - os.path.join(self.rootdir, 'var/log'),
> - os.path.join(self.rootdir, 'dev'),
> - os.path.join(self.rootdir, 'etc/rpm'),
> - os.path.join(self.rootdir, 'tmp'),
> - os.path.join(self.rootdir, 'var/tmp'),
> - os.path.join(self.rootdir, 'etc/yum.repos.d')]:
> - self._ensure_dir(item)
> -
> - self._mount()
>
> + def _prep_dev(self):
> + """prep basic /dev contents in the chroot"""
> # we need stuff
> devices = [('null', 'c', '1', '3', '666'),
> ('urandom', 'c', '1', '9', '644'),
> @@ -641,7 +630,7 @@
> devpath = os.path.join(self.rootdir, 'dev/fd')
> if not os.path.exists(devpath):
> os.symlink('../proc/self/fd', devpath)
> -
> +
> fd = 0
> for item in ('stdin', 'stdout', 'stderr'):
> devpath = os.path.join(self.rootdir, 'dev', item)
> @@ -650,6 +639,27 @@
> os.symlink(fdpath, devpath)
> fd += 1
>
> + def _prep_install(self):
> + """prep chroot for installation"""
> + # make chroot dir
> + # make /dev, mount /proc
> + #
> + for item in [self.basedir, self.rootdir, self.statedir,
> self.resultdir,
> + os.path.join(self.rootdir, 'var/lib/rpm'),
> + os.path.join(self.rootdir, 'var/log'),
> + os.path.join(self.rootdir, 'dev'),
> + os.path.join(self.rootdir, 'etc/rpm'),
> + os.path.join(self.rootdir, 'tmp'),
> + os.path.join(self.rootdir, 'var/tmp'),
> + os.path.join(self.rootdir, 'etc/yum.repos.d')]:
> + self._ensure_dir(item)
> +
> + self._mount()
per previous email, I believe that do_chroot is a better place for
_mount(), but I suppose that could be a separate patch.
> +
> + if not self.config['bind_dev']:
> + #if we don't bind mount dev, at least provide the basics
> + self._prep_dev()
> +
> for item in [os.path.join(self.rootdir, 'etc', 'mtab'),
> os.path.join(self.rootdir, 'etc', 'fstab'),
> os.path.join(self.rootdir, 'var', 'log',
> 'yum.log')]:
> @@ -771,6 +781,8 @@
> parser.add_option("-r", action="store", type="string",
> dest="chroot",
> help="chroot name/config file name default: %default",
> default='default')
> + parser.add_option("--bind-dev", action ="store_true",
> + help="bind mount /dev in the chroot")
Why not have 'default=False' here?
> parser.add_option("--no-clean", action ="store_false",
> dest="clean",
> help="do not clean chroot before building", default=True)
> parser.add_option("--arch", action ="store", dest="arch",
> @@ -813,6 +825,7 @@
> config_opts['debug'] = False
> config_opts['quiet'] = False
> config_opts['target_arch'] = 'i386'
> + config_opts['bind_dev'] = False
> config_opts['files'] = {}
> config_opts['yum.conf'] = ''
> config_opts['macros'] = """
> @@ -856,6 +869,9 @@
> if options.uniqueext:
> config_opts['unique-ext'] = options.uniqueext
>
> + if options.bind_dev is not None:
> + config_opts['bind_dev'] = options.bind_dev
And then you can drop the conditional here.
--
Michael
More information about the Fedora-buildsys-list
mailing list