basicio: write through a temp file so the original survives an interrupted metadata write (#9482) - #9483
Closed
AetherAI3 wants to merge 1 commit into
Closed
Conversation
…xiv2#9482) FileIo::transfer's generic branch opened the target with "w+b" (which truncates) before the new bytes had been written. A crash, a source-open failure, or an out-of-space error between the truncate and write() left a zero-byte file where the original image was, with no backup. This mirrors the FileIo branch above: build the new content in a temporary file next to the target, then rename it into place — atomic on POSIX (fs::rename) and via ReplaceFileA on Windows. The original bytes are only unlinked at the moment the rename installs the new file. Original permissions are preserved the same way the FileIo branch already does. Added unit test AFileIO.transferGenericBranchPreservesOriginalOnSourceOpenFailure_9482 that constructs a MemIo whose open() fails, calls transfer, and asserts the target file still exists with its original bytes. It fails on main (file becomes 0 bytes) and passes with this change. Reported-by: yottayoshida (https://github.com/yottayoshida) Signed-off-by: Brandon Barrante <aetherai@aethersystems.net>
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
2 participants
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.
Fix for #9482.
The bug
FileIo::transferhas two branches. When the source is anotherFileIo(a temp file), it renames it into place. When the source isnot — which is the branch JPEG metadata writes reach through
MemIo— it opens the target with
"w+b"first, and"w+b"truncates. Ifanything between that truncate and the write completing goes wrong
(a crash, a failing
src.open(), out of space during the write), theoriginal image is left as a zero-byte file with no backup.
The issue's syscall trace shows the same on the current
main.The change
Route the generic branch through the same shape the
FileIobranchalready uses:
(
<path>.exv-tmp-<pid>[-<counter>], same directory sofs::renameis atomic on POSIX and
ReplaceFileAworks on Windows).tmp.open,src.open,write,close— remove the temp file and throw. The original isnever touched.
FileIobranch already does.
Test
AFileIO.transferGenericBranchPreservesOriginalOnSourceOpenFailure_9482in
unitTests/test_FileIo.cpp. It writes a fixture file, callstransferwith aMemIowhoseopen()returns-1, and asserts thefixture is still there with its original bytes. On
mainatdc9364bit fails with the fixture reduced to 0 bytes; with thischange it passes.
Also checked the happy path:
exiv2 rmon a real JPEG still stripsmetadata (118 685 → 115 163 bytes, no
.exv-tmpfiles left behind),and the full
AFileIO.*suite runs 11 / 11.Reporter
Thanks to @yottayoshida — the report has a syscall-level trace and
sets out the two possible responses; this PR is option 1.
AI assistance
I used an AI assistant to help draft the patch, the test and this
description. I read every line, checked the invariants against the
fixed code, and ran the tests myself before opening this.