diff --git a/imagecraft/pack/image.py b/imagecraft/pack/image.py index 04ecc756..92b76166 100644 --- a/imagecraft/pack/image.py +++ b/imagecraft/pack/image.py @@ -15,6 +15,7 @@ """Image handling.""" import contextlib +import fcntl import json from collections.abc import Generator, Iterator from pathlib import Path @@ -88,11 +89,17 @@ def attach_loopdev(self) -> Iterator[str]: "--partscan", self.disk_path, ).stdout.strip() - emit.debug( - f"Attached image {self.disk_path} as loop device {self.loop_device}" - ) try: - yield self.loop_device + # Acquire a shared BSD lock on the loop device. udev holds an + # exclusive lock while it processes the new device; taking a shared + # lock here blocks until udev is done and then holds it for the + # duration of the context so udev does not interfere with our use. + with open(self.loop_device, "rb") as loop_fd: + fcntl.flock(loop_fd, fcntl.LOCK_SH) + emit.debug( + f"Attached image {self.disk_path} as loop device {self.loop_device}" + ) + yield self.loop_device finally: self._detach_loopdevs() diff --git a/imagecraft/services/image.py b/imagecraft/services/image.py index 18bf48f7..83dff47f 100644 --- a/imagecraft/services/image.py +++ b/imagecraft/services/image.py @@ -16,13 +16,14 @@ import atexit import contextlib +import fcntl import json import pathlib import shutil import subprocess import time from collections.abc import Mapping -from typing import Any, cast +from typing import IO, Any, cast from craft_application import AppMetadata, AppService, ServiceFactory from craft_cli import CraftError, emit @@ -50,6 +51,7 @@ def __init__( self._sector_size = gptutil.SECTOR_SIZE_512 self._images: dict[str, pathlib.Path] | None = None self._loop_devices: dict[str, str] = {} + self._loop_fds: list[IO[bytes]] = [] self._atexit_registered = False def get_images(self) -> Mapping[str, pathlib.Path]: @@ -156,6 +158,18 @@ def attach_images(self) -> Mapping[str, str]: resolution="Ensure loop devices are available and you have sufficient permissions (sudo).", ) from err + # 3. Acquire a shared BSD lock on the loop device. udev holds an + # exclusive lock while it processes the device; a shared lock here + # blocks until udev is done and then holds it while we use the + # device's partitions, preventing udev from interfering. + loop_fd = open(attached_device, "rb") # noqa: SIM115 (held deliberately) + try: + fcntl.flock(loop_fd, fcntl.LOCK_SH) + except Exception: + loop_fd.close() + raise + self._loop_fds.append(loop_fd) + self._loop_devices[name] = attached_device if not self._atexit_registered: @@ -169,6 +183,12 @@ def detach_images(self) -> None: Includes a retry loop for busy devices. Safe to call as an atexit handler. """ + # Release shared flocks so the devices are no longer considered in use. + for fd in self._loop_fds: + with contextlib.suppress(Exception): + fd.close() + self._loop_fds.clear() + for name, device in list(self._loop_devices.items()): success = False start_time = time.monotonic() diff --git a/tests/unit/pack/test_image.py b/tests/unit/pack/test_image.py index ba441213..0fe103c8 100644 --- a/tests/unit/pack/test_image.py +++ b/tests/unit/pack/test_image.py @@ -17,6 +17,7 @@ from typing import Any from unittest.mock import call +import fcntl import pytest from imagecraft.models import Volume from imagecraft.pack.image import Image @@ -48,6 +49,8 @@ def run(cmd: str, *args: Any, **kwargs: Any) -> CompletedProcess[str]: class TestImage: def test_loopdev(self, mocker, new_dir: Path): mock_run = mocker.patch("imagecraft.pack.image.run", side_effect=run) + mock_flock = mocker.patch("fcntl.flock") + mocker.patch("builtins.open", mocker.mock_open()) volume = Volume.unmarshal( { @@ -78,6 +81,7 @@ def test_loopdev(self, mocker, new_dir: Path): call("losetup", "--json"), call("losetup", "-d", "/dev/loop99"), ] + mock_flock.assert_called_once_with(mocker.ANY, fcntl.LOCK_SH) @pytest.mark.parametrize( ("volume_data", "has_data_partition"), @@ -248,3 +252,4 @@ def test_has_boot_partition( ) assert image.has_boot_partition == has_boot_partition + diff --git a/tests/unit/services/test_image.py b/tests/unit/services/test_image.py index 221d84af..dfcadd6b 100644 --- a/tests/unit/services/test_image.py +++ b/tests/unit/services/test_image.py @@ -12,6 +12,7 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . +import fcntl import subprocess from unittest.mock import MagicMock, patch @@ -104,6 +105,8 @@ def test_attach_images_new(image_service, project_dir, mocker): mocker.patch.object(image_service, "_get_all_loop_devices", return_value=[]) mock_run.return_value.stdout = "/dev/loop8\n" + mock_flock = mocker.patch("fcntl.flock") + mocker.patch("builtins.open", return_value=mocker.MagicMock()) with patch("atexit.register") as mock_atexit: devices = image_service.attach_images() @@ -118,6 +121,8 @@ def test_attach_images_new(image_service, project_dir, mocker): ) mock_atexit.assert_called_once_with(image_service.detach_images) + mock_flock.assert_called_once_with(mocker.ANY, fcntl.LOCK_SH) + def test_attach_images_reuse(image_service, project_dir, mocker): image_path = project_dir / ".pc.img.tmp" @@ -134,12 +139,16 @@ def test_attach_images_reuse(image_service, project_dir, mocker): # Mock samefile to return True mocker.patch("pathlib.Path.samefile", return_value=True) mock_run = mocker.patch("imagecraft.services.image.run") + mock_flock = mocker.patch("fcntl.flock") + mocker.patch("builtins.open", return_value=mocker.MagicMock()) devices = image_service.attach_images() assert devices == {"pc": "/dev/loop9"} mock_run.assert_not_called() # Should not call losetup attach + mock_flock.assert_called_once_with(mocker.ANY, fcntl.LOCK_SH) + def test_attach_images_stale_inode(image_service, project_dir, mocker): image_path = project_dir / ".pc.img.tmp" @@ -157,6 +166,8 @@ def test_attach_images_stale_inode(image_service, project_dir, mocker): mocker.patch("pathlib.Path.samefile", side_effect=FileNotFoundError) mock_run = mocker.patch("imagecraft.services.image.run") mock_run.return_value.stdout = "/dev/loop11\n" + mocker.patch("fcntl.flock") + mocker.patch("builtins.open", return_value=mocker.MagicMock()) devices = image_service.attach_images() @@ -169,6 +180,30 @@ def test_attach_images_stale_inode(image_service, project_dir, mocker): ) +def test_attach_images_flock_released_on_detach(image_service, project_dir, mocker): + """attach_images() holds a shared flock; detach_images() closes the fd to release it.""" + image_service._images = {"pc": project_dir / ".pc.img.tmp"} + + mocker.patch.object(image_service, "_get_all_loop_devices", return_value=[]) + mock_run = mocker.patch("imagecraft.services.image.run") + mock_run.return_value.stdout = "/dev/loop8\n" + mocker.patch("fcntl.flock") + mock_fd = mocker.MagicMock() + mocker.patch("builtins.open", return_value=mock_fd) + + with patch("atexit.register"): + image_service.attach_images() + + assert len(image_service._loop_fds) == 1 + + # After detach the fd should be closed and the list cleared + mock_run.return_value = mocker.MagicMock() # losetup -d + image_service.detach_images() + + mock_fd.close.assert_called_once() + assert image_service._loop_fds == [] + + def test_detach_images_success(image_service, mocker): image_service._loop_devices = {"pc": "/dev/loop8"} mock_run = mocker.patch("imagecraft.services.image.run")