From a21d85554b3ab477c111439693e60a4d5536f279 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Thu, 13 Aug 2026 03:30:06 +0530 Subject: [PATCH 1/5] check --repair: resync past corrupt object headers when rebuilding the chunks index, #8476 When check --repair rebuilds the chunks index from the packs, a corrupt object header now makes iter_headers resync rather than raise: it takes a validate function and scans forward for the next object, in 1 MiB windows that overlap by one header so a header on a window boundary is still found. Repository-only checks pass no validate and keep raising IntegrityError on a corrupt header. OBJ_MAGIC also occurs inside payloads, so a candidate is accepted only when it authenticates. For AEAD keys, decrypting the metadata authenticates it against the header's magic, version and chunk_id, so the walk confirms a chunk id from a few hundred bytes. Keys that authenticate by chunk_id == id_hash(content) (id_check_is_authentication) read the whole object and parse() at the "repair" id place; validate.needs_data selects between the two. Authentication needs the key, so check --repair makes it before the rebuild with manifest_only=True. A repair that cannot read the manifest has no key and walks without resyncing. --- docs/internals/packs.rst | 28 +++- src/borg/archive.py | 39 ++++- src/borg/cache.py | 11 +- src/borg/repository.py | 103 +++++++++--- src/borg/testsuite/archiver/check_cmd_test.py | 28 ++++ src/borg/testsuite/cache_test.py | 23 ++- src/borg/testsuite/repository_test.py | 147 ++++++++++++++++++ 7 files changed, 350 insertions(+), 29 deletions(-) diff --git a/docs/internals/packs.rst b/docs/internals/packs.rst index 0f8d6acec9..5e43d1a23f 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -91,10 +91,30 @@ 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 forward for the next blob and +resumes there, so the blobs after the damaged part of the pack are still found. + +``OBJ_MAGIC`` occurs inside the payloads as well, and in ``none`` and +``authenticated`` mode the payloads are user content stored as it is, so a +backed up file can contain something shaped like a blob. The scan therefore +accepts a candidate only if it parses. For the AEAD keys it reads the header and +the encrypted metadata, a few hundred bytes: decrypting the metadata +authenticates it together with the header's magic, version and chunk_id, which +are its AAD (additional authenticated data: authenticated with the ciphertext, +but not encrypted). The other keys authenticate by ``chunk_id == id_hash(content)`` +(``KeyBase.id_check_is_authentication``), which needs the blob's data, so for +those the scan reads the whole blob. The key is needed either way; a repair that +cannot read the manifest walks without scanning. + +``data_size`` is not part of that 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:: diff --git a/src/borg/archive.py b/src/borg/archive.py index 5c7f2f2ddc..58e2a444b9 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2081,6 +2081,32 @@ 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 holds an object's header and encrypted metadata, plus its encrypted data when + validate.needs_data is set. For most keys, decrypting the metadata authenticates it against the + header (magic, version, chunk_id), so the metadata alone decides. Keys that authenticate by + chunk_id == id_hash(content) (id_check_is_authentication) need the data; for them + validate.needs_data is set and parse() checks that id at the "repair" id place. + """ + needs_data = repo_objs.key.id_check_is_authentication + + def validate(chunk_id, obj): + try: + if needs_data: + repo_objs.parse(chunk_id, obj, ro_type=ROBJ_DONTCARE, assert_id_place="repair") + else: + repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) + except Exception: + # authentication, id check, msgpack or decompression can each raise on non-object bytes. + return False + return True + + validate.needs_data = needs_data + 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 +2161,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) + # Under --repair, validate lets the rebuild resync past a corrupt object header (see resync_validator). + # It authenticates objects with the key, so make the key first; manifest_only=True makes make_key use + # the manifest, not self.chunks, which is still unset here. + if 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) diff --git a/src/borg/cache.py b/src/borg/cache.py index c49277d745..0b9ce0f8dd 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -857,7 +857,13 @@ 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. @@ -952,7 +958,8 @@ 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(): + # validate makes iter_headers resync past a corrupt object header and index the objects after it. + 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 diff --git a/src/borg/repository.py b/src/borg/repository.py index a007f7561f..c7b1484d38 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -30,7 +30,7 @@ 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__) @@ -38,6 +38,9 @@ # 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,24 +365,73 @@ 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 confirms it. + """ + 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 (Repository.put), so a larger candidate is a + # false match on OBJ_MAGIC in a payload. + if obj_size <= MAX_DATA_SIZE: + size = obj_size if validate.needs_data else hdr_size + hdr.meta_size + 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): returns whether obj is a repo object with id chunk_id, where obj is + its header and metadata, plus its data when validate.needs_data is set. When validate is + given, a corrupt header makes the walk resync: it scans for the next object validate accepts + (see _find_header), continues there, and logs the skipped bytes. """ pack_hex = bin_to_hex(self.pack_id) if self.pack_id is not None else "" pack_size = self.size() @@ -389,17 +441,26 @@ def iter_headers(self): 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(offset + 1, pack_size, validate) + if next_offset is None: + logger.warning( + f"pack {pack_hex}: invalid object header at offset {offset} and none after it, " + f"skipping the remaining {pack_size - offset} bytes." + ) + break + logger.warning( + f"pack {pack_hex}: invalid object header at offset {offset}, " + f"skipping {next_offset - offset} bytes to the next one." ) + offset = next_offset + 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 offset += obj_size diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 34a9ee5cb3..911548f100 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -718,6 +718,34 @@ 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 index from a pack whose object header is damaged. + + A damaged header makes the walk lose the object boundaries, so the rebuild scans for the next + object that authenticates and carries on there. That needs the key, which --repair makes before + the rebuild. Repairing the pack itself is a separate step, see #10026. + """ + 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 "bytes to the next one" 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). diff --git a/src/borg/testsuite/cache_test.py b/src/borg/testsuite/cache_test.py index 24e64488d8..192d3006dd 100644 --- a/src/borg/testsuite/cache_test.py +++ b/src/borg/testsuite/cache_test.py @@ -26,7 +26,8 @@ ) from ..hashindex import ChunkIndex, ChunkIndexEntry from ..crypto.key import AESOCBKey -from ..helpers import safe_ns +from ..helpers import bin_to_hex, safe_ns +from ..helpers import IntegrityError from ..helpers.msgpack import int_to_timestamp from ..manifest import Manifest from ..repository import Repository @@ -505,6 +506,26 @@ def test_close_consolidates_fragments_across_sessions(tmp_path, monkeypatch): assert cid in index +def test_build_chunkindex_repair_resyncs_after_corrupt_header(tmp_path): + """A corrupt object header fails the rebuild, but with repair=True the rest of the pack is indexed.""" + from .repository_test import accept_all, fchunk + + obj1 = bytearray(fchunk(b"first", chunk_id=H(90))) + obj2 = fchunk(b"second", chunk_id=H(91)) + obj1[0] ^= 0xFF # break the magic of the first object's header + pack_id = H(92) + with Repository(os.fspath(tmp_path / "repository"), exclusive=True, create=True) as repository: + repository.store_store("packs/" + bin_to_hex(pack_id), bytes(obj1) + obj2) + with pytest.raises(IntegrityError): + build_chunkindex_from_repo(repository, slow_rebuild=True) + # accept_all: accepts every candidate, so this exercises the plumbing only. + index = build_chunkindex_from_repo(repository, slow_rebuild=True, validate=accept_all) + assert H(91) in index # found by resyncing past the damaged header + assert H(90) not in index # its header is gone, so the object can not be indexed + assert index[H(91)].pack_id == pack_id + assert index[H(91)].obj_offset == len(obj1) + + def test_repack_leaves_sealed_untouched_and_reconstructs(tmp_path, monkeypatch): """Sealed (>= MIN) fragments survive a repack; build_chunkindex_from_repo reconstructs the index.""" monkeypatch.setattr(cache_mod, "CHUNKINDEX_FRAGMENT_ENTRIES_MIN", 1000) diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 2352e5ad79..11aab716c9 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -13,6 +13,11 @@ from ..constants import MAX_CLOCK_SKEW from ..helpers import IntegrityError, Location, bin_to_hex from ..hashindex import ChunkIndex +from .. import repository as repository_module +from ..archive import resync_validator +from ..compress import CNONE +from ..constants import ROBJ_FILE_STREAM +from ..crypto.key import CHPOKey, PlaintextKey from ..repository import Repository, MAX_DATA_SIZE, propagate_rsh, rest_serve_command, PackWriter, PackReader from ..repository import PackTracker from ..repoobj import RepoObj, OBJ_MAGIC, OBJ_VERSION @@ -1868,6 +1873,148 @@ def test_pack_reader_raises_on_object_past_end_of_pack_through_store(tmp_path): list(reader.iter_headers()) +def test_pack_reader_raises_on_unsupported_version(): + obj = bytearray(fchunk(b"data", chunk_id=H(7))) + obj[len(OBJ_MAGIC)] = 0xEE # version byte + with pytest.raises(IntegrityError): + list(PackReader(pack_contents=bytes(obj)).iter_headers()) + + +def accept_all(chunk_id, obj): + # validate stand-in: accepts every candidate. + return True + + +accept_all.needs_data = False + + +def test_pack_reader_resync_skips_to_next_object(): + # after a corrupt header the walk continues at the next object. + obj1 = bytearray(fchunk(b"payload-one", meta=b"meta1", chunk_id=H(1))) + obj2 = fchunk(b"payload-two", meta=b"meta2", chunk_id=H(2)) + obj1[0] ^= 0xFF # break the magic of the first object's header + reader = PackReader(pack_contents=bytes(obj1) + obj2) + assert list(reader.iter_headers(validate=accept_all)) == [(H(2), len(obj1), len(obj2))] + + +def test_pack_reader_resync_recovers_from_corrupted_size(): + # a header whose sizes point past the pack, so the next object is found by scanning. + obj1 = bytearray(fchunk(b"payload-one", meta=b"meta1", chunk_id=H(1))) + obj2 = fchunk(b"payload-two", meta=b"meta2", chunk_id=H(2)) + obj3 = fchunk(b"payload-three", chunk_id=H(3)) + # the header's data_size field (magic 8, version 1, chunk_id 32, meta_size 4, data_size 4), + # set to a value reaching far past the end of the pack: + obj1[45:49] = b"\xff\xff\xff\x00" + pack = bytes(obj1) + obj2 + obj3 + reader = PackReader(pack_contents=pack) + assert list(reader.iter_headers(validate=accept_all)) == [ + (H(2), len(obj1), len(obj2)), + (H(3), len(obj1) + len(obj2), len(obj3)), + ] + + +def test_pack_reader_resync_ignores_magic_in_payload(): + # both headers are broken, so the scan runs into the OBJ_MAGIC in obj2's payload before obj3. + obj1 = bytearray(fchunk(b"data", chunk_id=H(1))) + obj2 = bytearray(fchunk(OBJ_MAGIC + b"looks like a header, is not", chunk_id=H(2))) + obj3 = fchunk(b"payload-three", chunk_id=H(3)) + obj1[0] ^= 0xFF + obj2[0] ^= 0xFF + reader = PackReader(pack_contents=bytes(obj1) + bytes(obj2) + obj3) + assert list(reader.iter_headers(validate=accept_all)) == [(H(3), len(obj1) + len(obj2), len(obj3))] + + +def test_pack_reader_resync_finds_header_across_window_boundary(monkeypatch): + # the next header straddles a scan window boundary. + monkeypatch.setattr(repository_module, "RESYNC_WINDOW_SIZE", 64) + obj1 = bytearray(fchunk(b"x" * 100, chunk_id=H(1))) + obj2 = fchunk(b"payload-two", chunk_id=H(2)) + obj1[0] ^= 0xFF + reader = PackReader(pack_contents=bytes(obj1) + obj2) + assert list(reader.iter_headers(validate=accept_all)) == [(H(2), len(obj1), len(obj2))] + + +def test_pack_reader_resync_no_further_header(): + # no object after the damage: the walk ends with what it found. + obj = fchunk(b"data", chunk_id=H(1)) + pack = obj + b"\xaa" * 200 + reader = PackReader(pack_contents=pack) + assert list(reader.iter_headers(validate=accept_all)) == [(H(1), 0, len(obj))] + + +def aead_repo_objs(tmp_path): + # a RepoObj with an AEAD key, whose metadata authenticates on its own. + repository = Repository(str(tmp_path / "repo"), create=True) + key = CHPOKey(repository) + key.init_from_random_data() + key.init_ciphers() + return RepoObj(key) + + +def test_pack_reader_resync_rejects_metadata_that_does_not_authenticate(tmp_path): + # bytes with a well-formed header whose metadata does not decrypt: the scan must walk past them. + repo_objs = aead_repo_objs(tmp_path) + data = b"the real next object" + real_id = repo_objs.id_hash(data) + obj1 = bytearray(repo_objs.format(repo_objs.id_hash(b"first"), {}, b"first", ro_type=ROBJ_FILE_STREAM)) + obj1[0] ^= 0xFF # break obj1's header, so the walk has to resync + garbage = fchunk(b"payload", meta=b"not encrypted metadata", chunk_id=H(9)) + obj2 = repo_objs.format(real_id, {}, data, ro_type=ROBJ_FILE_STREAM) + reader = PackReader(pack_contents=bytes(obj1) + garbage + obj2) + headers = list(reader.iter_headers(validate=resync_validator(repo_objs))) + assert headers == [(real_id, len(obj1) + len(garbage), len(obj2))] + + +def test_pack_reader_resync_accepts_an_object_with_corrupt_data(tmp_path): + # the AEAD keys authenticate the metadata, so the scan resyncs at an object with damaged data. + # Reading that object reports the damage. + repo_objs = aead_repo_objs(tmp_path) + data = b"the real next object" + real_id = repo_objs.id_hash(data) + obj1 = bytearray(repo_objs.format(repo_objs.id_hash(b"first"), {}, b"first", ro_type=ROBJ_FILE_STREAM)) + obj1[0] ^= 0xFF # break obj1's header, so the walk has to resync + obj2 = bytearray(repo_objs.format(real_id, {}, data, ro_type=ROBJ_FILE_STREAM)) + obj2[-1] ^= 0xFF # damage the encrypted data, leaving the header and the metadata intact + reader = PackReader(pack_contents=bytes(obj1) + bytes(obj2)) + headers = list(reader.iter_headers(validate=resync_validator(repo_objs))) + assert headers == [(real_id, len(obj1), len(obj2))] + with pytest.raises(IntegrityError): + repo_objs.parse(real_id, bytes(obj2), ro_type=ROBJ_FILE_STREAM) + + +def test_pack_reader_resync_rejects_user_content_that_looks_like_an_object(tmp_path): + # In "none" mode with no compression, user content lands in the pack as it is, so a backed up + # file can contain something shaped like an object. Those keys authenticate by the id check over + # the content, so the scan reads whole candidates. + repository = Repository(str(tmp_path / "repo"), create=True) + repo_objs = RepoObj(PlaintextKey(repository)) + assert resync_validator(repo_objs).needs_data + repo_objs.compressor = CNONE() + decoy = bytearray(repo_objs.format(repo_objs.id_hash(b"decoy"), {}, b"decoy", ro_type=ROBJ_FILE_STREAM)) + decoy[-1] ^= 0xFF # its content no longer hashes to the id in its header + content = bytes(decoy) # a user stores exactly those bytes in a file + obj1 = bytearray(repo_objs.format(repo_objs.id_hash(content), {}, content, ro_type=ROBJ_FILE_STREAM)) + assert content in obj1 # the decoy is in the pack verbatim + obj1[0] ^= 0xFF # break obj1's header, so the walk resyncs and runs into the decoy + data = b"the real next object" + real_id = repo_objs.id_hash(data) + obj2 = repo_objs.format(real_id, {}, data, ro_type=ROBJ_FILE_STREAM) + reader = PackReader(pack_contents=bytes(obj1) + obj2) + headers = list(reader.iter_headers(validate=resync_validator(repo_objs))) + assert headers == [(real_id, len(obj1), len(obj2))] + + +def test_pack_reader_resync_through_store(tmp_path): + obj1 = bytearray(fchunk(b"FIRST", chunk_id=H(47))) + obj2 = fchunk(b"SECOND", chunk_id=H(48)) + obj1[0] ^= 0xFF + pack_id = H(50) + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + repository.store_store("packs/" + bin_to_hex(pack_id), bytes(obj1) + obj2) + reader = PackReader(repository.store, pack_id) + assert list(reader.iter_headers(validate=accept_all)) == [(H(48), len(obj1), len(obj2))] + + def test_pack_reader_size(tmp_path): obj = fchunk(b"data", meta=b"meta", chunk_id=H(6)) assert PackReader(pack_contents=obj).size() == len(obj) From 90c4c07713b3921534d97e13cf5b79d22d9965ed Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Sat, 15 Aug 2026 19:28:07 +0530 Subject: [PATCH 2/5] check --repair: validate a resync candidate from its metadata slot alone, #8476 Every key mode covers the object header by the metadata slot's AAD, so parse_meta confirms a candidate and validate.needs_data is gone. --- docs/internals/packs.rst | 25 ++++++++--------- src/borg/archive.py | 28 ++++++++----------- src/borg/cache.py | 3 +- src/borg/repository.py | 16 +++++------ src/borg/testsuite/archiver/check_cmd_test.py | 7 ++--- src/borg/testsuite/cache_test.py | 6 ++-- src/borg/testsuite/repository_test.py | 20 ++++++------- 7 files changed, 49 insertions(+), 56 deletions(-) diff --git a/docs/internals/packs.rst b/docs/internals/packs.rst index 5e43d1a23f..0fdf58cb59 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -100,19 +100,18 @@ repair walk (``iter_headers(validate=...)``, used when ``borg check --repair`` rebuilds the chunks index from the packs) scans forward for the next blob and resumes there, so the blobs after the damaged part of the pack are still found. -``OBJ_MAGIC`` occurs inside the payloads as well, and in ``none`` and -``authenticated`` mode the payloads are user content stored as it is, so a -backed up file can contain something shaped like a blob. The scan therefore -accepts a candidate only if it parses. For the AEAD keys it reads the header and -the encrypted metadata, a few hundred bytes: decrypting the metadata -authenticates it together with the header's magic, version and chunk_id, which -are its AAD (additional authenticated data: authenticated with the ciphertext, -but not encrypted). The other keys authenticate by ``chunk_id == id_hash(content)`` -(``KeyBase.id_check_is_authentication``), which needs the blob's data, so for -those the scan reads the whole blob. The key is needed either way; a repair that -cannot read the manifest walks without scanning. - -``data_size`` is not part of that AAD, so accepting a candidate authenticates +``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. diff --git a/src/borg/archive.py b/src/borg/archive.py index 58e2a444b9..f8c950d492 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2084,26 +2084,22 @@ def __next__(self): def resync_validator(repo_objs): """Return validate(chunk_id, obj): True if obj is the repo object with id chunk_id. - obj holds an object's header and encrypted metadata, plus its encrypted data when - validate.needs_data is set. For most keys, decrypting the metadata authenticates it against the - header (magic, version, chunk_id), so the metadata alone decides. Keys that authenticate by - chunk_id == id_hash(content) (id_check_is_authentication) need the data; for them - validate.needs_data is set and parse() checks that id at the "repair" id place. + 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. """ - needs_data = repo_objs.key.id_check_is_authentication def validate(chunk_id, obj): try: - if needs_data: - repo_objs.parse(chunk_id, obj, ro_type=ROBJ_DONTCARE, assert_id_place="repair") - else: - repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) + repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) except Exception: - # authentication, id check, msgpack or decompression can each raise on non-object bytes. + # arbitrary bytes fail the tag, the msgpack unpacking or the length checks. return False return True - validate.needs_data = needs_data return validate @@ -2161,10 +2157,10 @@ 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. - # Under --repair, validate lets the rebuild resync past a corrupt object header (see resync_validator). - # It authenticates objects with the key, so make the key first; manifest_only=True makes make_key use - # the manifest, not self.chunks, which is still unset here. - if self.key is None: + # --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: diff --git a/src/borg/cache.py b/src/borg/cache.py index 0b9ce0f8dd..d146af0cd9 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -867,6 +867,8 @@ def build_chunkindex_from_repo( ): # 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: @@ -958,7 +960,6 @@ def build_chunkindex_from_repo( repository._lock_refresh() pi.show(increase=1) pack_id = hex_to_bin(info.name) - # validate makes iter_headers resync past a corrupt object header and index the objects after it. for chunk_id, obj_offset, obj_size in PackReader(repository.store, pack_id).iter_headers(validate=validate): num_chunks += 1 chunks[chunk_id] = ChunkIndexEntry( diff --git a/src/borg/repository.py b/src/borg/repository.py index c7b1484d38..1e8afd504e 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -388,7 +388,7 @@ def _find_header(self, offset, pack_size, validate): 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 confirms it. + 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: @@ -404,10 +404,10 @@ def _find_header(self, offset, pack_size, validate): 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 (Repository.put), so a larger candidate is a - # false match on OBJ_MAGIC in a payload. + # 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 = obj_size if validate.needs_data else hdr_size + hdr.meta_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) @@ -428,10 +428,10 @@ def iter_headers(self, validate=None): 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. - validate(chunk_id, obj): returns whether obj is a repo object with id chunk_id, where obj is - its header and metadata, plus its data when validate.needs_data is set. When validate is - given, a corrupt header makes the walk resync: it scans for the next object validate accepts - (see _find_header), continues there, and logs the skipped bytes. + 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 for the next object validate accepts, logs how many bytes that skipped and + continues there. """ pack_hex = bin_to_hex(self.pack_id) if self.pack_id is not None else "" pack_size = self.size() diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 911548f100..76d2e2cc9b 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -719,11 +719,10 @@ def test_extra_chunks(archivers, request): def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request): - """--repair rebuilds the index from a pack whose object header is damaged. + """--repair rebuilds the chunks index from a pack whose object header is damaged. - A damaged header makes the walk lose the object boundaries, so the rebuild scans for the next - object that authenticates and carries on there. That needs the key, which --repair makes before - the rebuild. Repairing the pack itself is a separate step, see #10026. + 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": diff --git a/src/borg/testsuite/cache_test.py b/src/borg/testsuite/cache_test.py index 192d3006dd..cf945247ba 100644 --- a/src/borg/testsuite/cache_test.py +++ b/src/borg/testsuite/cache_test.py @@ -507,7 +507,7 @@ def test_close_consolidates_fragments_across_sessions(tmp_path, monkeypatch): def test_build_chunkindex_repair_resyncs_after_corrupt_header(tmp_path): - """A corrupt object header fails the rebuild, but with repair=True the rest of the pack is indexed.""" + """A corrupt object header fails the rebuild; with validate, the objects after it are indexed.""" from .repository_test import accept_all, fchunk obj1 = bytearray(fchunk(b"first", chunk_id=H(90))) @@ -518,10 +518,10 @@ def test_build_chunkindex_repair_resyncs_after_corrupt_header(tmp_path): repository.store_store("packs/" + bin_to_hex(pack_id), bytes(obj1) + obj2) with pytest.raises(IntegrityError): build_chunkindex_from_repo(repository, slow_rebuild=True) - # accept_all: accepts every candidate, so this exercises the plumbing only. + # accept_all takes any candidate, so this covers the plumbing, not the authentication. index = build_chunkindex_from_repo(repository, slow_rebuild=True, validate=accept_all) assert H(91) in index # found by resyncing past the damaged header - assert H(90) not in index # its header is gone, so the object can not be indexed + assert H(90) not in index # its header is damaged, so its id is unknown assert index[H(91)].pack_id == pack_id assert index[H(91)].obj_offset == len(obj1) diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 11aab716c9..6bd0bf022f 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -17,7 +17,7 @@ from ..archive import resync_validator from ..compress import CNONE from ..constants import ROBJ_FILE_STREAM -from ..crypto.key import CHPOKey, PlaintextKey +from ..crypto.key import CHPOKey, ChecksumKey from ..repository import Repository, MAX_DATA_SIZE, propagate_rsh, rest_serve_command, PackWriter, PackReader from ..repository import PackTracker from ..repoobj import RepoObj, OBJ_MAGIC, OBJ_VERSION @@ -1885,9 +1885,6 @@ def accept_all(chunk_id, obj): return True -accept_all.needs_data = False - - def test_pack_reader_resync_skips_to_next_object(): # after a corrupt header the walk continues at the next object. obj1 = bytearray(fchunk(b"payload-one", meta=b"meta1", chunk_id=H(1))) @@ -1982,16 +1979,17 @@ def test_pack_reader_resync_accepts_an_object_with_corrupt_data(tmp_path): repo_objs.parse(real_id, bytes(obj2), ro_type=ROBJ_FILE_STREAM) -def test_pack_reader_resync_rejects_user_content_that_looks_like_an_object(tmp_path): - # In "none" mode with no compression, user content lands in the pack as it is, so a backed up - # file can contain something shaped like an object. Those keys authenticate by the id check over - # the content, so the scan reads whole candidates. +def test_pack_reader_resync_rejects_damaged_user_content_without_a_key(tmp_path): + # In "none-*" mode with no compression, user content lands in the pack as it is, so a backed up + # file can contain something shaped like an object. The metadata slot's checksum covers the + # object header, so damaged candidate bytes are still ruled out - what these modes can not rule + # out is an intact object put into a file on purpose, there being no secret to tell them apart. repository = Repository(str(tmp_path / "repo"), create=True) - repo_objs = RepoObj(PlaintextKey(repository)) - assert resync_validator(repo_objs).needs_data + repo_objs = RepoObj(ChecksumKey(repository)) repo_objs.compressor = CNONE() decoy = bytearray(repo_objs.format(repo_objs.id_hash(b"decoy"), {}, b"decoy", ro_type=ROBJ_FILE_STREAM)) - decoy[-1] ^= 0xFF # its content no longer hashes to the id in its header + hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(bytes(decoy[: RepoObj.obj_header.size]))) + decoy[RepoObj.obj_header.size + hdr.meta_size - 1] ^= 0xFF # damage its metadata slot content = bytes(decoy) # a user stores exactly those bytes in a file obj1 = bytearray(repo_objs.format(repo_objs.id_hash(content), {}, content, ro_type=ROBJ_FILE_STREAM)) assert content in obj1 # the decoy is in the pack verbatim From 46162be7f8c9632120404fc21b947b830597737a Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Sun, 23 Aug 2026 18:46:39 +0530 Subject: [PATCH 3/5] check --repair: validate every object header the repair walk accepts, #8476 A meta_size or data_size corrupted to a value that keeps the object inside the pack leaves the header parseable, so a walk that only checks the header follows a wrong offset and loses the intact objects after it. The walk now validates every header it accepts, not only the candidates the resync scan finds: it reads the metadata slot along with the header and checks it. The slot's tag covers the header's magic, version and chunk id and the slot itself, so a corrupted meta_size fails it. data_size, the one header field outside the tag, must equal csize - the data slot's payload size, recorded in the tagged metadata - plus the key's fixed envelope overhead. A header that fails makes the walk scan forward for the next object that validates and resume there. The object with the failed header is dropped. --- docs/internals/packs.rst | 32 ++--- src/borg/archive.py | 19 ++- src/borg/cache.py | 4 +- src/borg/repository.py | 56 +++++---- src/borg/testsuite/archiver/check_cmd_test.py | 18 ++- src/borg/testsuite/cache_test.py | 3 +- src/borg/testsuite/repository_test.py | 119 +++++++++++++++++- 7 files changed, 199 insertions(+), 52 deletions(-) diff --git a/docs/internals/packs.rst b/docs/internals/packs.rst index 0fdf58cb59..40ed95aaea 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -97,23 +97,27 @@ 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 forward for the next blob and -resumes there, so the blobs after the damaged part of the pack are still found. +rebuilds the chunks index from the packs) validates every header it walks, +reading the metadata slot along with it: the slot's tag covers the header AAD +described above and the slot itself, so a corrupted magic, version, chunk id or +``meta_size`` fails it, and ``data_size`` - the one header field outside the +tag - must equal ``csize`` (the data payload size recorded in the tagged +metadata) plus the key's fixed envelope overhead. A header that fails makes the +walk scan for the next blob that validates and resume there, so the blobs after +the damaged one are still found; the damaged blob itself is dropped, 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. +backed up file can contain something shaped like a blob. The scan therefore +accepts a candidate only when it validates like any walked header. Validating +needs the key, so a repair that cannot read the manifest walks without it. + +In the ``none-*`` modes the tag is an unkeyed checksum, so the walk accepts any +well-formed blob, including one a backed up file contains - but it only scans +into a payload after the blob owning it failed to validate. + +Bit flips in the data are caught when the blob is read, on that blob alone. Blobs follow one another contiguously with no padding:: diff --git a/src/borg/archive.py b/src/borg/archive.py index f8c950d492..be5fc003e4 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2086,19 +2086,25 @@ def resync_validator(repo_objs): 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). + data: bytes the tag covers without being part of the ciphertext) and over the slot itself, so a + wrong meta_size fails it too. data_size, the one header field the tag does not cover, must + match csize - the data slot's payload size, recorded in the tagged metadata - plus the key's + fixed envelope overhead. 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. """ + 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): try: - repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) + meta = 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 + data_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj[:hdr_size])).data_size + return data_size == meta["csize"] + overhead return validate @@ -2701,7 +2707,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.") diff --git a/src/borg/cache.py b/src/borg/cache.py index d146af0cd9..b1fbfe674f 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -867,8 +867,8 @@ def build_chunkindex_from_repo( ): # 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. + # validate: handed to PackReader.iter_headers when rebuilding from the packs, making it check + # every object header it walks and resync past the ones that fail 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: diff --git a/src/borg/repository.py b/src/borg/repository.py index 1e8afd504e..e02e6d8676 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -40,6 +40,9 @@ # how much of a pack PackReader reads at once when searching for the next object header. RESYNC_WINDOW_SIZE = 1024 * 1024 +# how much PackReader reads per object when it validates the headers it walks: the header and, +# for the usual metadata slot size, the slot as well, so validating needs no second read. +VALIDATE_READ_SIZE = 1024 def repo_lister(repository, *, limit=None): @@ -374,12 +377,14 @@ def size(self): 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. + Valid means: OBJ_MAGIC, a supported version, and an object that fits into the pack and is + at most MAX_DATA_SIZE bytes, the limit put() enforces on a whole object. """ 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: + obj_size = RepoObj.obj_header.size + hdr.meta_size + hdr.data_size + if obj_size > MAX_DATA_SIZE or offset + obj_size > pack_size: return None return hdr @@ -403,16 +408,12 @@ def _find_header(self, offset, pack_size, validate): 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 + 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) @@ -421,27 +422,34 @@ def _find_header(self, offset, pack_size, validate): def iter_headers(self, validate=None): """Yield (chunk_id, offset, size) for each object by walking the fixed object headers. - 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. + The walk reads one range per object (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. + A header that _parse_header does not accept means a corrupt pack and raises IntegrityError. + A read shorter than a header ends the walk: that is the end of the pack. 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 for the next object validate accepts, logs how many bytes that skipped and - continues there. + repo object with id chunk_id. Given one, the walk validates every header, reading the + metadata slot along with it, and a header that fails makes the walk resync rather than + raise: it scans from just past that header for the next object validate accepts and + continues there. The object with the failed header is dropped - its id, its extent or its + metadata is wrong, so it can not be read back. """ pack_hex = bin_to_hex(self.pack_id) if self.pack_id is not None else "" pack_size = self.size() hdr_size = RepoObj.obj_header.size + read_size = VALIDATE_READ_SIZE if validate is not None else hdr_size offset = 0 while True: - hdr_data = self.read(offset, hdr_size) - if len(hdr_data) < hdr_size: + buf = self.read(offset, read_size) + if len(buf) < hdr_size: break # clean EOF, or trailing partial bytes - hdr = self._parse_header(hdr_data, offset, pack_size) + hdr = self._parse_header(buf[:hdr_size], offset, pack_size) + if hdr is not None and validate is not None: + size = hdr_size + hdr.meta_size # the bytes validate looks at + obj = buf[:size] if size <= len(buf) else self.read(offset, size) + if not validate(hdr.chunk_id, obj): + hdr = None if hdr is None: if validate is None: raise IntegrityError( @@ -456,7 +464,7 @@ def iter_headers(self, validate=None): break logger.warning( f"pack {pack_hex}: invalid object header at offset {offset}, " - f"skipping {next_offset - offset} bytes to the next one." + f"continuing at the object at offset {next_offset}." ) offset = next_offset continue diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 76d2e2cc9b..1b695094e9 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/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,11 +719,14 @@ 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): +@pytest.mark.parametrize("damaged_field", ["magic", "data_size"]) +def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request, damaged_field): """--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. + A damaged data_size leaves the header parseable, so the rebuild catches it against the csize + in the authenticated metadata. """ archiver = request.getfixturevalue(archivers) if archiver.get_kind() != "local": @@ -736,13 +740,18 @@ def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request): 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] + damaged_offset, damaged_id = objs[1] + next_offset, next_id = objs[2] + field_offset = {"magic": 0, "data_size": 45}[damaged_field] # magic 8, version 1, chunk_id 32, meta_size 4 key = "packs/" + bin_to_hex(pack_id) - repository.store_store(key, corrupt(repository.store_load(key), damaged_offset)) + repository.store_store(key, corrupt(repository.store_load(key), damaged_offset + field_offset)) output = cmd(archiver, "check", "--repair", "--debug", exit_code=0) assert f"invalid object header at offset {damaged_offset}" in output - assert "bytes to the next one" in output # the rebuild resumed at the next object + assert f"continuing at the object at offset {next_offset}" in output # the rebuild resumed at the next object + with Repository(archiver.repository_location, exclusive=True) as repository: + assert damaged_id not in repository.chunks # the damaged object can not be read back, so it is not indexed + assert repository.chunks[next_id].obj_offset == next_offset # the one after it is def test_repair_finish_flushes_pack_writer(archivers, request): @@ -762,6 +771,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 diff --git a/src/borg/testsuite/cache_test.py b/src/borg/testsuite/cache_test.py index cf945247ba..948f9d23c0 100644 --- a/src/borg/testsuite/cache_test.py +++ b/src/borg/testsuite/cache_test.py @@ -26,8 +26,7 @@ ) from ..hashindex import ChunkIndex, ChunkIndexEntry from ..crypto.key import AESOCBKey -from ..helpers import bin_to_hex, safe_ns -from ..helpers import IntegrityError +from ..helpers import IntegrityError, bin_to_hex, safe_ns from ..helpers.msgpack import int_to_timestamp from ..manifest import Manifest from ..repository import Repository diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 6bd0bf022f..df835fa684 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -1,6 +1,7 @@ import io import logging import os +import struct import sys import time from collections import namedtuple @@ -17,7 +18,7 @@ from ..archive import resync_validator from ..compress import CNONE from ..constants import ROBJ_FILE_STREAM -from ..crypto.key import CHPOKey, ChecksumKey +from ..crypto.key import AESOCBKey, AuthenticatedKey, CHPOKey, ChecksumKey from ..repository import Repository, MAX_DATA_SIZE, propagate_rsh, rest_serve_command, PackWriter, PackReader from ..repository import PackTracker from ..repoobj import RepoObj, OBJ_MAGIC, OBJ_VERSION @@ -1894,7 +1895,7 @@ def test_pack_reader_resync_skips_to_next_object(): assert list(reader.iter_headers(validate=accept_all)) == [(H(2), len(obj1), len(obj2))] -def test_pack_reader_resync_recovers_from_corrupted_size(): +def test_pack_reader_resync_recovers_from_size_past_the_pack_end(): # a header whose sizes point past the pack, so the next object is found by scanning. obj1 = bytearray(fchunk(b"payload-one", meta=b"meta1", chunk_id=H(1))) obj2 = fchunk(b"payload-two", meta=b"meta2", chunk_id=H(2)) @@ -1910,6 +1911,120 @@ def test_pack_reader_resync_recovers_from_corrupted_size(): ] +def none_repo_objs(): + # a RepoObj with a "none-*" key and no compression: it formats real objects (tagged metadata + # slot, csize) and stores their payload as it is. + repo_objs = RepoObj(ChecksumKey(None)) + repo_objs.compressor = CNONE() + return repo_objs + + +def real_chunk(repo_objs, data): + # (chunk_id, obj) of a real repo object storing data. + chunk_id = repo_objs.id_hash(data) + return chunk_id, repo_objs.format(chunk_id, {}, data, ro_type=ROBJ_FILE_STREAM) + + +@pytest.mark.parametrize("shape", ["into_obj3", "onto_obj3_header", "into_itself"]) +def test_pack_reader_resync_rejects_a_header_with_a_wrong_data_size(shape): + # data_size is the one header field no tag covers. A wrong one that keeps the object inside the + # pack leaves the header parseable, so the walk checks it against csize from the tagged metadata + # and drops obj1. The shapes are where the wrong size points: into obj3, exactly onto obj3's + # header, and back into obj1 itself. + repo_objs = none_repo_objs() + _, obj1 = real_chunk(repo_objs, b"A" * 100) + id2, obj2 = real_chunk(repo_objs, b"B" * 100) + id3, obj3 = real_chunk(repo_objs, b"C" * 100) + true_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj1[: RepoObj.obj_header.size])).data_size + bad_size = { + "into_obj3": true_size + len(obj2) + 60, + "onto_obj3_header": true_size + len(obj2), + "into_itself": true_size - 60, + }[shape] + obj1 = bytearray(obj1) + obj1[45:49] = struct.pack(" Date: Sun, 23 Aug 2026 18:34:46 +0530 Subject: [PATCH 4/5] check --repair: report a failed object validation apart from a corrupt header, #8476 --- src/borg/archive.py | 12 ++++----- src/borg/cache.py | 4 +-- src/borg/repository.py | 27 ++++++++++--------- src/borg/testsuite/archiver/check_cmd_test.py | 16 ++++++++--- src/borg/testsuite/repository_test.py | 22 +++++++-------- 5 files changed, 47 insertions(+), 34 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index be5fc003e4..cbdec506bc 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2081,7 +2081,7 @@ def __next__(self): return next(self._unpacker) -def resync_validator(repo_objs): +def object_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 @@ -2100,11 +2100,11 @@ def resync_validator(repo_objs): def validate(chunk_id, obj): try: meta = repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) + data_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj[:hdr_size])).data_size + return data_size == meta["csize"] + overhead except Exception: - # arbitrary bytes fail the tag, the msgpack unpacking or the length checks. + # arbitrary bytes fail the tag, the msgpack unpacking, the length checks or the csize lookup. return False - data_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj[:hdr_size])).data_size - return data_size == meta["csize"] + overhead return validate @@ -2171,7 +2171,7 @@ def check( 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 + validate = object_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 ) @@ -2710,7 +2710,7 @@ def finish(self): build_chunkindex_from_repo( self.repository, slow_rebuild=True, - validate=resync_validator(self.repo_objs), + validate=object_validator(self.repo_objs), write_immediately=True, ) else: diff --git a/src/borg/cache.py b/src/borg/cache.py index b1fbfe674f..fa50acd1f3 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -867,8 +867,8 @@ def build_chunkindex_from_repo( ): # 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 check - # every object header it walks and resync past the ones that fail rather than raise IntegrityError. + # validate: a repo object validator, handed to PackReader.iter_headers so the rebuild skips the + # objects that fail it. 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: diff --git a/src/borg/repository.py b/src/borg/repository.py index e02e6d8676..028672f3b8 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -40,9 +40,9 @@ # how much of a pack PackReader reads at once when searching for the next object header. RESYNC_WINDOW_SIZE = 1024 * 1024 -# how much PackReader reads per object when it validates the headers it walks: the header and, -# for the usual metadata slot size, the slot as well, so validating needs no second read. -VALIDATE_READ_SIZE = 1024 +# how much to read to get an object's header plus, at the usual metadata slot sizes, its metadata +# slot in the same read. +META_READ_SIZE = 1024 def repo_lister(repository, *, limit=None): @@ -438,32 +438,35 @@ def iter_headers(self, validate=None): pack_hex = bin_to_hex(self.pack_id) if self.pack_id is not None else "" pack_size = self.size() hdr_size = RepoObj.obj_header.size - read_size = VALIDATE_READ_SIZE if validate is not None else hdr_size + read_size = META_READ_SIZE if validate is not None else hdr_size offset = 0 while True: buf = self.read(offset, read_size) if len(buf) < hdr_size: break # clean EOF, or trailing partial bytes hdr = self._parse_header(buf[:hdr_size], offset, pack_size) - if hdr is not None and validate is not None: + if hdr is None: + problem = "invalid object header" + elif validate is not None: size = hdr_size + hdr.meta_size # the bytes validate looks at obj = buf[:size] if size <= len(buf) else self.read(offset, size) - if not validate(hdr.chunk_id, obj): - hdr = None - if hdr is None: + problem = None if validate(hdr.chunk_id, obj) else "object does not authenticate" + else: + problem = None + if problem is not None: if validate is None: raise IntegrityError( - f'pack {pack_hex}: invalid object header at offset {offset} (pack corruption), run "borg check"' + f'pack {pack_hex}: {problem} at offset {offset} (pack corruption), run "borg check"' ) next_offset = self._find_header(offset + 1, pack_size, validate) if next_offset is None: logger.warning( - f"pack {pack_hex}: invalid object header at offset {offset} and none after it, " + f"pack {pack_hex}: {problem} at offset {offset} and no object after it, " f"skipping the remaining {pack_size - offset} bytes." ) break logger.warning( - f"pack {pack_hex}: invalid object header at offset {offset}, " + f"pack {pack_hex}: {problem} at offset {offset}, " f"continuing at the object at offset {next_offset}." ) offset = next_offset @@ -1389,7 +1392,7 @@ def get(self, id, read_data=True, raise_missing=True): # RepoObj layout supports separately encrypted metadata and data. # We return enough bytes so the client can decrypt the metadata. hdr_size = RepoObj.obj_header.size - extra_size = 1024 - hdr_size # load a bit more, 1024b, reduces round trips + extra_size = META_READ_SIZE - hdr_size load_size = hdr_size + extra_size # keep the read inside this object: a pack holds neighbouring objects, so don't pull # bytes past obj_size into the next one. (an overshoot would be harmless -- parse_meta diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 1b695094e9..0b5c45dfe1 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -2,6 +2,7 @@ from pathlib import Path import re import shutil +import struct from unittest.mock import patch import pytest @@ -742,12 +743,21 @@ def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request, dama pack_id, objs = next((p, sorted(o)) for p, o in by_pack.items() if len(o) > 2) damaged_offset, damaged_id = objs[1] next_offset, next_id = objs[2] - field_offset = {"magic": 0, "data_size": 45}[damaged_field] # magic 8, version 1, chunk_id 32, meta_size 4 key = "packs/" + bin_to_hex(pack_id) - repository.store_store(key, corrupt(repository.store_load(key), damaged_offset + field_offset)) + pack = repository.store_load(key) + if damaged_field == "magic": + pack = corrupt(pack, damaged_offset) + else: + hdr_size = RepoObj.obj_header.size + hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(pack[damaged_offset : damaged_offset + hdr_size])) + # a data_size that keeps the object inside the pack, so the header still parses. + pos = damaged_offset + 45 # magic 8, version 1, chunk_id 32, meta_size 4 + pack = pack[:pos] + struct.pack(" Date: Sun, 23 Aug 2026 19:16:36 +0530 Subject: [PATCH 5/5] check --repair: report which header check failed and cap the metadata read, #8476 --- docs/internals/packs.rst | 12 ++-- src/borg/repository.py | 66 +++++++++++-------- src/borg/testsuite/archiver/check_cmd_test.py | 2 +- src/borg/testsuite/repository_test.py | 28 ++++++-- 4 files changed, 71 insertions(+), 37 deletions(-) diff --git a/docs/internals/packs.rst b/docs/internals/packs.rst index 40ed95aaea..de2106e2c0 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -92,8 +92,9 @@ A reader locates the next blob by advancing:: next_blob_offset = current_blob_offset + REPOOBJ_HEADER_SIZE + meta_size + data_size ``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. +supported version, and sizes that keep the blob inside the pack and within +``MAX_DATA_SIZE``. A header that fails these checks means a corrupt pack, and +``IntegrityError`` is raised, naming which check it failed. The per-blob magic limits the blast radius of corrupted length fields. The repair walk (``iter_headers(validate=...)``, used when ``borg check --repair`` @@ -114,8 +115,11 @@ accepts a candidate only when it validates like any walked header. Validating needs the key, so a repair that cannot read the manifest walks without it. In the ``none-*`` modes the tag is an unkeyed checksum, so the walk accepts any -well-formed blob, including one a backed up file contains - but it only scans -into a payload after the blob owning it failed to validate. +well-formed blob, including one a backed up file contains. Such a blob carries +its own chunk id and reads back as itself, so indexing it is harmless. Bytes +crafted to pass the unkeyed checksum are not caught here - authenticating them +is what these modes give up. The scan reaches a payload only after the blob +owning it failed to validate. Bit flips in the data are caught when the blob is read, on that blob alone. diff --git a/src/borg/repository.py b/src/borg/repository.py index 028672f3b8..85328a577e 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -43,6 +43,9 @@ # how much to read to get an object's header plus, at the usual metadata slot sizes, its metadata # slot in the same read. META_READ_SIZE = 1024 +# the largest metadata slot a validating read fetches. a slot holds a few compression fields, +# packed and encrypted. +MAX_VALIDATED_META_SIZE = 64 * 1024 def repo_lister(repository, *, limit=None): @@ -375,18 +378,37 @@ def size(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. + """Return (ObjHeader, None) for a valid header at offset, (None, problem) otherwise. Valid means: OBJ_MAGIC, a supported version, and an object that fits into the pack and is - at most MAX_DATA_SIZE bytes, the limit put() enforces on a whole object. + at most MAX_DATA_SIZE bytes, the limit put() enforces on a whole object. problem names + which of these failed. """ 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 hdr.magic != OBJ_MAGIC: + return None, "no object header" + if hdr.version not in SUPPORTED_OBJ_VERSIONS: + return None, f"unsupported object version {hdr.version}" obj_size = RepoObj.obj_header.size + hdr.meta_size + hdr.data_size - if obj_size > MAX_DATA_SIZE or offset + obj_size > pack_size: - return None - return hdr + if offset + obj_size > pack_size: + return None, "object extends past end of file" + if obj_size > MAX_DATA_SIZE: + return None, f"object of {obj_size} bytes exceeds the maximum of {MAX_DATA_SIZE}" + return hdr, None + + def _validates(self, hdr, offset, buf, buf_offset, validate): + """Return whether validate accepts the object with header hdr at offset. + + buf holds the pack bytes from buf_offset on; the metadata slot is read separately when buf + does not reach its end. A slot over MAX_VALIDATED_META_SIZE fails without that read. + """ + if hdr.meta_size > MAX_VALIDATED_META_SIZE: + return False + size = RepoObj.obj_header.size + hdr.meta_size + start = offset - buf_offset + end = start + size + obj = buf[start:end] if end <= len(buf) else self.read(offset, size) + return validate(hdr.chunk_id, obj) def _find_header(self, offset, pack_size, validate): """Scan forward from offset for the next object validate accepts, return its offset or None. @@ -406,14 +428,9 @@ def _find_header(self, offset, pack_size, validate): 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: - 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 + hdr, _ = self._parse_header(buf[pos : pos + hdr_size], offset + pos, pack_size) + if hdr is not None and self._validates(hdr, offset + pos, buf, offset, validate): + 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) @@ -425,8 +442,8 @@ def iter_headers(self, validate=None): The walk reads one range per object (or a slice, for a pack in memory), plus one store metadata lookup for the pack size. - A header that _parse_header does not accept means a corrupt pack and raises IntegrityError. - A read shorter than a header ends the walk: that is the end of the pack. + A header that _parse_header does not accept means a corrupt pack: IntegrityError names what + is wrong with it. A read shorter than a header ends the walk: that is the end of the pack. validate(chunk_id, obj) tells whether obj - an object's header and metadata slot - is the repo object with id chunk_id. Given one, the walk validates every header, reading the @@ -438,21 +455,18 @@ def iter_headers(self, validate=None): pack_hex = bin_to_hex(self.pack_id) if self.pack_id is not None else "" pack_size = self.size() hdr_size = RepoObj.obj_header.size + # TODO: objects smaller than META_READ_SIZE make the validating walk read the pack several + # times over. Buffering a window, as _find_header scans with, would suit them; skipping a + # large object stays cheaper with a short read per header. read_size = META_READ_SIZE if validate is not None else hdr_size offset = 0 while True: buf = self.read(offset, read_size) if len(buf) < hdr_size: break # clean EOF, or trailing partial bytes - hdr = self._parse_header(buf[:hdr_size], offset, pack_size) - if hdr is None: - problem = "invalid object header" - elif validate is not None: - size = hdr_size + hdr.meta_size # the bytes validate looks at - obj = buf[:size] if size <= len(buf) else self.read(offset, size) - problem = None if validate(hdr.chunk_id, obj) else "object does not authenticate" - else: - problem = None + hdr, problem = self._parse_header(buf[:hdr_size], offset, pack_size) + if hdr is not None and validate is not None and not self._validates(hdr, offset, buf, offset, validate): + problem = "object does not authenticate" if problem is not None: if validate is None: raise IntegrityError( diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 0b5c45dfe1..a36c0695e6 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -756,7 +756,7 @@ def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request, dama repository.store_store(key, pack) output = cmd(archiver, "check", "--repair", "--debug", exit_code=0) - problem = {"magic": "invalid object header", "data_size": "object does not authenticate"}[damaged_field] + problem = {"magic": "no object header", "data_size": "object does not authenticate"}[damaged_field] assert f"{problem} at offset {damaged_offset}" in output assert f"continuing at the object at offset {next_offset}" in output # the rebuild resumed at the next object with Repository(archiver.repository_location, exclusive=True) as repository: diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index d10f3f7c02..bdb5b57f4b 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -20,7 +20,7 @@ from ..constants import ROBJ_FILE_STREAM from ..crypto.key import AESOCBKey, AuthenticatedKey, CHPOKey, ChecksumKey from ..repository import Repository, MAX_DATA_SIZE, propagate_rsh, rest_serve_command, PackWriter, PackReader -from ..repository import PackTracker +from ..repository import PackTracker, MAX_VALIDATED_META_SIZE from ..repoobj import RepoObj, OBJ_MAGIC, OBJ_VERSION from .hashindex_test import H @@ -1840,7 +1840,7 @@ def test_pack_reader_raises_on_bad_magic(): obj2 = bytearray(fchunk(b"d2", meta=b"m2", chunk_id=H(2))) obj2[0] ^= 0xFF # break the magic of the second object's header reader = PackReader(pack_contents=obj1 + bytes(obj2)) - with pytest.raises(IntegrityError): + with pytest.raises(IntegrityError, match="no object header at offset"): list(reader.iter_headers()) @@ -1851,7 +1851,7 @@ def test_pack_reader_raises_on_bad_magic_through_store(tmp_path): with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: repository.store_store("packs/" + bin_to_hex(pack_id), bytes(obj)) reader = PackReader(repository.store, pack_id) - with pytest.raises(IntegrityError): + with pytest.raises(IntegrityError, match="no object header at offset"): list(reader.iter_headers()) @@ -1860,7 +1860,7 @@ def test_pack_reader_raises_on_object_past_end_of_pack(): obj = fchunk(b"data", meta=b"meta", chunk_id=H(5)) pack = obj[:-1] # drop a byte, so the header's data_size no longer fits reader = PackReader(pack_contents=pack) - with pytest.raises(IntegrityError): + with pytest.raises(IntegrityError, match="object extends past end of file at offset"): list(reader.iter_headers()) @@ -1870,17 +1870,33 @@ def test_pack_reader_raises_on_object_past_end_of_pack_through_store(tmp_path): with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: repository.store_store("packs/" + bin_to_hex(pack_id), obj[:-1]) reader = PackReader(repository.store, pack_id) - with pytest.raises(IntegrityError): + with pytest.raises(IntegrityError, match="object extends past end of file at offset"): list(reader.iter_headers()) def test_pack_reader_raises_on_unsupported_version(): obj = bytearray(fchunk(b"data", chunk_id=H(7))) obj[len(OBJ_MAGIC)] = 0xEE # version byte - with pytest.raises(IntegrityError): + with pytest.raises(IntegrityError, match="unsupported object version 238 at offset"): list(PackReader(pack_contents=bytes(obj)).iter_headers()) +def test_pack_reader_rejects_an_object_over_max_data_size(): + # a header claiming more than put() would ever write, in a pack large enough to hold it. + hdr = RepoObj.obj_header.pack(OBJ_MAGIC, OBJ_VERSION, H(8), 0, MAX_DATA_SIZE) + parsed, problem = PackReader._parse_header(hdr, 0, 2 * MAX_DATA_SIZE) + assert parsed is None + assert "exceeds the maximum" in problem + + +def test_pack_reader_does_not_fetch_an_oversized_metadata_slot(): + # an oversized meta_size fails validation on the header alone, with no read of the slot. + hdr = RepoObj.ObjHeader(OBJ_MAGIC, OBJ_VERSION, H(9), MAX_VALIDATED_META_SIZE + 1, 0) + reader = PackReader(pack_contents=b"") + reader.read = lambda offset, size: pytest.fail("the slot was fetched") + assert not reader._validates(hdr, 0, b"", 0, lambda chunk_id, obj: pytest.fail("validate was called")) + + def accept_all(chunk_id, obj): # validate stand-in: accepts every candidate. return True