fix(images): reconcile the image inventory with filesystem changes

gns3server/utils/image_inventory.py

@@ -0,0 +1,96 @@
+"""Filesystem primitives shared by image writers and inventory reconciliation."""
+
+import asyncio
+import hashlib
+import os
+import stat
+
+from gns3server.config import Config
+
+
+def normalized_path(path):
+    return os.path.normcase(os.path.abspath(os.path.expanduser(path)))
+
+
+def contained_path(path, root):
+    try:
+        return os.path.commonpath((normalized_path(path), normalized_path(root))) == normalized_path(root)
+    except ValueError:
+        return False
+
+
+def fingerprint(path):
+    info = os.stat(path, follow_symlinks=False)
+    if not stat.S_ISREG(info.st_mode):
+        raise OSError(f"Not a regular image file: {path}")
+    return stat_fingerprint(info)
+
+
+def stat_fingerprint(info):
+    # Store as text: inode/device numbers need not fit a signed SQL BIGINT.
+    return ":".join(str(value) for value in (
+        info.st_dev, info.st_ino, info.st_size, info.st_mtime_ns, info.st_ctime_ns))
+
+
+class ImageLockBusy(Exception):
+    pass
+
+
+class ImageLock:
+    """Advisory lock shared by controller processes using the same config directory.
+
+    Lock files are deliberately retained: unlinking them permits two processes to
+    lock different inodes for the same name. OS locks are released on process exit.
+    """
+
+    def __init__(self, key, wait=True):
+        self.key = key
+        self.wait = wait
+        self._file = None
+
+    async def __aenter__(self):
+        directory = os.path.join(Config.instance().config_dir, ".image-locks")
+        os.makedirs(directory, exist_ok=True)
+        path = os.path.join(directory, hashlib.sha256(self.key.encode()).hexdigest() + ".lock")
+        self._file = open(path, "a+b")
+        if os.name == "nt" and os.fstat(self._file.fileno()).st_size == 0:
+            self._file.write(b"\0")
+            self._file.flush()
+        try:
+            while True:
+                try:
+                    if os.name == "nt":
+                        import msvcrt
+                        self._file.seek(0)
+                        msvcrt.locking(self._file.fileno(), msvcrt.LK_NBLCK, 1)
+                    else:
+                        import fcntl
+                        fcntl.flock(self._file, fcntl.LOCK_EX | fcntl.LOCK_NB)
+                    return self
+                except (BlockingIOError, PermissionError):
+                    if not self.wait:
+                        raise ImageLockBusy(self.key)
+                    await asyncio.sleep(0.05)
+        except BaseException:
+            self._file.close()
+            self._file = None
+            raise
+
+    async def __aexit__(self, *args):
+        if self._file is not None:
+            self._file.close()
+            self._file = None
+
+
+def image_lock(path):
+    return ImageLock("image:" + normalized_path(os.path.realpath(path)))
+
+
+def publish_image(temporary, destination):
+    """Atomically publish a complete file without replacing an existing image.
+
+    Both paths must be on the same filesystem. Hard linking provides the atomic
+    no-overwrite operation that os.replace()/shutil.move() cannot provide.
+    """
+    os.link(temporary, destination)
+    os.unlink(temporary)

gns3server/utils/images.py

@@ -18,7 +18,10 @@ import os
 import hashlib
 import stat
 import aiofiles
-import shutil
+import asyncio
+import tempfile
+
+from gns3server.utils.image_inventory import fingerprint, stat_fingerprint, image_lock, publish_image, contained_path
 
 try:
     import importlib_resources
@@ -123,31 +126,40 @@ def get_builtin_disks() -> List[str]:
     return builtin_disks
 
 
