Repository: osbuild/osbuild
Pull request #1842 (2024-08-13 16:43:00)

devices/loopback: add read-only option
It's sometimes useful to set up a loop device for an already formatted disk/filesystem image to derive new artifacts from it. In that case, we want to make sure it's impossible to modify its contents in any way in that process, both for our own purposes and for other stages operating on it.

Notably, mounting some filesystems read-only still seem to touch the disk (like XFS).

github-actions[bot]: This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.
mvo5: This has some test failures right now, I can try to look at those later in the week if it's okay that I push to this branch.
jlebon: @mvo5 Thanks! I think I fixed the source of the error, but if there's anything else needed, feel free to push.

Patch:
diff --git a/devices/org.osbuild.loopback b/devices/org.osbuild.loopback
index b98ddfea..b19f147d 100755
--- a/devices/org.osbuild.loopback
+++ b/devices/org.osbuild.loopback
@@ -58,6 +58,10 @@ SCHEMA = """
   "lock": {
     "type": "boolean",
     "description": "Lock the device after opening it"
+  },
+  "read-only": {
+    "type": "boolean",
+    "description": "Set up the device as read-only"
   }
 }
 """
@@ -77,7 +81,7 @@ class LoopbackService(devices.DeviceService):
         lock = UdevInhibitor.for_device(lo.LOOP_MAJOR, lo.minor)
         lo.on_close = lambda _l: lock.release()
 
-    def make_loop(self, fd: int, offset, sizelimit, lock, partscan):
+    def make_loop(self, fd: int, offset, sizelimit, lock, partscan, read_only):
         if not sizelimit:
             sizelimit = os.fstat(fd).st_size - offset
         else:
@@ -89,6 +93,7 @@ class LoopbackService(devices.DeviceService):
                                   sizelimit=sizelimit,
                                   blocksize=self.sector_size,
                                   partscan=partscan,
+                                  read_only=read_only,
                                   autoclear=True)
 
         return lo
@@ -100,12 +105,13 @@ class LoopbackService(devices.DeviceService):
         size = options.get("size")
         lock = options.get("lock", False)
         partscan = options.get("partscan", False)
+        read_only = options.get("read-only", False)
 
         path = os.path.join(tree, filename.lstrip("/"))
 
         self.fd = os.open(path, os.O_RDWR | os.O_CLOEXEC)
         try:
-            self.lo = self.make_loop(self.fd, start, size, lock, partscan)
+            self.lo = self.make_loop(self.fd, start, size, lock, partscan, read_only)
         except Exception as error:  # pylint: disable: broad-except
             self.close()
             raise error from None
