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