check --repair: resync past corrupt object headers when rebuilding the chunks index

docs/internals/packs.rst

@@ -91,10 +91,34 @@ A reader locates the next blob by advancing::
 
     next_blob_offset = current_blob_offset + REPOOBJ_HEADER_SIZE + meta_size + data_size
 
-The per-blob magic limits the blast radius of corrupted length fields: if
-``meta_size`` or ``data_size`` is damaged, the scanner loses at most one blob.
-Once it finds the next ``OBJ_MAGIC`` sequence it resumes. Other corruption
-(payload bit flips) is caught by AEAD on that blob without losing position.
+``iter_headers()`` checks every header it walks: it must have ``OBJ_MAGIC``, a
+supported version, and sizes that keep the blob inside the pack. A header that
+fails these checks means a corrupt pack, and ``IntegrityError`` is raised.
+
+The per-blob magic limits the blast radius of corrupted length fields. The
+repair walk (``iter_headers(validate=...)``, used when ``borg check --repair``
+rebuilds the chunks index from the packs) scans for the next blob and resumes
+there, so the blobs after the damaged part of the pack are still found. The scan
+starts just past the last blob the walk accepted: a corrupted length field that
+keeps the blob inside the pack leaves the header valid, and is noticed only at
+the misaligned offset it points to. The blob carrying it is dropped when a
+recovered blob starts inside the extent it claims - its length field is wrong,
+so it can not be read back.
+
+``OBJ_MAGIC`` occurs inside the payloads as well, and in the ``none-*`` and
+``authenticated-*`` modes the payloads are user content stored as it is, so a
+backed up file can contain something shaped like a blob. A candidate is
+therefore accepted only when its metadata slot verifies against the header AAD
+described above; the header and that slot, a few hundred bytes, are what the
+scan reads. Verifying needs the key, so a repair that cannot read the manifest
+walks without scanning.
+
+In the ``none-*`` modes the tag is an unkeyed checksum, so the scan accepts any
+well-formed blob, including one a backed up file contains.
+
+``data_size`` is not part of the AAD, so accepting a candidate authenticates
+its chunk id, and its size only as far as the blob fits into the pack. Bit flips
+in the data are caught when the blob is read, on that blob alone.
 
 Blobs follow one another contiguously with no padding::
 

src/borg/archive.py

@@ -2081,6 +2081,28 @@ def __next__(self):
             return next(self._unpacker)
 
 
+def resync_validator(repo_objs):
+    """Return validate(chunk_id, obj): True if obj is the repo object with id chunk_id.
+
+    obj is an object's header plus its metadata slot. Parsing that slot verifies its tag, which is
+    computed over the header's magic, version and chunk id as well (AAD, additional authenticated
+    data: bytes the tag covers without being part of the ciphertext).
+
+    In the "none-*" modes the tag is an unkeyed checksum, so validate accepts any well-formed
+    object, including one that a backed up file contains.
+    """
+
+    def validate(chunk_id, obj):
+        try:
+            repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE)
+        except Exception:
+            # arbitrary bytes fail the tag, the msgpack unpacking or the length checks.
+            return False
+        return True
+
+    return validate
+
+
 class ArchiveChecker:
     # Bound how many missing file chunks rebuild_archives buffers for its end-of-run report,
     # so checking a badly damaged repo with very many missing chunks can not exhaust memory.