diff --git a/osbuild/inputs.py b/osbuild/inputs.py
index 24c580e6..8ad682e9 100644
--- a/osbuild/inputs.py
+++ b/osbuild/inputs.py
@@ -17,17 +17,15 @@ osbuild is the path. The input options are just passed to the
 """
 
 import abc
-import contextlib
 import hashlib
 import json
 import os
-import tempfile
 from typing import Any, Dict, Optional, Tuple
 
 from osbuild import host
 from osbuild.util.types import PathLike
 
-from .objectstore import ObjectStore, StoreClient, StoreServer
+from .objectstore import StoreClient, StoreServer
 
 
 class Input:
@@ -67,7 +65,7 @@ class InputManager:
         self.root = root
         self.inputs: Dict[str, Input] = {}
 
-    def map(self, ip: Input, store: ObjectStore) -> Tuple[str, Dict]:
+    def map(self, ip: Input) -> Tuple[str, Dict]:
 
         target = os.path.join(self.root, ip.name)
         os.makedirs(target)
@@ -88,12 +86,8 @@ class InputManager:
             }
         }
 
-        with make_args_and_reply_files(store.tmp, args) as (fd_args, fd_reply):
-            fds = [fd_args, fd_reply]
-            client = self.service_manager.start(f"input/{ip.name}", ip.info.path)
-            _, _ = client.call_with_fds("map", {}, fds)
-            with os.fdopen(os.dup(fd_reply)) as f:
-                reply = json.loads(f.read())
+        client = self.service_manager.start(f"input/{ip.name}", ip.info.path)
+        reply = client.call("map", args)
 
         path = reply["path"]
 
@@ -107,15 +101,6 @@ class InputManager:
         return reply
 
 
-@contextlib.contextmanager
-def make_args_and_reply_files(tmp, args):
-    with tempfile.TemporaryFile("w+", dir=tmp, encoding="utf-8") as f_args, \
-            tempfile.TemporaryFile("w+", dir=tmp, encoding="utf-8") as f_reply:
-        json.dump(args, f_args)
-        f_args.seek(0)
-        yield f_args.fileno(), f_reply.fileno()
-
-
 class InputService(host.Service):
     """Input host service"""
 
@@ -129,21 +114,14 @@ class InputService(host.Service):
     def stop(self):
         self.unmap()
 
-    def dispatch(self, method: str, _, fds):
+    def dispatch(self, method: str, args, fds):
         if method == "map":
-            # map() sends fd[0] to read the arguments from and fd[1] to
-            # write the reply back. This avoids running into EMSGSIZE
-            with os.fdopen(fds.steal(0)) as f:
-                args = json.load(f)
             store = StoreClient(connect_to=args["api"]["store"])
             r = self.map(store,
                          args["origin"],
                          args["refs"],
                          args["target"],
                          args["options"])
-            with os.fdopen(fds.steal(1), "w") as f:
-                f.write(json.dumps(r))
-                f.seek(0)
-            return "{}", None
+            return r, None
 
         raise host.ProtocolError("Unknown method")
diff --git a/osbuild/loop.py b/osbuild/loop.py
index 2e05638e..ec6d3619 100644
--- a/osbuild/loop.py
+++ b/osbuild/loop.py
@@ -317,7 +317,7 @@ class Loop:
         # it is bound, check if it is bound by `fd`
         return loop_info.is_bound_to(file_info)
 
-    def _config_info(self, info, offset, sizelimit, autoclear, partscan):
+    def _config_info(self, info, offset, sizelimit, autoclear, partscan, read_only):
         #  pylint: disable=attribute-defined-outside-init
         if offset:
             info.lo_offset = offset
@@ -333,9 +333,14 @@ class Loop:
                 info.lo_flags |= self.LO_FLAGS_PARTSCAN
             else:
                 info.lo_flags &= ~self.LO_FLAGS_PARTSCAN
+        if read_only is not None:
+            if read_only:
+                info.lo_flags |= self.LO_FLAGS_READ_ONLY
+            else:
+                info.lo_flags &= ~self.LO_FLAGS_READ_ONLY
         return info
 
-    def set_status(self, offset=None, sizelimit=None, autoclear=None, partscan=None):
+    def set_status(self, offset=None, sizelimit=None, autoclear=None, partscan=None, read_only=None):
         """Set properties of the loopback device
 
         The loopback device must be bound, and the properties will be
@@ -371,12 +376,16 @@ class Loop:
         partscan : bool, optional
             Whether or not to enable partition scanning, or None to leave
             unchanged (default is None)
+        read_only : bool, optional
+            Whether or not to setup the loopback device as read-only (default
+            is None).
         """
 
-        info = self._config_info(self.get_status(), offset, sizelimit, autoclear, partscan)
+        info = self._config_info(self.get_status(), offset, sizelimit, autoclear, partscan, read_only)
         fcntl.ioctl(self.fd, self.LOOP_SET_STATUS64, info)
 
-    def configure(self, fd: int, offset=None, sizelimit=None, blocksize=0, autoclear=None, partscan=None):
+    def configure(self, fd: int, offset=None, sizelimit=None, blocksize=0, autoclear=None, partscan=None,
+                  read_only=None):
         """
         Configure the loopback device
         Bind and configure in a single operation a file descriptor to the
@@ -426,12 +435,15 @@ class Loop:
         partscan : bool, optional
             Whether or not to enable partition scanning, or None to leave
             unchanged (default is None)
