index rebuild: abort cleanly on a corrupt object header, #10122 - #10168
Open
mr-raj12 wants to merge 5 commits into
Open
index rebuild: abort cleanly on a corrupt object header, #10122#10168mr-raj12 wants to merge 5 commits into
mr-raj12 wants to merge 5 commits into
Conversation
…e chunks index, borgbackup#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.
…one, borgbackup#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.
…up#8476 A meta_size or data_size corrupted to a value that keeps the object inside the pack leaves the header valid, so the walk only notices at the misaligned offset it jumps to; scanning from there loses the intact objects in between. An object is dropped when a recovered object starts inside it, its size field being wrong.
…rgbackup#8476 ArchiveChecker builds its own chunks index with a validate callable, but repository.get() resolves chunk locations through repository.chunks, a separate index that is built lazily on first access. That rebuild had no validator, so a repair run aborted at a corrupt object header it had already resynced past when building the checker index. Add Repository.chunkindex_validate, passed on to the lazy rebuild, and set it in ArchiveChecker.check() when repairing.
…0122 Rebuilding the chunks index from the packs walks every object header. Without a validate callable, iter_headers raises IntegrityError at a corrupt header, which reached the user as a traceback with rc 90. Catch it in build_chunkindex_from_repo and raise CorruptPack (rc 93, no traceback) instead, telling the user to run "borg check --repair". Continuing past the header is not an option there: the resulting index would be incomplete. Only check --repair passes a validator and resyncs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10168 +/- ##
==========================================
+ Coverage 86.55% 86.60% +0.05%
==========================================
Files 101 101
Lines 17996 18071 +75
Branches 2738 2753 +15
==========================================
+ Hits 15577 15651 +74
- Misses 1717 1718 +1
Partials 702 702 ☔ View full report in Codecov by Harness. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #10122. Stacked on #10094, and the first commit here belongs to that one, so review/merge #10094 first.
When the
index/fragments are gone or unreadable, borg rebuilds the chunks index by walking the object headers of every pack. Hit a corrupt header anditer_headers()raisesIntegrityError, which nobody catches, so a plainborg repo-listdies with a traceback and rc 90.build_chunkindex_from_repo()now catches it and raisesCorruptPack, a newErrorsubclass with rc 93 and no traceback:It aborts instead of skipping the damaged object, because everything after that offset would be missing from the index without the user ever knowing.
borg check --repairis the one caller that passes a validate callable, and it still resyncs past the header as before.CorruptPackis not anIntegrityErrorsubclass. That would print a traceback, and it would get swallowed by theexcept IntegrityErrorhandlers incheck_cmd.pyandcompact_cmd.pythat handle single corrupt objects. The, run "borg check"tail is gone from theiter_headers()message now that the wrapper says something more useful.The other commit
Testing this turned up a gap in #10094.
ArchiveCheckerbuilds its own chunks index with a validator, butrepository.get()resolves chunk locations throughrepository.chunks, a separate index built lazily on first access with no validator at all. Soborg check --repairaborted at a header it had already resynced past. OnceCorruptPackwas in, that turned into the command telling the user to run the command they were running.Repository.chunkindex_validatefixes it: the lazy rebuild hands it toiter_headers(),ArchiveChecker.check()sets it when repairing. Handing over the checker's index object instead does not work, it stores plaintext sizes andPackWriter.add()assertssize == 0.Verification
Real repository, one header byte flipped at a known object offset,
index/deleted:borg repo-list: the message above, rc 93, no tracebackBORG_EXIT_CODES=legacy: rc 2borg compact: same message, rc 93borg check --repair: resyncs, reports the one lost chunk, rc 0test_build_chunkindex_repair_resyncs_after_corrupt_header(from #10094) now expectsCorruptPackat the API level.test_repo_list_aborts_cleanly_on_corrupt_packcovers the exit code and the missing traceback through the real CLI, forked so the top level error handler actually runs.One thing the test does not claim:
borg check --repairdoes not rewrite the damaged pack yet, so a later index-less rebuild walks into the same header again. That is a TODO in the test rather than an assertion that the repository is fine afterwards.Full suite 2528 passed, 333 skipped (
-k "not remote and not binary"). ruff and black clean. Exit code 93 added to the table infrontends.rst, plus a note in theiter_headers()section ofpacks.rst.