-async def read_image_info(path: str, expected_image_type: str = None) -> dict:
+def inspect_image_file(path, expected_image_type=None, allow_raw_image=False, stopped_event=None):
+    """Read a stable regular file once, never trusting checksum sidecars."""
+    before = fingerprint(path)
+    flags = os.O_RDONLY | getattr(os, "O_BINARY", 0) | getattr(os, "O_NOFOLLOW", 0)
+    with os.fdopen(os.open(path, flags), "rb") as f:
+        info = os.fstat(f.fileno())
+        if not stat.S_ISREG(info.st_mode) or stat_fingerprint(info) != before:
+            raise ImageChangedError(f"Image changed while opening: {path}")
+        header = f.read(7)
+        if len(header) < 7:
+            raise InvalidImageError(f"Image '{path}' is too small to be valid")
+        image_type = check_valid_image_header(path, header, allow_raw_image)
+        if expected_image_type and image_type != expected_image_type:
+            raise InvalidImageError(f"Detected image type for '{path}' is {image_type}, expected {expected_image_type}")
+        digest = hashlib.md5(header)
+        while True:
+            if stopped_event is not None and stopped_event.is_set():
+                raise InterruptedError("Image inspection cancelled")
+            chunk = f.read(1024 * 1024)
+            if not chunk:
+                break
+            digest.update(chunk)
+        if stat_fingerprint(os.fstat(f.fileno())) != before or fingerprint(path) != before:
+            raise ImageChangedError(f"Image changed while reading: {path}")
+    return dict(image_name=os.path.basename(path), image_type=image_type,
+                image_size=info.st_size, path=path, checksum=digest.hexdigest(),
+                checksum_algorithm="md5", file_fingerprint=before)
 
-    header_magic_len = 7
+
+async def read_image_info(path: str, expected_image_type: str = None, allow_raw_image=False) -> dict:
     try:
-        async with aiofiles.open(path, "rb") as f:
-            image_header = await f.read(header_magic_len)  # read the first 7 bytes of the file
-            if len(image_header) >= header_magic_len:
-                detected_image_type = check_valid_image_header(path, image_header)
-                if expected_image_type and detected_image_type != expected_image_type:
-                    raise InvalidImageError(f"Detected image type for '{path}' is {detected_image_type}, "
-                                            f"expected type is {expected_image_type}")
-            else:
-                raise InvalidImageError(f"Image '{path}' is too small to be valid")
+        return await asyncio.to_thread(inspect_image_file, path, expected_image_type, allow_raw_image)
     except OSError as e:
-        raise InvalidImageError(f"Cannot read image '{path}': {e}")
-
-    image_info = {
-        "image_name": os.path.basename(path),
-        "image_type": detected_image_type,
-        "image_size": os.stat(path).st_size,
-        "path": path,
-        "checksum": await wait_run_in_executor(md5sum, path, cache_to_md5file=False),
-        "checksum_algorithm": "md5",
-    }
-    return image_info
+        raise InvalidImageError(f"Cannot read image '{path}': {e}") from e
 
 
 async def discover_images(image_type: str, skip_image_paths: list = None) -> List[dict]:
@@ -239,7 +251,7 @@ def images_directories(image_type, include_parent_directory=True):
     return [force_unix_path(p) for p in paths if os.path.exists(p)]
 
 
