Repository navigation
reveal: keep the original stego file when nothing was extracted - #36
Merged
Merged
Conversation
reveal_file() removed the input whenever an extraction method reported success, without checking that anything had reached the disk. Several shapes make a method return True while writing zero files: an archive whose members are all directory entries (zipping an empty folder produces one), an empty archive, or member names that sanitize_path() rejects. The archive inside the stego file is then the only copy left, and it was deleted. The decision now rests on a before/after snapshot of the output directory (relative path -> size and mtime in ns), not on the methods' return values. The input file is excluded from the snapshot, as are the 7z_ / 7z_unzip_ temp directories _extract_with_7zip creates, whose contents are intermediates rather than results -- if its cleanup ever fails, counting them would reintroduce the deletion this guard exists to prevent. A per-call counter incremented at the four places that write extracted files was tried first and was wrong, which is worth recording: 7-Zip is invoked with -o<output_dir> when auto-rename is off, so a subprocess writes the results and no in-process counter sees them. That version passed the "nothing extracted" case but broke the default for a normal reveal -- the original survived a successful extraction. The snapshot covers subprocess writers as well, which is why it is the mechanism. When a method reports success and nothing appeared, the original is kept and the reason is logged, rather than falling through to the generic "all methods failed" list, which would be misleading -- a method did succeed. Verified with scripts/test-reveal-keeps-original.py, covering both halves so that a guard which simply stopped deleting anything could not pass: nothing extracted keeps the original, a normal reveal still deletes it (delete_original_after_reveal defaults to True), and --keep-original still keeps it. Mutation-tested by replacing the guard condition with "if False": the first case then fails on the deleted stego file, and removing the mutation restores a byte-identical source. Not changed, deliberately: without --auto-rename, extracting into a directory that already holds a same-named file overwrites it silently (_resolve_conflict_path returns the same path and the writer opens it with 'wb'). That is a separate behaviour with its own decision to make, and it is what made this defect look like a "nothing was extracted" case when it was first hit.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
The defect
reveal_file()removed the input whenever an extraction method reported success, without checking that anything had reached the disk. Several shapes make a method returnTruewhile writing zero files:sanitize_path()rejects.The archive inside the stego file is then the only copy left — and it was deleted.
The fix
The decision now rests on a before/after snapshot of the output directory (relative path → size and mtime in ns), not on the methods' return values. The input file is excluded from the snapshot, as are the
7z_/7z_unzip_temp directories_extract_with_7zipcreates — those hold intermediates, not results, and counting them would reintroduce the deletion this guard exists to prevent.When a method reports success and nothing appeared, the original is kept and the reason is logged, instead of falling through to the generic "all methods failed" list, which would be misleading — a method did succeed.
A wrong first attempt, worth recording
A per-call counter incremented at the four places that write extracted files was tried first, and it was wrong. 7-Zip is invoked with
-o<output_dir>when auto-rename is off, so a subprocess writes the results and no in-process counter sees them. That version passed the "nothing extracted" case but broke the default for a normal reveal — the original survived a successful extraction. The snapshot covers subprocess writers too, which is why it is the mechanism.Verification
New
scripts/test-reveal-keeps-original.py, run in CI. It covers both halves, so a guard that simply stopped deleting anything could not pass:--keep-originalMutation-tested by replacing the guard condition with
if False: the first case then fails on the deleted stego file, and reverting the mutation restores a byte-identical source.Adjacent behaviour, deliberately not changed
Without
--auto-rename, extracting into a directory that already holds a same-named file overwrites it silently (_resolve_conflict_pathreturns the same path and the writer opens it with'wb'). That is a separate behaviour with its own decision to make. It is also what made this defect look like a "nothing was extracted" case when it was first hit.