diff --git a/src/borg/archive.py b/src/borg/archive.py index 33687e832..ab35a3f01 100644 --- a/src/borg/archive.py +++ b/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) diff --git a/src/borg/repoobj.py b/src/borg/repoobj.py index 574183adb..6f5960582 100644 --- a/src/borg/repoobj.py +++ b/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 diff --git a/src/borg/repository.py b/src/borg/repository.py index 87b4d9772..9768f99dd 100644 --- a/src/borg/repository.py +++ b/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 diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index cdd6bb61b..8ce32ebac 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/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) diff --git a/src/borg/testsuite/repoobj_test.py b/src/borg/testsuite/repoobj_test.py index 627dd7dab..b05363ba9 100644 --- a/src/borg/testsuite/repoobj_test.py +++ b/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"), ], diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 58ad72f62..c281b3dd9 100644 --- a/src/borg/testsuite/repository_test.py +++ b/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):