install: avoid several races with file lock

we should never release the lock until the entire read+write operation
is completed as the data might have changed between the read and the
write.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Atomic/util.py

@@ -865,47 +865,55 @@ def file_lock(path):
 # Records additional data for containers outside of the native storage (docker/ostree)
 class InstallData(object):
 
+    @classmethod
+    def read_install_data_locked(cls):
+        try:
+            with open(ATOMIC_INSTALL_JSON, 'r') as f:
+                # Backwards compatibility - we previously created an empty file explicitly;
+                # see https://github.com/projectatomic/atomic/pull/966
+                if os.fstat(f.fileno()).st_size == 0:
+                    return {}
+                data = json.load(f)
+                # Backwards compatibility - we supported only one container per image
+                # if the file is stored in the old format, then automatically convert
+                # it to the new format where each image name can refer to a list of
+                # installed containers.
+                for k, v in data.items():
+                    if isinstance(v, dict):
+                        data[k] = [v]
+                return data
+        except IOError as e:
+            if e.errno == errno.ENOENT:
+                return {}
+            raise e
+
     @classmethod
     def read_install_data(cls):
         with file_lock(ATOMIC_INSTALL_JSON):
-            try:
-                with open(ATOMIC_INSTALL_JSON, 'r') as f:
-                    # Backwards compatibility - we previously created an empty file explicitly;
-                    # see https://github.com/projectatomic/atomic/pull/966
-                    if os.fstat(f.fileno()).st_size == 0:
-                        return {}
-                    data = json.load(f)
-                    # Backwards compatibility - we supported only one container per image
-                    # if the file is stored in the old format, then automatically convert
-                    # it to the new format where each image name can refer to a list of
-                    # installed containers.
-                    for k, v in data.items():
-                        if isinstance(v, dict):
-                            data[k] = [v]
-                    return data
-            except IOError as e:
-                if e.errno == errno.ENOENT:
-                    return {}
-                raise e
+            return cls.read_install_data_locked()
+
+    @classmethod
+    def write_install_data_locked(cls, new_data, append=False):
+        install_data = cls.read_install_data_locked()
+        if not append:
+            install_data = new_data
+        else:
+            for k, v in new_data.items():
+                if k not in install_data:
+                    install_data[k] = []
+                install_data[k].append(v)
+        temp_file = tempfile.NamedTemporaryFile(mode='w', delete=False)
+
+        json.dump(install_data, temp_file)
+        temp_file.close()
+        if not os.path.exists(ATOMIC_VAR_LIB):
+            os.makedirs(ATOMIC_VAR_LIB)
+        shutil.move(temp_file.name, ATOMIC_INSTALL_JSON)
 
     @classmethod
     def write_install_data(cls, new_data, append=False):
-        install_data = cls.read_install_data()
         with file_lock(ATOMIC_INSTALL_JSON):
-            if not append:
-                install_data = new_data
-            else:
-                for k, v in new_data.items():
-                    if k not in install_data:
-                        install_data[k] = []
-                    install_data[k].append(v)
-            temp_file = tempfile.NamedTemporaryFile(mode='w', delete=False)
-
-            json.dump(install_data, temp_file)
-            temp_file.close()
-            if not os.path.exists(ATOMIC_VAR_LIB):
-                os.makedirs(ATOMIC_VAR_LIB)
-            shutil.move(temp_file.name, ATOMIC_INSTALL_JSON)
+            return cls.write_install_data_locked(new_data, append)
 
     @classmethod
     def get_install_data_by_id(cls, iid):
@@ -931,18 +939,19 @@ def get_install_name_by_id(cls, iid, install_data=None):
     @classmethod
     def delete_by_id(cls, iid, name, ignore=False):
         last_image = False
-        install_data = cls.read_install_data()
-        for installed_image in install_data:
-            containers = install_data[installed_image]
-            for index, container in enumerate(containers):
-                if container['id'] == iid and container['container_name'] == name:
-                    del containers[index]
-                    install_data[installed_image] = containers
-                    if len(containers) == 0:
-                        del install_data[installed_image]
-                        last_image = True
-                    cls.write_install_data(install_data)
-                    return last_image
+        with file_lock(ATOMIC_INSTALL_JSON):
+            install_data = cls.read_install_data_locked()
+            for installed_image in install_data:
+                containers = install_data[installed_image]
+                for index, container in enumerate(containers):
+                    if container['id'] == iid and container['container_name'] == name:
+                        del containers[index]
+                        install_data[installed_image] = containers
+                        if len(containers) == 0:
+                            del install_data[installed_image]
+                            last_image = True
+                        cls.write_install_data_locked(install_data)
+                        return last_image
         if not ignore:
             raise ValueError("Unable to find {} in the installed containers".format(iid))