mock patch proposal: unmount everything mounted under chroot

[Mike McLean]

On 03/15/2010 09:29 AM, Alan Franzoni wrote:
> Hello,
> after getting some errors about unmount in mock, most probably caused
> by binfmt_misc issues - somewhere some package I was building did
> indeed trigger some "automagic" binfmt_misc mount under /proc/sys, I
> decided to patch mock to just unmount *all* directories that have been
> mounted under such chroot, no matter who mounted them or how they got
> mounted.

While I am a fan of extra-paranoid unmounting (as is anyone who has even 
run mock clean with an extra mount accidentally left in place), I think 
the implementation needs to be more robust.

To go a step further than your suggestion, I would like to see mock's 
util.rmtree altered to not cross filesystem boundaries.

Comments on the patch itself inline.

> --- /tmp/backend.py.orig	2010-03-15 12:10:30.000000000 +0100
> +++ /usr/lib/python2.6/site-packages/mock/backend.py	2010-03-15 14:16:38.000000000 +0100

When submitting a patch, it is best to make sure the patch is can be 
easily applied by the developers. Your patch references absolute 
pathnames which makes applying a hassle. Depending on invocation, patch 
might try to apply the change to the system's backend.py instead of the 
development copy, or simply fail to find a file to patch.

> @@ -85,14 +85,14 @@
>              self.pluginConf[key]['cache_topdir'] = self.cache_topdir
>              self.pluginConf[key]['cachedir'] = self.cachedir
>              self.pluginConf[key]['root'] = self.sharedRootName
> -
> -        # mount/umount
> -        self.umountCmds = ['umount -n %s' % self.makeChrootPath('proc'),
> -                'umount -n %s' % self.makeChrootPath('sys')
> -               ]
> +
      ^^^^ trailing whitespace

> +        # mount
>          self.mountCmds = ['mount -n -t proc   mock_chroot_proc   %s' % self.makeChrootPath('proc'),
>                  'mount -n -t sysfs  mock_chroot_sysfs  %s' % self.makeChrootPath('sys'),
>                 ]
> +        # retain umountCmds for interfacing sake, but they're not required
> +        # anymore
> +        self.umountCmds = []
>
>          self.build_log_fmt_str = config['build_log_fmt_str']
>          self.root_log_fmt_str = config['root_log_fmt_str']
> @@ -340,13 +340,6 @@
>
>          os.umask(prevMask)
>
> -        # mount/umount
> -        for devUnmtCmd in (
> -                'umount -n %s' % self.makeChrootPath('/dev/pts'),
> -                'umount -n %s' % self.makeChrootPath('/dev/shm') ):
> -            if devUnmtCmd not in self.umountCmds:
> -                self.umountCmds.append(devUnmtCmd)
> -
>          mountopt = 'gid=%d,mode=0620,ptmxmode=0666' % grp.getgrnam('tty').gr_gid
>          if kver >= '2.6.29':
>              mountopt += ',newinstance'
> @@ -587,9 +580,19 @@
>      decorate(traceLog())
>      def _umountall(self):
>          """umount all mounted chroot fs."""
> -        for cmd in self.umountCmds:
> -            self.root_log.debug(cmd)
> -            mock.util.do(cmd, raiseExc=0, shell=True)
> +        mountpoints = mock.util.do("cat /proc/mounts", raiseExc=0, shell=True, returnOutput=1)

You're reading /proc/mounts by running cat in a shell, which is a bit 
silly when python can just read the file itself.

> +        # umount in reverse mount order to prevent nested mount issues that
> +        # may prevent clean unmount.
> +        for mountline in reversed(mountpoints.split("\n")):
> +            try:
> +                mountpoint = mountline.split(" ")[1]
> +                if self.makeChrootPath("/") in mountpoint:
                           ^^
The test "self.makeChrootPath("/") in mountpoint" is not quite correct. 
It could generate false positives (granted this is unlikely), but more 
importantly, it could fail to detect a chroot mount when there are 
multiple ways to specify it (e.g. with symlinks in use). A more careful 
approach would:
   - use os.path.realpath() to canonicalize paths before comparing
   - retain mount data as a fallback

> +                    cmd = "umount -n %s" % mountpoint
> +                    self.root_log.debug(cmd)
> +                    mock.util.do(cmd, raiseExc=0, shell=True)
> +            except:
> +                # dont mind about wrong parsing, etc. just go on with the next
> +                continue

What are you trying to catch here? You passed raiseExc=0, so the command 
is not going to generate an error. What's left? Parsing the line from 
/proc/mounts? It looks like the only way that can fail is if 
/proc/mounts contains a malformed line. I'm not sure if that can even 
happen (and if it does I think it is worth noting instead of ignoring).

Also, unqualified except clauses should be used sparingly, if at all.

>
>      decorate(traceLog())
>      def _yum(self, cmd, returnOutput=0):