From 0ccf95b431a6ea9b51903fd7366eca7b096f738c Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Wed, 12 Aug 2026 12:52:27 +0530 Subject: [PATCH] validate object headers when walking a pack, see #8476 PackReader.iter_headers took the next offset from the sizes in each object header without ever checking that header. On corruption the walk either continues on payload bytes and yields garbage (chunk_id, offset, size) tuples, or skips past the end of the pack, where the short read looks like a clean EOF. The index build_chunkindex_from_repo rebuilds from that is wrong either way, without saying so. Check OBJ_MAGIC and that the object fits into the pack, raise IntegrityError naming the pack otherwise, like check_pack_objects does. The bounds check needs the pack size, which PackReader did not have, so add PackReader.size(): len(pack_contents) in memory, one store.info() per pack otherwise. No per-object roundtrip is added. --- src/borg/repository.py | 27 ++++++++++++++- src/borg/testsuite/repository_test.py | 49 +++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/src/borg/repository.py b/src/borg/repository.py index 120a0f59ec..ba99358fb1 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -361,12 +361,28 @@ def read(self, offset, size): return memoryview(self.pack_contents)[offset : 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.""" + if self.pack_contents is not None: + return len(self.pack_contents) + return self.store.info(self.key).size + def iter_headers(self): """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). + range read per object (or just a slice, when the pack is already in memory), plus one + store metadata lookup for the pack size. + + 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. """ + 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 offset = 0 while True: @@ -374,7 +390,16 @@ def iter_headers(self): 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"' + ) 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/repository_test.py b/src/borg/testsuite/repository_test.py index 34d39646fb..9e1abb1aac 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -1635,6 +1635,55 @@ def test_pack_reader_iter_headers_reads_through_store(tmp_path): assert list(reader.iter_headers()) == [(H(47), 0, len(obj1)), (H(48), len(obj1), len(obj2))] +def test_pack_reader_raises_on_bad_magic(): + # a header without OBJ_MAGIC means the walk desynced onto payload bytes: corruption, not EOF. + obj1 = fchunk(b"payload-one", meta=b"meta1", chunk_id=H(1)) + 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): + list(reader.iter_headers()) + + +def test_pack_reader_raises_on_bad_magic_through_store(tmp_path): + obj = bytearray(fchunk(b"FIRST", chunk_id=H(47))) + obj[0] ^= 0xFF + pack_id = H(44) + 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): + list(reader.iter_headers()) + + +def test_pack_reader_raises_on_object_past_end_of_pack(): + # a valid header whose declared sizes overrun the pack: the object cannot be there. + 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): + list(reader.iter_headers()) + + +def test_pack_reader_raises_on_object_past_end_of_pack_through_store(tmp_path): + obj = fchunk(b"FIRST", chunk_id=H(49)) + pack_id = H(45) + 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): + list(reader.iter_headers()) + + +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) + pack_id = H(46) + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + repository.store_store("packs/" + bin_to_hex(pack_id), obj) + assert PackReader(repository.store, pack_id).size() == len(obj) + + def test_pack_reader_in_memory_read_returns_view(): # read() over an in-memory pack returns a memoryview into pack_contents. obj1 = fchunk(b"payload-one", meta=b"meta1", chunk_id=H(1))