@@ -2135,7 +2157,18 @@ def check(
         # so we do not rebuild it from the packs (reading every pack is far too slow for a routine check).
         # --repair does rebuild from the packs (slow_rebuild=repair), working from the real packs so it
         # can detect and fix archives that reference chunks whose pack has gone missing.
-        self.chunks = build_chunkindex_from_repo(self.repository, slow_rebuild=repair, write_immediately=False)
+        # --repair also passes validate, which makes the rebuild resync past a corrupt object header.
+        # Validating needs the key, so read it here. manifest_only=True, because the other source
+        # make_key reads keys from is self.chunks, which is only built below.
+        if repair and self.key is None:
+            try:
+                self.key = self.make_key(repository, manifest_only=True)
+            except IntegrityError as err:
+                logger.warning(f"{err}. Packs with a corrupt object header can not be repaired.")
+        validate = resync_validator(RepoObj(self.key)) if repair and self.key is not None else None
+        self.chunks = build_chunkindex_from_repo(
+            self.repository, slow_rebuild=repair, validate=validate, write_immediately=False
+        )
         if self.key is None:
             self.key = self.make_key(repository)
         self.repo_objs = RepoObj(self.key)
@@ -2668,7 +2701,12 @@ def finish(self):
                 # the packs changed, so the index no longer matches them: rebuild it from the packs
                 # and persist it.
                 logger.info("Rebuilding and writing the repository chunks index.")
-                build_chunkindex_from_repo(self.repository, slow_rebuild=True, write_immediately=True)
+                build_chunkindex_from_repo(
+                    self.repository,
+                    slow_rebuild=True,
+                    validate=resync_validator(self.repo_objs),
+                    write_immediately=True,
+                )
             else:
                 # the packs are unchanged, so the index still matches them: persist it as is.
                 logger.info("Writing the rebuilt repository chunks index.")

src/borg/cache.py

@@ -857,10 +857,18 @@ def repack_chunkindex(repository):
 
 
 def build_chunkindex_from_repo(
-    repository, *, slow_rebuild=False, fragments_only=False, write_immediately=False, init_flags=ChunkIndex.F_USED
+    repository,
+    *,
+    slow_rebuild=False,
+    fragments_only=False,
+    validate=None,
+    write_immediately=False,
+    init_flags=ChunkIndex.F_USED,
 ):
     # fragments_only: build the index from the index/ fragments only, returning None if they cannot be
     # read completely, and never write to the repo.
+    # validate: handed to PackReader.iter_headers when rebuilding from the packs, making it resync
+    # past a corrupt object header rather than raise IntegrityError.
     assert not (slow_rebuild and fragments_only)
     assert not (fragments_only and write_immediately)  # fragments_only never writes to the repo
     # first, try to build a fresh, mostly complete chunk index from centrally stored index fragments:
@@ -952,7 +960,7 @@ def build_chunkindex_from_repo(
         repository._lock_refresh()
         pi.show(increase=1)
         pack_id = hex_to_bin(info.name)
-        for chunk_id, obj_offset, obj_size in PackReader(repository.store, pack_id).iter_headers():
+        for chunk_id, obj_offset, obj_size in PackReader(repository.store, pack_id).iter_headers(validate=validate):
             num_chunks += 1
             chunks[chunk_id] = ChunkIndexEntry(
                 flags=init_flags, size=0, pack_id=pack_id, obj_offset=obj_offset, obj_size=obj_size

src/borg/repository.py

@@ -30,14 +30,17 @@
 from .storelocking import Lock
 from .logger import create_logger
 from .manifest import NoManifestError
-from .repoobj import RepoObj, OBJ_MAGIC
+from .repoobj import RepoObj, OBJ_MAGIC, SUPPORTED_OBJ_VERSIONS
 from .crypto.key import is_keyfile
 
 logger = create_logger(__name__)
 
 # an object name is its sha256 as 64 lowercase hex digits.
 _valid_object_name = re.compile(r"[0-9a-f]{64}").fullmatch
 
+# how much of a pack PackReader reads at once when searching for the next object header.
+RESYNC_WINDOW_SIZE = 1024 * 1024
+
 
 def repo_lister(repository, *, limit=None):
     marker = None
@@ -362,46 +365,124 @@ def read(self, offset, size):
         return self.store.load(self.key, offset=offset, size=size)
 
     def size(self):
-        """Return the pack size in bytes; for a store-backed pack this is one metadata lookup."""
+        """Return the pack size in bytes (a store metadata lookup, unless the pack is in memory)."""
         if self.pack_contents is not None:
             return len(self.pack_contents)
         return self.store.info(self.key).size
 
-    def iter_headers(self):
+    @staticmethod
+    def _parse_header(hdr_data, offset, pack_size):
+        """Return the ObjHeader in hdr_data if it is a valid header at offset, None otherwise.
+
+        Valid means: OBJ_MAGIC, a supported version, and an object that fits into the pack.
+        """
+        hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(hdr_data))
+        if hdr.magic != OBJ_MAGIC or hdr.version not in SUPPORTED_OBJ_VERSIONS:
+            return None
+        if offset + RepoObj.obj_header.size + hdr.meta_size + hdr.data_size > pack_size:
+            return None
+        return hdr
+
+    def _find_header(self, offset, pack_size, validate):
+        """Scan forward from offset for the next object validate accepts, return its offset or None.
+
+        A pack has no framing besides the object headers, so this searches for OBJ_MAGIC. That byte
+        sequence also occurs inside payloads, so a candidate is accepted only when its header parses
+        and validate(chunk_id, obj) confirms the header and metadata slot at that position.
+        """
+        hdr_size = RepoObj.obj_header.size
+        while offset + hdr_size <= pack_size:
+            # a window at a time, so the scan costs one store request per RESYNC_WINDOW_SIZE bytes.
+            buf = bytes(self.read(offset, min(RESYNC_WINDOW_SIZE, pack_size - offset)))
+            if len(buf) < hdr_size:
+                break
+            pos = 0
+            while True:
+                pos = buf.find(OBJ_MAGIC, pos)
+                if pos < 0 or pos + hdr_size > len(buf):
+                    break  # not in this window, or a header overlapping its end: the next window has it
+                hdr = self._parse_header(buf[pos : pos + hdr_size], offset + pos, pack_size)
+                if hdr is not None:
+                    obj_size = hdr_size + hdr.meta_size + hdr.data_size
+                    # an object is at most MAX_DATA_SIZE bytes, so a bigger one is a false match on
+                    # OBJ_MAGIC inside a payload.
+                    if obj_size <= MAX_DATA_SIZE:
+                        size = hdr_size + hdr.meta_size  # the bytes validate looks at
+                        end = pos + size
+                        # the window holds these bytes, unless the candidate crosses its end.
+                        obj = buf[pos:end] if end <= len(buf) else self.read(offset + pos, size)
+                        if validate(hdr.chunk_id, obj):
+                            return offset + pos
+                pos += 1
+            # step by the window less one header, so a magic straddling the boundary is still found.
+            offset += max(len(buf) - (hdr_size - 1), 1)
+        return None
+
+    def iter_headers(self, validate=None):
         """Yield (chunk_id, offset, size) for each object by walking the fixed object headers.
 
-        Only the headers are read, not the payloads, so locating every object costs one short
-        range read per object (or just a slice, when the pack is already in memory), plus one
-        store metadata lookup for the pack size.
+        The walk reads a header per object: one short range read each (or a slice, for a pack in
+        memory), plus one store metadata lookup for the pack size.
+
+        A header must have OBJ_MAGIC, a supported version and describe an object that fits into
+        the pack, otherwise the pack is corrupt and IntegrityError is raised. A read shorter than
+        a header ends the walk: that is the end of the pack.
 
-        Each full header must have OBJ_MAGIC and describe an object that fits into the pack,
-        otherwise the pack is corrupt and IntegrityError is raised. Ending the walk instead
-        would be worse than raising: the chunks index rebuilt from these headers would just be
-        missing the rest of the pack, and borg check --repair would then "fix" the archives by
-        dropping chunks that are there.
-        A trailing partial header is the clean end of the pack, not corruption.
+        validate(chunk_id, obj) tells whether obj - an object's header and metadata slot - is the
+        repo object with id chunk_id. Given one, a corrupt header makes the walk resync instead:
+        it scans from just past the last object it accepted for the next object validate accepts
+        and continues there. It scans from there because a corrupt meta_size or data_size leaves
+        the header valid and moves the walk into a later object. An object is dropped when a
+        recovered object starts inside it: its size field is wrong.
         """
         pack_hex = bin_to_hex(self.pack_id) if self.pack_id is not None else "<no id>"
         pack_size = self.size()
         hdr_size = RepoObj.obj_header.size
         offset = 0
+        scan_from = 1  # just past the last accepted object's header
+        pending = None  # the last object seen, yielded once the walk gets past its end
         while True:
             hdr_data = self.read(offset, hdr_size)
             if len(hdr_data) < hdr_size:
                 break  # clean EOF, or trailing partial bytes
-            hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(hdr_data))
-            if hdr.magic != OBJ_MAGIC:
-                raise IntegrityError(
-                    f'pack {pack_hex}: no object header at offset {offset} (pack corruption), run "borg check"'
+            hdr = self._parse_header(hdr_data, offset, pack_size)
+            if hdr is None:
+                if validate is None:
+                    raise IntegrityError(
+                        f'pack {pack_hex}: invalid object header at offset {offset} (pack corruption), run "borg check"'
+                    )
+                next_offset = self._find_header(scan_from, pack_size, validate)
+                if next_offset is None:
+                    logger.warning(
+                        f"pack {pack_hex}: invalid object header at offset {offset} and no object after "
+                        f"offset {scan_from}, skipping the remaining {pack_size - offset} bytes."
+                    )
+                    break
+                logger.warning(
+                    f"pack {pack_hex}: invalid object header at offset {offset}, "
+                    f"continuing at the object at offset {next_offset}."
                 )
+                if pending is not None:
+                    _, pending_offset, pending_size = pending
+                    if next_offset < pending_offset + pending_size:
+                        # the recovered object lies inside the bytes this one claims, so its size
+                        # field is wrong: it can not be read back, and index entries that overlap
+                        # are index corruption.
+                        logger.warning(
+                            f"pack {pack_hex}: object at offset {pending_offset} has a wrong size, dropping it."
+                        )
+                        pending = None
+                offset = next_offset
+                scan_from = next_offset + 1
+                continue
             obj_size = hdr_size + hdr.meta_size + hdr.data_size
-            if offset + obj_size > pack_size:
-                raise IntegrityError(
-                    f"pack {pack_hex}: object extends past end of file at offset {offset} "
-                    f'(pack corruption), run "borg check"'
-                )
-            yield hdr.chunk_id, offset, obj_size
+            if pending is not None:
+                yield pending
+            pending = (hdr.chunk_id, offset, obj_size)
+            scan_from = offset + 1
             offset += obj_size
+        if pending is not None:
+            yield pending
 
 
 def check_pack_objects(pack_hex, obj_ranges, pack_size):

src/borg/testsuite/archiver/check_cmd_test.py

@@ -10,6 +10,7 @@
 from ...constants import *  # NOQA
 from ...helpers import bin_to_hex, msgpack, CommandError, Error, IntegrityError, sig_int
 from ...manifest import Archives, Manifest
+from ...repoobj import RepoObj
 from ...repository import PackTracker, Repository
 from ..repository_test import fchunk, corrupt_chunk_on_disk
 from . import (
@@ -718,6 +719,33 @@ def test_extra_chunks(archivers, request):
     cmd(archiver, "check", "-v", exit_code=0)  # check does not deal with orphans anymore
 
 
+def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request):
+    """--repair rebuilds the chunks index from a pack whose object header is damaged.
+
+    A damaged header loses the object boundaries, so the rebuild scans for the next object that
+    authenticates and continues there. Authenticating needs the key, which --repair reads first.
+    """
+    archiver = request.getfixturevalue(archivers)
+    if archiver.get_kind() != "local":
+        pytest.skip("inspects the store directly")
+    check_cmd_setup(archiver)
+    cmd(archiver, "check", exit_code=0)
+
+    with Repository(archiver.repository_location, exclusive=True) as repository:
+        # damage the header of the second object of a pack that holds more than two.
+        by_pack = {}
+        for chunk_id, entry in repository.chunks.items():
+            by_pack.setdefault(entry.pack_id, []).append((entry.obj_offset, chunk_id))
+        pack_id, objs = next((p, sorted(o)) for p, o in by_pack.items() if len(o) > 2)
+        damaged_offset, _ = objs[1]
+        key = "packs/" + bin_to_hex(pack_id)
+        repository.store_store(key, corrupt(repository.store_load(key), damaged_offset))
+
+    output = cmd(archiver, "check", "--repair", "--debug", exit_code=0)
+    assert f"invalid object header at offset {damaged_offset}" in output
+    assert "continuing at the object at offset" in output  # the rebuild resumed at the next object
+
+
 def test_repair_finish_flushes_pack_writer(archivers, request):
     """finish() stores chunks re-added during --repair before it (re)builds the index (#10055).
 
@@ -735,6 +763,7 @@ def test_repair_finish_flushes_pack_writer(archivers, request):
         checker.repair = True
         checker.repository = repository
         checker.key = checker.make_key(repository)
+        checker.repo_objs = RepoObj(checker.key)
         checker.manifest = Manifest.load(repository, (Manifest.Operation.CHECK,), key=checker.key)
         # re-adding a chunk makes the chunks index no longer match the packs, so finish() rebuilds it.
         checker.chunks_modified = True