-def md5sum(path, working_dir=None, stopped_event=None, cache_to_md5file=True):
+def md5sum(path, working_dir=None, stopped_event=None, cache_to_md5file=True, use_cache=True):
     """
     Return the md5sum of an image and cache it on disk
 
@@ -258,7 +270,7 @@ def md5sum(path, working_dir=None, stopped_event=None, cache_to_md5file=True):
     else:
         md5sum_file = path + ".md5sum"
 
-    if os.path.exists(md5sum_file):
+    if use_cache and os.path.exists(md5sum_file):
         try:
             with open(md5sum_file) as f:
                 md5 = f.read().strip()
@@ -315,6 +327,10 @@ class InvalidImageError(Exception):
         return self._message
 
 
+class ImageChangedError(InvalidImageError):
+    """An observation must be retried because the file is still changing."""
+
+
 def check_valid_image_header(path: str, data: bytes, allow_raw_image: bool = False) -> str:
 
     if data[:7] == b'\x7fELF\x01\x02\x01':
@@ -343,57 +359,53 @@ async def write_image(
 ) -> models.Image:
 
     image_dir, image_name = os.path.split(image_filename)
-    # Store the file under its final name only when the upload is completed
-    tmp_path = image_path + ".tmp"
-    log.info(f"Writing image file to '{tmp_path}'")
+    # HTTP chunk boundaries need not align with the seven-byte image header.
+    iterator = stream.__aiter__()
+    prefix = bytearray()
+    async for chunk in iterator:
+        prefix.extend(chunk)
+        if len(prefix) >= 7:
+            break
+    if len(prefix) < 7:
+        raise InvalidImageError("The image content is empty or too small to be valid")
+    image_type = check_valid_image_header(image_path, prefix, allow_raw_image or not check_image_header)
+    if not image_dir:
+        image_path = os.path.abspath(os.path.join(default_images_directory(image_type), image_name))
+        root = os.path.realpath(os.path.expanduser(Config.instance().settings.Server.images_path))
+        if not contained_path(os.path.realpath(image_path), root):
+            raise InvalidImageError(f"Image destination is outside the configured image directory: {image_path}")
     os.makedirs(os.path.dirname(image_path), exist_ok=True)
+    descriptor, tmp_path = tempfile.mkstemp(prefix=".gns3-upload-", suffix=".tmp", dir=os.path.dirname(image_path))
+    os.close(descriptor)
     checksum = hashlib.md5()
-    header_magic_len = 7
-    image_type = None
-    image_size = 0
     try:
         async with aiofiles.open(tmp_path, "wb") as f:
-            async for chunk in stream:
-                if check_image_header and len(chunk) >= header_magic_len:
-                    check_image_header = False
-                    image_type = check_valid_image_header(image_path, chunk, allow_raw_image)
+            await f.write(prefix)
+            checksum.update(prefix)
+            async for chunk in iterator:
                 await f.write(chunk)
                 checksum.update(chunk)
-
         image_size = os.path.getsize(tmp_path)
-        if not image_size or image_size < header_magic_len:
-            raise InvalidImageError("The image content is empty or too small to be valid")
-
-        if not image_dir:
-            directory = default_images_directory(image_type)
-            os.makedirs(directory, exist_ok=True)
-            image_path = os.path.abspath(os.path.join(directory, image_filename))
-
-        if os.path.exists(image_path):
-            raise InvalidImageError(f"File '{image_path}' already exists, "
-                                    f"please choose a different name or remove the existing image")
-
-        checksum = checksum.hexdigest()
-        image_dir = os.path.dirname(image_path)
-        duplicate_image = await images_repo.get_image_by_checksum(checksum, image_dir)
-        if duplicate_image:
-            raise InvalidImageError(f"Image '{duplicate_image.filename}' with the "
-                                    f"same checksum already exists in '{image_dir}'")
-
-        shutil.move(tmp_path, image_path)
-        os.chmod(image_path, stat.S_IWRITE | stat.S_IREAD | stat.S_IEXEC)
+        async with image_lock(image_path):
+            if os.path.lexists(image_path):
+                raise InvalidImageError(f"File '{image_path}' already exists, "
+                                        f"please choose a different name or remove the existing image")
+            checksum = checksum.hexdigest()
+            duplicate_image = await images_repo.get_image_by_checksum(checksum, os.path.dirname(image_path))
+            if duplicate_image:
+                raise InvalidImageError(f"Image '{duplicate_image.filename}' with the same checksum "
+                                        f"already exists in '{os.path.dirname(image_path)}'")
+            os.chmod(tmp_path, stat.S_IWRITE | stat.S_IREAD | stat.S_IEXEC)
+            publish_image(tmp_path, image_path)
+            # Complete files survive a database failure so the next scan can
+            # recover them. Never compensate by unlinking a published image.
+            return await images_repo.save_verified_image(dict(
+                image_name=image_name, image_type=image_type, image_size=image_size,
+                path=image_path, checksum=checksum, checksum_algorithm="md5",
+                file_fingerprint=fingerprint(image_path)))
     finally:
         try:
             if os.path.exists(tmp_path):
                 os.remove(tmp_path)
         except OSError:
             log.warning(f"Could not remove '{tmp_path}'")
-
-    return await images_repo.add_image(
-        image_name,
-        image_type,
-        image_size,
-        image_path,
-        checksum,
-        checksum_algorithm="md5"
-    )