Merge lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/handle-invalid-removed-file into lp:ubuntu/vivid/ubuntu-core-upgrader

[James Hunt]

* ubuntucoreupgrader/upgrader.py: Tolerate an invalid 'removed' file
  to avoid the upgrade failing attempting to remove '/writable/cache'
  (see LP: #1437225).

[Michael Vogt (mvo)]

Thanks for adding this bugfix. I think we should also include a regression test.

[James Hunt (jamesodhunt)]

Tests added.

[Michael Vogt (mvo)]

Thanks for adding the test. It runs fine.

One thing that is a odd is that if I revert the change in r23 (the bugfix for the empty file) and run the tests they still succeed. Is this intended?

[James Hunt (jamesodhunt)]

Branch updated. Phew...

[Michael Vogt (mvo)]

Thanks, I put some suggestions inline.

[lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/handle-invalid-removed-file]

26. By James Hunt on 2015-04-08

* Review changes.

[James Hunt (jamesodhunt)]

Thanks for reviewing - branch updated.

[Michael Vogt (mvo)]

Thanks, looks good, one question about the strip() below seems to have slipped through, would be nice to get this clarified in some way. Otherwise fine and ready to go.

review: Needs Information

[Michael Vogt (mvo)]

I merged/resolved-conflicts/uploaded as per Steve.

=== modified file 'debian/changelog'
--- debian/changelog	2015-03-13 08:14:06 +0000
+++ debian/changelog	2015-04-08 14:54:30 +0000
@@ -1,3 +1,12 @@
+ubuntu-core-upgrader (0.7.8) UNRELEASED; urgency=medium
+
+  * ubuntucoreupgrader/upgrader.py: Tolerate an invalid 'removed' file
+    to avoid the upgrade failing attempting to remove '/writable/cache'
+    (see LP: #1437225).
+  * functional/test_upgrader.py: Add tests for an invalid removed file.
+
+ -- James Hunt <[e-mail]>  Fri, 27 Mar 2015 09:33:15 +0000
+
 ubuntu-core-upgrader (0.7.7) vivid; urgency=low
 
   * fix entry point for s-i 3.0

=== modified file 'functional/test_upgrader.py'
--- functional/test_upgrader.py	2015-03-04 16:27:30 +0000
+++ functional/test_upgrader.py	2015-04-08 14:54:30 +0000
@@ -25,7 +25,6 @@
 import os
 import logging
 import unittest
-import shutil
 
 from ubuntucoreupgrader.upgrader import (
     Upgrader,
@@ -37,7 +36,6 @@
 sys.path.append(base_dir)
 
 from ubuntucoreupgrader.tests.utils import (
-    make_tmp_dir,
     append_file,
     TEST_DIR_MODE,
     create_file,
@@ -68,21 +66,11 @@
     args.append(command_file)
     commands = file_to_list(command_file)
 
-    cache_dir = make_tmp_dir()
-
-    def mock_get_cache_dir():
-        cache_dir = update.tmp_dir
-        sys_dir = os.path.join(cache_dir, 'system')
-        os.makedirs(sys_dir, exist_ok=True)
-        return cache_dir
-
     upgrader = Upgrader(parse_args(args), commands, [])
-    upgrader.get_cache_dir = mock_get_cache_dir
+    upgrader.cache_dir = root_dir
     upgrader.MOUNTPOINT_CMD = "true"
     upgrader.run()
 
-    shutil.rmtree(cache_dir)
-
 
 def create_device_file(path, type='c', major=-1, minor=-1):
     '''
@@ -216,7 +204,10 @@
         self.assertTrue(os.path.exists(file_path))
         self.assertTrue(os.path.isfile(file_path))
 
-        call_upgrader(cmd_file, self.victim_dir, self.update)
+        # remove 'system' suffix that upgrader will add back on
+        vdir = os.path.split(self.victim_dir)[0]
+
+        call_upgrader(cmd_file, vdir, self.update)
 
         self.assertFalse(os.path.exists(file_path))
 
@@ -240,7 +231,8 @@
         self.assertTrue(os.path.exists(dir_path))
         self.assertTrue(os.path.isdir(dir_path))
 
-        call_upgrader(cmd_file, self.victim_dir, self.update)
+        vdir = os.path.split(self.victim_dir)[0]
+        call_upgrader(cmd_file, vdir, self.update)
 
         self.assertFalse(os.path.exists(dir_path))
 
@@ -272,7 +264,8 @@
         self.assertTrue(os.path.exists(link_file_path))
         self.assertTrue(os.path.islink(link_file_path))
 
-        call_upgrader(cmd_file, self.victim_dir, self.update)
+        vdir = os.path.split(self.victim_dir)[0]
+        call_upgrader(cmd_file, vdir, self.update)
 
         # original file should still be there
         self.assertTrue(os.path.exists(src_file_path))
@@ -310,7 +303,8 @@
         self.assertTrue(os.path.exists(link_file_path))
         self.assertTrue(os.path.islink(link_file_path))
 
-        call_upgrader(cmd_file, self.victim_dir, self.update)
+        vdir = os.path.split(self.victim_dir)[0]
+        call_upgrader(cmd_file, vdir, self.update)
 
         # original directory should still be there
         self.assertTrue(os.path.exists(src_dir_path))
@@ -352,7 +346,8 @@
 
         self.assertTrue(src_inode == link_inode)
 
-        call_upgrader(cmd_file, self.victim_dir, self.update)
+        vdir = os.path.split(self.victim_dir)[0]
+        call_upgrader(cmd_file, vdir, self.update)
 
         # original file should still be there
         self.assertTrue(os.path.exists(src_file_path))
@@ -392,9 +387,11 @@
         # device because it won't be :)
         self.assertTrue(os.path.isfile(file_path))
 
-        call_upgrader(cmd_file, self.victim_dir, self.update)
+        vdir = os.path.split(self.victim_dir)[0]
+        call_upgrader(cmd_file, vdir, self.update)
 
-        self.assertFalse(os.path.exists(file_path))
+        # upgrader doesn't currently remove device files
+        self.assertTrue(os.path.exists(file_path))
 
 
 class UpgraderFileAddTestCase(UbuntuCoreUpgraderTestCase):
@@ -595,6 +592,67 @@
         self.assertTrue(is_sym_link_broken(victim_link_file_path))
 
 
+class UpgraderRemoveFileTests(UbuntuCoreUpgraderTestCase):
+
+    def common_removed_file_test(self, contents):
+        '''
+        Common code to test for an invalid removed file.
+
+        The contents parameter specifies the contents of the removed
+        file.
+        '''
+        file = 'created-regular-file'
+
+        create_file(self.update.removed_file, contents)
+
+        file_path = os.path.join(self.update.system_dir, file)
+        create_file(file_path, 'foo bar')
+
+        archive = self.update.create_archive(self.TARFILE)
+        self.assertTrue(os.path.exists(archive))
+
+        cmd_file = os.path.join(self.update.tmp_dir, CMD_FILE)
+        make_command_file(cmd_file, [self.TARFILE])
+
+        vdir = os.path.split(self.victim_dir)[0]
+
+        file_path = os.path.join(vdir, file)
+        self.assertFalse(os.path.exists(file_path))
+
+        # XXX: There is an implicit test here since if the upgrader
+        # fails (as documented on LP: #1437225), this test will also
+        # fail.
+        call_upgrader(cmd_file, vdir, self.update)
+
+        self.assertTrue(os.path.exists(vdir))
+        self.assertTrue(os.path.exists(self.victim_dir))
+        self.assertTrue(os.path.exists(file_path))
+
+        # ensure the empty removed file hasn't removed the directory the
+        # unpack applies to.
+        self.assertTrue(self.victim_dir)
+
+    def test_removed_file_empty(self):
+        '''
+        Ensure the upgrader ignores an empty 'removed' file.
+        '''
+        self.common_removed_file_test('')
+
+    def test_removed_file_space(self):
+        '''
+        Ensure the upgrader handles a 'removed' file containing just a
+        space.
+        '''
+        self.common_removed_file_test(' ')
+
+    def test_removed_file_nl(self):
+        '''
+        Ensure the upgrader handles a 'removed' file containing just a
+        newline
+        '''
+        self.common_removed_file_test('\n')
+
+
 def main():
     kwargs = {}
     format =             \

=== modified file 'ubuntucoreupgrader/tests/test_upgrader.py'
--- ubuntucoreupgrader/tests/test_upgrader.py	2015-03-04 16:50:08 +0000
+++ ubuntucoreupgrader/tests/test_upgrader.py	2015-04-08 14:54:30 +0000
@@ -196,15 +196,6 @@
 
 class UbuntuCoreUpgraderObectTestCase(UbuntuCoreUpgraderTestCase):
 
-    def mock_get_cache_dir(self):
-        '''
-        Mock to allow the tests to control the cache directory location.
-        '''
-        assert(self.cache_dir)
-        self.sys_dir = os.path.join(self.cache_dir, 'system')
-        os.makedirs(self.sys_dir, exist_ok=True)
-        return self.cache_dir
-
     def test_create_object(self):
         root_dir = make_tmp_dir()
 
@@ -233,7 +224,6 @@
         options.root_dir = root_dir
 
         upgrader = Upgrader(options, commands, [])
-        upgrader.get_cache_dir = self.mock_get_cache_dir
         upgrader.MOUNTPOINT_CMD = "true"
         upgrader.run()
 
@@ -242,6 +232,52 @@
 
         shutil.rmtree(root_dir)
 
+    def test_empty_removed_file(self):
+        root_dir = make_tmp_dir()
+
+        file = 'created-regular-file'
+
+        file_path = os.path.join(self.update.system_dir, file)
+        create_file(file_path, 'foo bar')
+
+        self.cache_dir = self.update.tmp_dir
+
+        removed_file = self.update.removed_file
+        # Create an empty removed file
+        create_file(removed_file, '')
+
+        archive = self.update.create_archive(self.TARFILE)
+        self.assertTrue(os.path.exists(archive))
+
+        dest = os.path.join(self.cache_dir, os.path.basename(archive))
+        touch_file('{}.asc'.format(dest))
+
+        commands = make_commands([self.TARFILE])
+
+        options = make_default_options()
+
+        # XXX: doesn't actually exist, but the option must be set since
+        # the upgrader looks for the update archives in the directory
+        # where this file is claimed to be.
+        options.cmdfile = os.path.join(self.cache_dir, 'ubuntu_command')
+
+        options.root_dir = root_dir
+
+        upgrader = Upgrader(options, commands, [])
+        upgrader.cache_dir = self.cache_dir
+        upgrader.MOUNTPOINT_CMD = "true"
+
+        si_file = self.TARFILE
+        si_signature = "{}.asc".format(self.TARFILE)
+        upgrader._cmd_update([si_file, si_signature])
+
+        # Ensure that the upgrader has not attempted to remove the
+        # cache_dir when the 'removed' file is empty.
+        #
+        # (Regression test for LP: #1437225).
+        self.assertTrue(os.path.exists(upgrader.cache_dir))
+
+        shutil.rmtree(self.cache_dir)
 
 if __name__ == "__main__":
     unittest.main()

=== modified file 'ubuntucoreupgrader/tests/utils.py'
--- ubuntucoreupgrader/tests/utils.py	2015-03-04 16:50:08 +0000
+++ ubuntucoreupgrader/tests/utils.py	2015-04-08 14:54:30 +0000
@@ -119,7 +119,8 @@
         '''
         # all files listed in the removed list must be system files
         final = list(map(lambda a:
-                     '{}{}'.format(self.TEST_SYSTEM_DIR, a), removed_files))
+                     os.path.normpath('{}{}'.format(self.TEST_SYSTEM_DIR,
+                                                    a)), removed_files))
 
         contents = "".join(final)
         append_file(self.removed_file, contents)
@@ -194,7 +195,7 @@
 
        This creates 2 temporary directories:
 
-       - self.system dir: Used as to generate an update archive from.
+       - self.system_dir: Used to generate an update archive from.
 
        - self.tmp_dir: Used to write the generated archive file to. The
          intention is that this directory should also be used to hold
@@ -244,7 +245,9 @@
 
         # The directory which will have the update archive applied to
         # it.
-        self.victim_dir = make_tmp_dir(tag='victim')
+        self.victim_dir_base = make_tmp_dir(tag='victim')
+        self.victim_dir = os.path.join(self.victim_dir_base, 'system')
+        os.makedirs(self.victim_dir, mode=0o750)
 
     def tearDown(self):
         '''

=== modified file 'ubuntucoreupgrader/upgrader.py'
--- ubuntucoreupgrader/upgrader.py	2015-03-05 13:24:40 +0000
+++ ubuntucoreupgrader/upgrader.py	2015-04-08 14:54:30 +0000
@@ -472,29 +472,26 @@
         # Note: Only used by UPGRADE_IN_PLACE.
         self.other_is_empty = False
 
+        # cache_dir is used as a scratch pad, for downloading new images
+        # to and bind mounting the rootfs.
+        if self.options.tmpdir:
+            self.cache_dir = self.options.tmpdir
+        else:
+            self.cache_dir = self.DEFAULT_CACHE_DIR
+
     def update_timestamp(self):
         '''
         Update the timestamp file to record the time the last upgrade
         completed successfully.
         '''
-        file = os.path.join(self.get_cache_dir(), self.TIMESTAMP_FILE)
+        file = os.path.join(self.cache_dir, self.TIMESTAMP_FILE)
         open(file, 'w').close()
 
-    def get_cache_dir(self):
-        '''
-        Returns the full path to the cache directory, which is used as a
-        scratch pad, for downloading new images to and bind mounting the
-        rootfs.
-        '''
-        return self.options.tmpdir \
-            if self.options.tmpdir \
-            else self.DEFAULT_CACHE_DIR
-
     def get_mount_target(self):
         '''
         Get the full path to the mount target directory.
         '''
-        return os.path.join(self.get_cache_dir(), self.MOUNT_TARGET)
+        return os.path.join(self.cache_dir, self.MOUNT_TARGET)
 
     def prepare(self):
         '''
@@ -740,13 +737,17 @@
             to_remove = []
 
         if not self.full_image:
-
             if found_removed_file:
                 log.debug('processing {} file'
                           .format(self.removed_file))
 
                 # process backwards to work around bug LP:#1381134.
                 for remove in sorted(to_remove, reverse=True):
+
+                    if not remove:
+                        # handle invalid removed file entry (see LP:#1437225)
+                        continue
+
                     remove = remove.strip()
 
                     # don't remove devices
@@ -762,13 +763,7 @@
                     if '../' in remove:
                         continue
 
-                    if self.options.root_dir == '/':
-                        final = os.path.join(self.get_cache_dir(), remove)
-                    else:
-                        # os.path.join() refuses to work if the file
-                        # begins with a slash.
-                        base = remove_prefix(remove)
-                        final = '{}{}'.format(self.options.root_dir, base)
+                    final = os.path.join(self.cache_dir, remove)
 
                     if not os.path.exists(final):
                         # This scenario can only mean there is a bug
@@ -804,9 +799,9 @@
             from unittest.mock import patch
             with patch("grp.getgrnam") as m:
                 m.side_effect = KeyError()
-                tar.extractall(path=self.get_cache_dir(),
+                tar.extractall(path=self.cache_dir,
                                members=tar_generator(
-                                   tar, self.get_cache_dir(),
+                                   tar, self.cache_dir,
                                    self.removed_file,
                                    self.options.root_dir))
         tar.close()
@@ -821,7 +816,7 @@
         '''
         target = self.get_mount_target()
         bindmount_rootfs_dir = tempfile.mkdtemp(prefix=script_name,
-                                                dir=self.get_cache_dir())
+                                                dir=self.cache_dir)
         bind_mount("/", bindmount_rootfs_dir)
 
         cwd = os.getcwd()