+        read_only : bool, optional
+            Whether or not to setup the loopback device as read-only (default
+            is None).
         """
         #  pylint: disable=attribute-defined-outside-init
         config = LoopConfig()
         config.fd = fd
         config.block_size = int(blocksize)
-        config.info = self._config_info(LoopInfo(), offset, sizelimit, autoclear, partscan)
+        config.info = self._config_info(LoopInfo(), offset, sizelimit, autoclear, partscan, read_only)
         try:
             fcntl.ioctl(self.fd, self.LOOP_CONFIGURE, config)
         except OSError as e:
diff --git a/osbuild/pipeline.py b/osbuild/pipeline.py
index 437a9001..84013740 100644
--- a/osbuild/pipeline.py
+++ b/osbuild/pipeline.py
@@ -210,7 +210,7 @@ class Stage:
 
             ipmgr = InputManager(mgr, storeapi, inputs_tmpdir)
             for key, ip in self.inputs.items():
-                data = ipmgr.map(ip, store)
+                data = ipmgr.map(ip)
                 inputs[key] = data
 
             devmgr = DeviceManager(mgr, build_root.dev, tree)
diff --git a/osbuild/sources.py b/osbuild/sources.py
index b09597e9..029dc6b8 100644
--- a/osbuild/sources.py
+++ b/osbuild/sources.py
@@ -1,5 +1,4 @@
 import abc
-import contextlib
 import hashlib
 import json
 import os
@@ -29,6 +28,7 @@ class Source:
         cache = os.path.join(store.store, "sources")
 
         args = {
+            "items": self.items,
             "options": self.options,
             "cache": cache,
             "output": None,
@@ -36,20 +36,10 @@ class Source:
         }
 
         client = mgr.start(f"source/{source}", self.info.path)
-
-        with self.make_items_file(store.tmp) as fd:
-            fds = [fd]
-            reply = client.call_with_fds("download", args, fds)
+        reply = client.call("download", args)
 
         return reply
 
-    @contextlib.contextmanager
-    def make_items_file(self, tmp):
-        with tempfile.TemporaryFile("w+", dir=tmp, encoding="utf-8") as f:
-            json.dump(self.items, f)
-            f.seek(0)
-            yield f.fileno()
-
     # "name", "id", "stages", "results" is only here to make it looks like a
     # pipeline for the monitor. This should be revisited at some point
     # and maybe the monitor should get first-class support for
@@ -103,12 +93,6 @@ class SourceService(host.Service):
         """Returns True if the item to download is in cache. """
         return os.path.isfile(f"{self.cache}/{checksum}")
 
-    @staticmethod
-    def load_items(fds):
-        with os.fdopen(fds.steal(0)) as f:
-            items = json.load(f)
-        return items
-
     def setup(self, args):
         self.cache = os.path.join(args["cache"], self.content_type)
         os.makedirs(self.cache, exist_ok=True)
@@ -118,7 +102,7 @@ class SourceService(host.Service):
         if method == "download":
             self.setup(args)
             with tempfile.TemporaryDirectory(prefix=".unverified-", dir=self.cache) as self.tmpdir:
-                self.fetch_all(SourceService.load_items(fds))
+                self.fetch_all(args["items"])
                 return None, None
 
         raise host.ProtocolError("Unknown method")
diff --git a/osbuild/util/jsoncomm.py b/osbuild/util/jsoncomm.py
index 8a0cbb7c..d9d49019 100644
--- a/osbuild/util/jsoncomm.py
+++ b/osbuild/util/jsoncomm.py
@@ -5,18 +5,39 @@ serialization. It uses unix-domain-datagram-sockets and provides a simple
 unicast message transmission.
 """
 
-
 import array
 import contextlib
 import errno
 import json
 import os
 import socket
-from typing import Any, Optional
+from typing import Any, List, Optional
 
+from .linux import Libc
 from .types import PathLike
 
 
