Refuse an entry that would be written over the archive being read - #105
Merged
Conversation
An archive holding an entry named after the archive, extracted into the archive's own directory, overwrote itself. All three formats; two of the three said "Extracted 1 file(s)" and exited 0 while doing it. [zip] 132 -> 12 bytes [tar] 2048 -> 12 bytes [7z] 115 -> 12 bytes The entry is written onto the file still being read, so the archive is truncated mid-read and what replaces it is whatever fraction the extractor had reached: the contents are lost from the output as much as from disk. What made this indefensible rather than merely unfortunate is that compression has always refused the mirror image, and refuses it even with `--force` (`OutputIsSource`, and an output inside the folder being archived). The same product held two opposite positions on the same question. The planning pass already reads the whole listing before a byte is written, so the check costs one comparison per entry and no extra pass over the file. Two details it would be easy to get wrong, both pinned by tests: - it compares **file identity**, not paths. A hardlink is a second name for one file and never resolves to the same string, which is exactly how `--force` was once able to overwrite its own source on the compression side. Verified against a real hardlink. - it follows the **planned** name, not the archive's spelling, since a rename can land an entry on the archive that the archive's own name does not match. Refused outright rather than made overridable. Nobody agrees to this by asking to extract something, so no flag should unlock it. `WouldOverwriteArchive` is a variant of its own rather than a `Failed` string: the two callers that match on `CompressionError` both have catch-all arms, so nothing breaks, and a caller that wants to offer "extract somewhere else" now has something to match on. Four tests, and all four mutations are caught: removing the guard, comparing paths instead of identity, ignoring the plan, and refusing too much. The last one matters most, since "refuse anything that resembles the archive" would pass the other three and break ordinary extractions. Closes #96
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
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 #96, the half of #62 that destroys data without the user getting anything wrong.
The bug
An archive holding an entry named after the archive, extracted into the archive's own directory, overwrote itself. Real binary, on
dev:The entry is written onto the file still being read, so the archive is truncated mid-read and what replaces it is whatever fraction the extractor had reached. The contents are lost from the output as much as from disk, and two of the three formats report success.
Why it is not a judgement call
Compression has always refused the mirror image, and refuses it even with
--force:OutputIsSource, plus a separate refusal for an output inside the folder being archived, on the grounds that "agreeing to replace a file is not agreeing to destroy the source". Extraction had no equivalent, so the same product held two opposite positions on the same question.So this is refused outright rather than made overridable. Nobody asks for this outcome.
Two things easy to get wrong
File identity, not paths. A hardlink is a second name for one file and never resolves to the same string. That is not hypothetical: it is exactly how
--forceused to be able to overwrite its own source on the compression side beforepaths::same_filewas introduced. Verified against a real hardlink:The planned name, not the archive's spelling. A rename can land an entry on the archive that the archive's own name does not match: archive
v_.zip, entryv?.zip, answered with_.Cost
None worth measuring. The planning pass already reads the whole listing before a byte is written, so this is one identity comparison per entry and no extra pass over the file.
Tests
Four, and every mutation is caught:
The last row is the one that matters. "Refuse anything that resembles the archive" would pass the first three and quietly break ordinary extractions, so
the_same_archive_extracts_normally_somewhere_elseexists to stop exactly that fix.Notes
WouldOverwriteArchiveis a variant of its own rather than aFailedstring. Both places that match onCompressionErrorhave catch-all arms, so nothing breaks, and a front end that wants to offer "extract somewhere else" now has something to match on.608 Rust tests and 116 Vitest, 493 offline.
threat_model.mdgains section 4a.#62keeps what remains: the general policy for overwriting files that already exist, which is a design decision rather than a bug and probably wants relabelling.