repoobj: address review, catch get() errors in --find-lost-archives, refs #10318

src/borg/archive.py

@@ -2515,8 +2515,8 @@ class ArchiveChecker:
             if sig_int:
                 break
             pi.show()
-            cdata = self.repository.get(chunk_id, read_data=False)  # only get metadata
             try:
+                cdata = self.repository.get(chunk_id, read_data=False)  # only get metadata
                 meta = self.repo_objs.parse_meta(chunk_id, cdata, ro_type=ROBJ_DONTCARE)
             except IntegrityErrorBase as exc:
                 logger.error("Skipping corrupted chunk: %s", exc)

src/borg/repoobj.py

@@ -80,7 +80,7 @@ class RepoObj:
     ObjHeader = namedtuple("ObjHeader", "magic version chunk_id meta_size data_size")
 
     @classmethod
-    def parse_header(cls, buf: bytes | memoryview) -> tuple:
+    def parse_header(cls, buf: bytes | memoryview) -> tuple["RepoObj.ObjHeader | None", str | None]:
         """Return (ObjHeader, None) if buf starts with a valid object header, (None, problem) otherwise.
 
         buf: object bytes starting at the header, bytes after the header are ignored.
@@ -89,7 +89,7 @@ class RepoObj:
         """
         hdr_size = cls.obj_header.size
         if len(buf) < hdr_size:
-            return None, f"object too small: expected at least {hdr_size} header bytes, got {len(buf)}"
+            return None, f"object too small: expected at least {hdr_size} header bytes, got {len(buf)} bytes"
         hdr = cls.ObjHeader(*cls.obj_header.unpack(buf[:hdr_size]))
         if hdr.magic != OBJ_MAGIC:
             return None, "no object header"
@@ -269,6 +269,7 @@ def object_validator(repo_objs):
     In the "authenticated-*" modes the tag is deterministic and binds an object to its chunk id
     alone. They therefore accept an object that a backed up file contains, at any offset in any pack.
     """
+    hdr_size = RepoObj.obj_header.size
     overhead = repo_objs.key.PAYLOAD_OVERHEAD  # the envelope adds a fixed number of bytes to the payload
 
     def validate(chunk_id, obj):
@@ -285,8 +286,8 @@ def object_validator(repo_objs):
         csize = meta.get("csize")
         if not isinstance(csize, int) or isinstance(csize, bool):  # msgpack unpacks true/false to bool
             return False
-        hdr, _ = RepoObj.parse_header(obj)
-        return hdr.data_size == csize + overhead
+        data_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj[:hdr_size])).data_size
+        return data_size == csize + overhead
 
     return validate
 

src/borg/repository.py

@@ -440,9 +440,11 @@ class PackReader:
     def _parse_header(hdr_data, offset, pack_size):
         """Return (ObjHeader, None) for a valid header at offset, (None, problem) otherwise.
 
-        hdr_data: the obj_header.size bytes at offset, a position in the pack. pack_size: the pack size in bytes.
+        hdr_data: at most obj_header.size bytes read at offset, a position in the pack.
+        pack_size: the pack size in bytes.
         Valid means: RepoObj.parse_header accepts hdr_data, and the object fits into the pack and is