+@contextlib.contextmanager
+def memfd(name):
+    if hasattr(os, "memfd_create"):
+        fd = os.memfd_create(name)
+    else:
+        # we can remove this "if/else" once we are at python3.8+
+        # and just use "os.memfd_create()"
+        libc = Libc.default()
+        fd = libc.memfd_create(name)
+
+    try:
+        yield fd
+    finally:
+        os.close(fd)
+
+
+# this marker is used when the arguments are passed via a filedescriptor
+# because they exceed the allowed size for a network package
+ARGS_VIA_FD_MARKER = b"<args-via-fd>"
+
+
 class FdSet:
     """File-Descriptor Set
 
@@ -92,6 +113,19 @@ class FdSet:
         return v
 
 
+def wmem_max() -> int:
+    """ Return the kernels maximum send socket buffer size in bytes
+
+    When /proc is not mounted return a conservative estimate (64kb).
+    """
+    try:
+        with open("/proc/sys/net/core/wmem_max", encoding="utf8") as wmem_file:
+            return int(wmem_file.read().strip())
+    except FileNotFoundError:
+        # conservative estimate for systems that have no /proc mounted
+        return 64_000
+
+
 class Socket(contextlib.AbstractContextManager):
     """Communication Socket
 
@@ -353,7 +387,17 @@ class Socket(contextlib.AbstractContextManager):
             if level == socket.SOL_SOCKET and ty == socket.SCM_RIGHTS:
                 assert len(data) % fds.itemsize == 0
                 fds.frombytes(data)
-        fdset = FdSet(rawfds=fds)
+        # Next we need to check if the serialzed data comes via an FD
+        # or via the message. FDs are used if the data size is big to
+        # avoid running into errno.EMSGSIZE
+        if msg[0] == ARGS_VIA_FD_MARKER:
+            fd_payload = fds[0]
+            fdset = FdSet(rawfds=fds[1:])
+            with os.fdopen(fd_payload) as f:
+                serialized = f.read()
+        else:
+            fdset = FdSet(rawfds=fds)
+            serialized = msg[0]
 
         # Check the returned message flags. If the message was truncated, we
         # have to discard it. This shouldn't happen, but there is no harm in
@@ -364,13 +408,38 @@ class Socket(contextlib.AbstractContextManager):
             raise BufferError
 
         try:
-            payload = json.loads(msg[0])
+            payload = json.loads(serialized)
         except json.JSONDecodeError as e:
             raise BufferError from e
 
         return (payload, fdset, msg[3])
 
-    def send(self, payload: object, *, fds: Optional[list] = None):
+    def _send_via_fd(self, serialized: bytes, fds: List[int]):
+        assert self._socket is not None
+        with memfd("jsoncomm/payload") as fd_payload:
+            os.write(fd_payload, serialized)
+            os.lseek(fd_payload, 0, 0)
+            cmsg = []
+            cmsg.append((socket.SOL_SOCKET, socket.SCM_RIGHTS, array.array("i", [fd_payload] + fds)))
+            n = self._socket.sendmsg([ARGS_VIA_FD_MARKER], cmsg, 0)
+        assert n == len(ARGS_VIA_FD_MARKER)
+
+    def _send_via_sendmsg(self, serialized: bytes, fds: List[int]):
+        assert self._socket is not None
+        cmsg = []
+        if fds:
+            cmsg.append((socket.SOL_SOCKET, socket.SCM_RIGHTS, array.array("i", fds)))
+        try:
+            self._socket.setsockopt(socket.SOL_SOCKET, socket.SO_SNDBUF, len(serialized))
+            n = self._socket.sendmsg([serialized], cmsg, 0)
+        except OSError as exc:
+            if exc.errno == errno.EMSGSIZE:
+                raise BufferError(
+                    f"jsoncomm message size {len(serialized)} is too big") from exc
+            raise exc
+        assert n == len(serialized)
+
+    def send(self, payload: object, *, fds: Optional[list] = None) -> None:
         """Send Message
 
         Send a new message via this socket. This operation is synchronous. The
@@ -399,19 +468,14 @@ class Socket(contextlib.AbstractContextManager):
         if not self._socket:
             raise RuntimeError("Tried to send without socket.")
 
-        serialized = json.dumps(payload).encode()
-        cmsg = []
-        if fds:
-            cmsg.append((socket.SOL_SOCKET, socket.SCM_RIGHTS, array.array("i", fds)))
+        if not fds:
+            fds = []
 
-        try:
-            n = self._socket.sendmsg([serialized], cmsg, 0)
-        except OSError as exc:
-            if exc.errno == errno.EMSGSIZE:
-                raise BufferError(
-                    f"jsoncomm message size {len(serialized)} is too big") from exc
-            raise exc
-        assert n == len(serialized)
+        serialized = json.dumps(payload).encode()
+        if len(serialized) > wmem_max():
+            self._send_via_fd(serialized, fds)
+        else:
+            self._send_via_sendmsg(serialized, fds)
 
     def send_and_recv(self, payload: object, *, fds: Optional[list] = None):
         """Send a message and wait for a reply
