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 -- Fedora-buildsys-list mailing list Fedora-buildsys-list@redhat.com https://www.redhat.com/mailman/listinfo/fedora-buildsys-list