-        at most MAX_DATA_SIZE bytes. problem is a message naming the check that failed.
+        at most MAX_DATA_SIZE bytes, the maximum size of a whole object. problem is a message naming the
+        check that failed.
         """
         hdr, problem = RepoObj.parse_header(hdr_data)
         if hdr is None:
@@ -634,10 +636,7 @@ def superseded_gap_ranges(reader, chunks, pack_id, obj_ranges, pack_size, *, val
         while offset < gend:
             # one read for the header and the metadata slot after it.
             buf = reader.read(offset, min(gend - offset, META_READ_SIZE))
-            if len(buf) < hdr_size:
-                hdr, problem = None, f"{len(buf)} bytes, too few for an object header,"
-            else:
-                hdr, problem = PackReader._parse_header(buf[:hdr_size], offset, pack_size)
+            hdr, problem = PackReader._parse_header(buf[:hdr_size], offset, pack_size)
             if hdr is not None and offset + hdr_size + hdr.meta_size + hdr.data_size > gend:
                 hdr, problem = None, "object reaching past its gap"
             if hdr is None:
@@ -1852,11 +1851,10 @@ class Repository:
                 # comes from the same index we already route with.
                 load_size = min(load_size, obj_size)
                 obj = reader.read(obj_offset, load_size)
-                parsed_hdr, problem = RepoObj.parse_header(obj)
-                if parsed_hdr is None:
+                hdr, problem = RepoObj.parse_header(obj)
+                if hdr is None:
                     raise IntegrityError(f"{problem} [id {id_hex}]")
-                hdr = obj[0:hdr_size]
-                meta_size = parsed_hdr.meta_size
+                meta_size = hdr.meta_size
                 if meta_size > extra_size:
                     # we did not get enough, need to load more, but not all.
                     # this should be rare, as chunk metadata is rather small usually.
@@ -1866,9 +1864,11 @@ class Repository:
                     obj = reader.read(obj_offset, retry_size)
                 meta = obj[hdr_size : hdr_size + meta_size]
                 if len(meta) != meta_size:
-                    raise IntegrityError(f"Object too small [id {id_hex}]: expected {meta_size}, got {len(meta)} bytes")
-                # hdr, meta are memoryviews for an in-memory pack; return them concatenated as bytes.
-                return bytes(hdr) + bytes(meta)
+                    raise IntegrityError(
+                        f"object too small: expected {meta_size} metadata bytes, got {len(meta)} bytes [id {id_hex}]"
+                    )
+                # obj, meta are memoryviews for an in-memory pack; return the header and meta as bytes.
+                return bytes(obj[:hdr_size]) + bytes(meta)
         except StoreObjectNotFound:
             if raise_missing:
                 raise self.ObjectNotFound(id, str(self._location)) from None

src/borg/testsuite/archiver/check_cmd_test.py

@@ -1093,6 +1093,22 @@ def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request, dama
     assert f"Store object packs/{bin_to_hex(pack_id)} is corrupted" in output
 
 
+def test_find_lost_archives_skips_chunk_with_corrupt_object_header(archivers, request):
+    """--find-lost-archives logs and skips an indexed chunk whose object header has a bad magic."""
+    archiver = request.getfixturevalue(archivers)
+    if archiver.get_kind() != "local":
+        pytest.skip("inspects the store directly")
+    check_cmd_setup(archiver)
+    with KeyedRepository(archiver.repository_location, exclusive=True) as repository:
+        chunk_id, entry = next(iter(repository.chunks.items()))
+        key = "packs/" + bin_to_hex(entry.pack_id)
+        repository.store_store(key, corrupt(repository.store_load(key), entry.obj_offset))
+    # without --repair, the archives check uses the stored chunks index, which still lists the damaged chunk.
+    output = cmd(archiver, "check", "--archives-only", "--find-lost-archives", exit_code=1)
+    assert f"Skipping corrupted chunk: Data integrity error: no object header [id {bin_to_hex(chunk_id)}]" in output
+    assert "Archive consistency check complete, problems found." in output
+
+
 def test_check_repair_validates_index_rebuild(archivers, request):
     """--repair leaves an object that fails validation out of the index and keeps the object after it (#9901)."""
     archiver = request.getfixturevalue(archivers)

src/borg/testsuite/repoobj_test.py

@@ -178,7 +178,7 @@ def test_parse_header(key):
 @pytest.mark.parametrize(
     "damage, problem",
     [
-        ("truncated", "object too small: expected at least 49 header bytes, got 48"),
+        ("truncated", "object too small: expected at least 49 header bytes, got 48 bytes"),
         ("magic", "no object header"),
         ("version", "unsupported object version 238"),
     ],

src/borg/testsuite/repository_test.py

@@ -1528,17 +1528,20 @@ def test_get_read_data_false_large_meta(tmp_path):
 @pytest.mark.parametrize(
     "damage, problem",
     [
-        ("truncated", "object too small: expected at least 49 header bytes, got 48"),
+        ("truncated", "object too small: expected at least 49 header bytes, got 48 bytes"),
         ("magic", "no object header"),
         ("version", "unsupported object version 238"),
+        ("truncated_meta", "object too small: expected 4 metadata bytes, got 2 bytes"),
     ],
-    ids=["truncated", "magic", "version"],
+    ids=["truncated", "magic", "version", "truncated_meta"],
 )
-def test_get_read_data_false_rejects_header(tmp_path, damage, problem):
-    # get(read_data=False) sizes its metadata read by meta_size, so it checks the header first.
+def test_get_read_data_false_rejects_damaged_object(tmp_path, damage, problem):
+    # get(read_data=False) raises IntegrityError for an invalid header or truncated metadata.
     chunk = bytearray(fchunk(b"DATA", meta=b"META"))
     if damage == "truncated":
         chunk = chunk[: RepoObj.obj_header.size - 1]
+    elif damage == "truncated_meta":
+        chunk = chunk[: RepoObj.obj_header.size + 2]
     elif damage == "magic":
         chunk[0] ^= 0xFF
     else:
@@ -3149,7 +3152,10 @@ def test_superseded_gap_ranges_warns_where_it_keeps_bytes(tmp_path, caplog):
         f"pack {pack_hex}: no object header at offset {len(rejected)} in a gap, "
         f"keeping the remaining {len(garbage)} bytes of the gap." in caplog.text
     )
-    assert f"pack {pack_hex}: 3 bytes, too few for an object header, at offset 0 in a gap" in caplog.text
+    assert (
+        f"pack {pack_hex}: object too small: expected at least {RepoObj.obj_header.size} header bytes, got 3 bytes "
+        f"at offset 0 in a gap" in caplog.text
+    )
 
 
 def test_superseded_gap_ranges_ends_at_an_object_reaching_past_the_gap(tmp_path, caplog):