diff --git a/osbuild/util/linux.py b/osbuild/util/linux.py
index d09af8fc..a26d62bb 100644
--- a/osbuild/util/linux.py
+++ b/osbuild/util/linux.py
@@ -490,6 +490,29 @@ class Libc:
         setattr(proto, "errcheck", self._errcheck_errno)
         setattr(proto, "__name__", "futimens")
         self.futimens = proto
+        # prototype: _memfd_create() (takes a byte type name)
+        # (can be removed once we move to python3.8)
+        proto = ctypes.CFUNCTYPE(
+            ctypes.c_int,  # restype (return type)
+            ctypes.c_char_p,
+            ctypes.c_uint,
+            use_errno=True,
+        )(
+            ("memfd_create", self._lib),
+            (
+                (1, "name"),
+                (1, "flags", 0),
+            ),
+        )
+        setattr(proto, "errcheck", self._errcheck_errno)
+        setattr(proto, "__name__", "memfd_create")
+        self._memfd_create = proto
+
+    # (can be removed once we move to python3.8)
+    def memfd_create(self, name: str, flags: int = 0) -> int:
+        """ create an anonymous file """
+        char_p_name = name.encode()
+        return self._memfd_create(char_p_name, flags)
 
     @staticmethod
     def make() -> "Libc":


Test patch:
diff --git a/test/mod/test_inputs.py b/test/mod/test_inputs.py
index 915d1313..fc2dc291 100644
--- a/test/mod/test_inputs.py
+++ b/test/mod/test_inputs.py
@@ -1,9 +1,7 @@
-import json
 import os
-from unittest.mock import patch
+from unittest.mock import call, patch
 
 from osbuild import inputs
-from osbuild.util.jsoncomm import FdSet
 
 
 class FakeInputService(inputs.InputService):
@@ -11,7 +9,7 @@ class FakeInputService(inputs.InputService):
         # do not call "super().__init__()" here to make it testable
         self.map_calls = []
 
-    def map(self, _store, origin, refs, target, options):
+    def map(self, store, origin, refs, target, options):
         self.map_calls.append([origin, refs, target, options])
         return "complex", 2, "reply"
 
@@ -20,8 +18,6 @@ def test_inputs_dispatches_map(tmp_path):
     store_api_path = tmp_path / "api-store"
     store_api_path.write_text("")
 
