fix: validate BORG_PACK_MAX_SIZE and BORG_PACK_MAX_COUNT

docs/internals/packs.rst

@@ -188,7 +188,9 @@ Writing packs
 (``PackWriter``). When the buffered blobs reach the pack limit, the buffer is
 stored as one pack. By default the limit is a size of 50 MB (``DEFAULT_PACK_MAX_SIZE``).
 ``BORG_PACK_MAX_SIZE`` sets the size limit and ``BORG_PACK_MAX_COUNT`` a blob count
-limit; with only ``BORG_PACK_MAX_COUNT`` set, packs are bound by count only, see
+limit. ``BORG_PACK_MAX_SIZE`` must be below ``MAX_PACK_SIZE_LIMIT``, which is
+2126512128 bytes (2 GiB - 20 MiB); with only ``BORG_PACK_MAX_COUNT`` set, packs are
+still capped at ``MAX_PACK_SIZE_LIMIT - 1`` (2126512127) bytes, see
 :ref:`env_vars`. The blob that reaches the limit is part of that pack, so a pack can
 be larger than the size limit by less than one blob.
 

src/borg/archiver/help_cmd.py

@@ -745,14 +745,18 @@ class HelpMixIn:
                 When set to a numeric value, limit the pack cache to that many bytes.
                 Only has an effect if BORG_STORE_CACHE is set.
             BORG_PACK_MAX_SIZE
