Repository: osbuild/osbuild
Pull request #1888 (2024-09-17 16:19:55)

stages/coreos.platform: use shutil.copy
Switch from shutil.copy2 so that we don't copy over the SELinux labels from the source file.


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":
diff --git a/stages/org.osbuild.coreos.platform b/stages/org.osbuild.coreos.platform
index a88951cc..7e66c26c 100755
--- a/stages/org.osbuild.coreos.platform
+++ b/stages/org.osbuild.coreos.platform
@@ -52,8 +52,10 @@ def main(paths, options):
     json_grub_args, json_kargs = None, None
     if os.path.exists(platforms_source_path):
         os.makedirs(os.path.dirname(platforms_dest_path), mode=0o755, exist_ok=True)
-        # Copy platforms.json to the boot partition
-        shutil.copy2(platforms_source_path, platforms_dest_path)
+        # Copy platforms.json to the boot partition. Use shutil.copy here and not
+        # shutil.copy2 because we don't want the selinux labels from the source
+        # to be copied over, but rather the defaults for the destination.
+        shutil.copy(platforms_source_path, platforms_dest_path)
         json_grub_args, json_kargs = process_platforms_json(platforms_dest_path, platform)
     if json_kargs:
         kernel_arguments.extend(json_kargs)


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