-    args_path = tmp_path / "args"
-    reply_path = tmp_path / "reply"
     args = {
         "api": {
             "store": os.fspath(store_api_path),
@@ -31,17 +27,14 @@ def test_inputs_dispatches_map(tmp_path):
         "target": "some-target",
         "options": "some-options",
     }
-    args_path.write_text(json.dumps(args))
-    reply_path.write_text("")
-
-    with args_path.open() as f_args, reply_path.open("w") as f_reply:
-        fd_args, fd_reply = os.dup(f_args.fileno()), os.dup(f_reply.fileno())
-        fds = FdSet.from_list([fd_args, fd_reply])
-        fake_service = FakeInputService(args="some")
-        with patch.object(inputs, "StoreClient"):
-            r = fake_service.dispatch("map", None, fds)
-            assert r == ('{}', None)
-        assert fake_service.map_calls == [
-            ["some-origin", "some-refs", "some-target", "some-options"],
-        ]
-    assert reply_path.read_text() == '["complex", 2, "reply"]'
+
+    fake_service = FakeInputService(args="some")
+    with patch.object(inputs, "StoreClient") as mocked_store_client_klass:
+        r = fake_service.dispatch("map", args, None)
+    assert mocked_store_client_klass.call_args_list == [
+        call(connect_to=os.fspath(store_api_path)),
+    ]
+    assert fake_service.map_calls == [
+        ["some-origin", "some-refs", "some-target", "some-options"],
+    ]
+    assert r == (("complex", 2, "reply"), None)
diff --git a/test/mod/test_util_jsoncomm.py b/test/mod/test_util_jsoncomm.py
index 93d62a13..53c406d3 100644
--- a/test/mod/test_util_jsoncomm.py
+++ b/test/mod/test_util_jsoncomm.py
@@ -2,13 +2,17 @@
 # Tests for the 'osbuild.util.jsoncomm' module.
 #
 
+# pylint: disable=protected-access
+
 import asyncio
 import errno
+import json
 import os
 import pathlib
 import tempfile
 import unittest
 from concurrent import futures
+from unittest.mock import patch
 
 import pytest
 
@@ -220,11 +224,39 @@ class TestUtilJsonComm(unittest.TestCase):
         pong, _, _ = a.recv()
         self.assertEqual(ping, pong)
 
-    def test_send_and_recv_tons_of_data_still_errors(self):
+    def test_sendmsg_errors_with_size_on_EMSGSIZE(self):
         a, _ = jsoncomm.Socket.new_pair()
 
-        ping = {"data": "1" * 1_000_000}
+        serialized = json.dumps({"data": "1" * 1_000_000}).encode()
         with pytest.raises(BufferError) as exc:
-            a.send(ping)
+            a._send_via_sendmsg(serialized, [])
         assert str(exc.value) == "jsoncomm message size 1000012 is too big"
         assert exc.value.__cause__.errno == errno.EMSGSIZE
+
+    def test_send_and_recv_tons_of_data_is_fine(self):
+        a, b = jsoncomm.Socket.new_pair()
+
+        ping = {"data": "tons" * 1_000_000}
+        a.send(ping)
+        pong, _, _ = b.send_and_recv(ping)
+        self.assertEqual(ping, pong)
+        pong, _, _ = a.recv()
+        self.assertEqual(ping, pong)
+
+    def test_send_small_data_via_sendmsg(self):
+        a, _ = jsoncomm.Socket.new_pair()
+        with patch.object(a, "_send_via_fd") as mock_send_via_fd, \
+                patch.object(a, "_send_via_sendmsg") as mock_send_via_sendmsg:
+            ping = {"data": "little"}
+            a.send(ping)
+        assert mock_send_via_fd.call_count == 0
+        assert mock_send_via_sendmsg.call_count == 1
+
+    def test_send_huge_data_via_fd(self):
+        a, _ = jsoncomm.Socket.new_pair()
+        with patch.object(a, "_send_via_fd") as mock_send_via_fd, \
+                patch.object(a, "_send_via_sendmsg") as mock_send_via_sendmsg:
+            ping = {"data": "tons" * 1_000_000}
+            a.send(ping)
+        assert mock_send_via_fd.call_count == 1
+        assert mock_send_via_sendmsg.call_count == 0
diff --git a/test/mod/test_util_linux.py b/test/mod/test_util_linux.py
index 518a4bfa..b77bc8dc 100644
--- a/test/mod/test_util_linux.py
+++ b/test/mod/test_util_linux.py
@@ -187,6 +187,7 @@ def test_libc():
     assert libc0.RENAME_NOREPLACE
     assert libc0.RENAME_WHITEOUT
     assert libc0.renameat2
+    assert libc0.memfd_create
 
 
 def test_libc_renameat2_errcheck():
@@ -226,6 +227,23 @@ def test_libc_renameat2_exchange(tmpdir):
         assert f.read() == "foo"
 
 
+def test_libc_memfd_create_errcheck():
+    libc = linux.Libc.default()
+    with pytest.raises(OSError) as exc:
+        libc.memfd_create("foo", -1)
+    assert "Invalid argument" in str(exc.value)
+
+
+def test_libc_memfd_create():
+    libc = linux.Libc.default()
+    fd = libc.memfd_create("foo", 0)
+    os.write(fd, b"file content")
+    fd2 = os.dup(fd)
+    os.close(fd)
+    os.lseek(fd2, 0, 0)
+    assert os.read(fd2, 4096) == b"file content"
+
+
 def test_proc_boot_id():
     #
     # Test the `proc_boot_id()` function which reads the current boot-id