-                When set to a numeric value, cap packs (the repository objects that batch up many
+                When set to a positive integer, cap packs (the repository objects that batch up many
                 chunks, see the internals documentation about pack files) at that many bytes
                 instead of the default of 50000000.
+                The value must be below 2126512128 (2 GiB - 20 MiB), which keeps packs
+                clear of OS bugs with files of 2 GiB or more. A non-integer, a non-positive
+                value, or a value that reaches that limit is rejected.
                 Smaller packs mean more (but smaller) repository objects and more
                 fine-grained uploads; bigger packs mean fewer objects and fewer stores.
             BORG_PACK_MAX_COUNT
-                When set to a numeric value, cap packs at that many objects per pack.
-                If BORG_PACK_MAX_SIZE is not also set, packs are then bound by count only.
+                When set to a positive integer, cap packs at that many objects per pack.
+                A non-integer or non-positive value is rejected.
+                If BORG_PACK_MAX_SIZE is not set, packs still stay below 2 GiB.
             BORG_PACK_ASYNC
                 When set to ``no``, disable the background thread that stores a finished pack
                 while the next one is being assembled, and store packs synchronously instead.

src/borg/constants.py

@@ -87,6 +87,11 @@
 # MAX_OBJECT_SIZE = MAX_DATA_SIZE + len(PUT header)
 MAX_OBJECT_SIZE = MAX_DATA_SIZE + 41  # see assertion at end of repository module
 
+# BORG_PACK_MAX_SIZE must stay below this to keep packs below 2 GiB and avoid OS bugs.
+# PackWriter.add includes the object that crosses the cap, so the
+# largest accepted size is one less than this limit.
+MAX_PACK_SIZE_LIMIT = 2**31 - MAX_OBJECT_SIZE
+
 # Clock skew is the difference between the clocks of the machines writing to a repository (seconds).
 # A check result timestamp up to this far in the future still counts as recent; further ahead than
 # this, the pack is re-verified.

src/borg/repository.py

@@ -984,6 +984,41 @@ def __init__(
         # True if packs are cached locally (BORG_STORE_CACHE): store.load() of a pack may return the cached copy.
         self.uses_pack_store_cache = cache_url is not None
 
+        # pack-sizing overrides: BORG_PACK_MAX_COUNT sets the max object count per pack,
+        # BORG_PACK_MAX_SIZE the max pack size in bytes. Default: size-bound only.
+        # They are validated here, so a bad value fails before the store is created, opened or locked.
+        # An empty value counts as unset, like BORG_PACK_CACHE_SIZE. A non-integer or non-positive
+        # value is rejected. The size must also stay below
+        # MAX_PACK_SIZE_LIMIT: add() keeps the object that crosses the cap, so this keeps packs
+        # below 2 GiB, clear of OS bugs with files of 2 GiB or more. Count-only mode still passes
+        # that ceiling (minus one) as max_size. pack_max_size remembers the configured size, or the
+        # default when the user did not set one, so the safety ceiling does not become the compact target.
+        max_count_env = os.environ.get("BORG_PACK_MAX_COUNT") or None
+        max_size_env = os.environ.get("BORG_PACK_MAX_SIZE") or None
+        if max_count_env is None:
+            max_count = None
+        else:
+            try:
+                max_count = int(max_count_env)
+            except ValueError:
+                raise Error(f"BORG_PACK_MAX_COUNT must be an integer, but is: {max_count_env!r}") from None
+            if max_count <= 0:
+                raise Error(f"BORG_PACK_MAX_COUNT must be positive, but is: {max_count}")
+        self._pack_max_count = max_count
+        if max_size_env is None:
+            self._pack_max_size = (MAX_PACK_SIZE_LIMIT - 1) if max_count is not None else DEFAULT_PACK_MAX_SIZE
+            self._configured_pack_max_size = DEFAULT_PACK_MAX_SIZE
+        else:
+            try:
+                max_size = int(max_size_env)
+            except ValueError:
+                raise Error(f"BORG_PACK_MAX_SIZE must be an integer, but is: {max_size_env!r}") from None
+            if max_size <= 0:
+                raise Error(f"BORG_PACK_MAX_SIZE must be positive, but is: {max_size}")
+            if max_size >= MAX_PACK_SIZE_LIMIT:
+                raise Error(f"BORG_PACK_MAX_SIZE must be below {MAX_PACK_SIZE_LIMIT}, but is: {max_size}")
+            self._pack_max_size = self._configured_pack_max_size = max_size
+
         propagate_rsh()  # borgstore shall use the same remote shell command as borg
 
         try:
@@ -1324,26 +1359,21 @@ def open(self, *, exclusive, lock_wait=None, lock=True):
             else:
                 self.acquire_lock()
         self._chunks = None
-        # pack-sizing overrides: BORG_PACK_MAX_COUNT sets the max object count per pack,
-        # BORG_PACK_MAX_SIZE the max pack size in bytes. Default: size-bound only.
-        max_count_env = os.environ.get("BORG_PACK_MAX_COUNT")
-        max_size_env = os.environ.get("BORG_PACK_MAX_SIZE")
-        max_count = int(max_count_env) if max_count_env is not None else None
-        if max_size_env is not None:
-            max_size = int(max_size_env)
-        else:
-            max_size = None if max_count is not None else DEFAULT_PACK_MAX_SIZE
         # BORG_PACK_ASYNC=no disables the background store-thread (debugging aid, see PackWriter).
         async_store = os.environ.get("BORG_PACK_ASYNC", "yes") != "no"
         self._pack_writer = PackWriter(
-            self.store, repository=self, max_count=max_count, max_size=max_size, async_store=async_store
+            self.store,
+            repository=self,
+            max_count=self._pack_max_count,
+            max_size=self._pack_max_size,
+            async_store=async_store,
         )
         self.opened = True
 
     @property
     def pack_max_size(self):
-        """The configured byte cap for a pack (BORG_PACK_MAX_SIZE, or the default if count-bound)."""
-        return self._pack_writer.max_size or DEFAULT_PACK_MAX_SIZE
+        """The configured byte cap for a pack (BORG_PACK_MAX_SIZE, or the default if it was not set)."""
+        return self._configured_pack_max_size
 
     @property
     def chunks(self):

src/borg/testsuite/repository_test.py

@@ -16,7 +16,7 @@
 from .. import repository as repository_module
 from ..cache import chunkindex_is_invalid, delete_chunkindex_from_repo, write_chunkindex_invalid
 from ..compress import CNONE
-from ..constants import MAX_CLOCK_SKEW, ROBJ_FILE_STREAM
+from ..constants import DEFAULT_PACK_MAX_SIZE, MAX_CLOCK_SKEW, MAX_PACK_SIZE_LIMIT, ROBJ_FILE_STREAM
 from ..crypto.key import AESOCBKey, AuthenticatedKey, Blake3AuthenticatedKey, CHPOKey
 from ..helpers import Error, IntegrityError, Location, bin_to_hex, hex_to_bin
 from ..hashindex import ChunkIndex, ChunkIndexEntry
@@ -1454,6 +1454,89 @@ def __getattr__(self, name):
         return getattr(self._inner, name)
 
 
+@pytest.mark.parametrize("value", ["0", "-1", "abc", str(MAX_PACK_SIZE_LIMIT)])
+def test_borg_pack_max_size_rejected(tmp_path, monkeypatch, value):
+    with Repository(os.fspath(tmp_path / "repo"), exclusive=True, create=True) as repository:
+        pass
+    monkeypatch.setenv("BORG_PACK_MAX_SIZE", value)
+    with pytest.raises(Error, match="BORG_PACK_MAX_SIZE"):
+        with reopen(repository):
+            pass
+
+
+@pytest.mark.parametrize("name", ["BORG_PACK_MAX_SIZE", "BORG_PACK_MAX_COUNT"])
+def test_borg_pack_limits_rejected_before_create(tmp_path, monkeypatch, name):
+    monkeypatch.setenv(name, "0")
+    with pytest.raises(Error, match=name):
+        with Repository(os.fspath(tmp_path / "repo"), exclusive=True, create=True):
+            pass
+    assert not (tmp_path / "repo").exists()
+
+
+def test_borg_pack_max_size_just_below_limit(tmp_path, monkeypatch):
+    with Repository(os.fspath(tmp_path / "repo"), exclusive=True, create=True) as repository:
+        pass
+    accepted = MAX_PACK_SIZE_LIMIT - 1
+    monkeypatch.setenv("BORG_PACK_MAX_SIZE", str(accepted))
+    with reopen(repository) as repository:
+        assert repository._pack_writer.max_size == accepted
+        assert repository.pack_max_size == accepted
+
+
+@pytest.mark.parametrize("value", ["0", "-1", "nope"])
+def test_borg_pack_max_count_rejected(tmp_path, monkeypatch, value):
+    with Repository(os.fspath(tmp_path / "repo"), exclusive=True, create=True) as repository:
+        pass
+    monkeypatch.setenv("BORG_PACK_MAX_COUNT", value)
+    with pytest.raises(Error, match="BORG_PACK_MAX_COUNT"):
+        with reopen(repository):
+            pass
+
+
+def test_borg_pack_max_count_only_keeps_size_ceiling(tmp_path, monkeypatch):
+    with Repository(os.fspath(tmp_path / "repo"), exclusive=True, create=True) as repository:
+        pass
+    monkeypatch.delenv("BORG_PACK_MAX_SIZE", raising=False)
+    monkeypatch.setenv("BORG_PACK_MAX_COUNT", "4")
+    with reopen(repository) as repository:
+        assert repository._pack_writer.max_count == 4
+        assert repository._pack_writer.max_size == MAX_PACK_SIZE_LIMIT - 1
+        assert repository.pack_max_size == DEFAULT_PACK_MAX_SIZE
+
+
+def test_borg_pack_limits_default_when_unset(tmp_path, monkeypatch):
+    with Repository(os.fspath(tmp_path / "repo"), exclusive=True, create=True) as repository:
+        pass
+    monkeypatch.delenv("BORG_PACK_MAX_COUNT", raising=False)
+    monkeypatch.delenv("BORG_PACK_MAX_SIZE", raising=False)
+    with reopen(repository) as repository:
+        assert repository._pack_writer.max_count is None
+        assert repository._pack_writer.max_size == DEFAULT_PACK_MAX_SIZE
+        assert repository.pack_max_size == DEFAULT_PACK_MAX_SIZE
+
+
+def test_borg_pack_limits_empty_means_unset(tmp_path, monkeypatch):
+    with Repository(os.fspath(tmp_path / "repo"), exclusive=True, create=True) as repository:
+        pass
+    monkeypatch.setenv("BORG_PACK_MAX_COUNT", "")
+    monkeypatch.setenv("BORG_PACK_MAX_SIZE", "")
+    with reopen(repository) as repository:
+        assert repository._pack_writer.max_count is None
+        assert repository._pack_writer.max_size == DEFAULT_PACK_MAX_SIZE
+        assert repository.pack_max_size == DEFAULT_PACK_MAX_SIZE
+
+
+def test_borg_pack_count_and_size_both_passed_through(tmp_path, monkeypatch):
+    with Repository(os.fspath(tmp_path / "repo"), exclusive=True, create=True) as repository:
+        pass
+    monkeypatch.setenv("BORG_PACK_MAX_COUNT", "5")
+    monkeypatch.setenv("BORG_PACK_MAX_SIZE", "100000")
+    with reopen(repository) as repository:
+        assert repository._pack_writer.max_count == 5
+        assert repository._pack_writer.max_size == 100000
+        assert repository.pack_max_size == 100000
+
+
 def test_pack_writer_returns_none_when_not_full():
     pw = PackWriter(MockStore(), max_count=2, chunks=ChunkIndex())
     assert pw.add(b"a" * 32, b"data") is None