Review of the safe writer, the editors and tag writing, done by trying to break them. Scope: SafeWriter and the editors (src/OneMedia/Writing/, docs/writer.md), tag writing in every system
(src/OneMedia/Tags/Writing/, docs/tag-writing.md), MediaFile.Save/SaveAs/SaveInPlace and the onemedia tags command. Muxers, remux and trim are out, except where they share the writer.
Verdict
Editing can be advertised for Save / SaveAs, with the limits below. SaveInPlace can be advertised as "padding-only, not atomic".
After the fixes of the follow-up round (findings 7 in part, 9 in part, 10, 11, 14, 16, 17; see the table), the verdict stands and the two limits it named are narrower: huge padding no longer costs memory or writes, and the stray temporary files of a killed Save are removed by the next save.
| Claim | Verdict |
|---|---|
| Untouched bytes stay untouched; payload copied unchanged | Holds. 445 untouched round trips byte-identical; payload hash unchanged in every tag-write sweep (0 failures over the 8 families swept); no packet difference against ffprobe beyond explained classes. |
Save is atomic | Holds with listed limits. 500 kill -9 rounds, no half-written target. Replace semantics (finding 7), and stray temp files remain after a kill (see numbers). |
SaveInPlace only within padding, else PaddingRequired with the file untouched | Holds for the refusal and fit rules. Not atomic: a kill can tear it (finding 9, measured: 1 torn of 300), and it rewrites the whole padding element (finding 14). |
| Adversarial input gives typed refusals | Holds with listed limits. 117 typed refusals in the review run; four fuzz targets clean for 600 s after fixing findings 3 and 13. Huge ID3 padding costs memory (finding 14). |
| Reports tell the truth | Holds with one limit. System lines match ExifTool/MediaInfo on 15 fixtures; the common Date write reports "written" while other date tags that other tools prefer stay (finding 16). |
Before the fixes, claims 2 (symlinks) and 1 (RF64) were false (findings 1, 2). Both are fixed and tested.
Findings
Severity: data loss (a user's data or file layout is lost), corrupt output (the written file is wrong or unreadable), wrong report (the report says something that is not so), cosmetic.
"Fixed" findings have a regression test in tests/OneMedia.Tests/Writing/WriterReviewTests.cs (and, for fuzz repros, a file under testdata/fuzz/). Open items say why they stay open.
| # | Severity | Finding | State |
|---|---|---|---|
| 1 | data loss | Save on a path that is a symbolic link replaced the link with a regular file and left the file it pointed to unedited: the edit appeared to succeed, the link was gone, the real file unchanged. | Fixed: Save follows the link and replaces the target file; the link stays a link. Test SaveThroughASymbolicLinkEditsTheTargetAndKeepsTheLink. |
| 2 | corrupt output | RF64/BW64 files: the real RIFF size in ds64 was not updated by any edit that changed the file's size (the editor treated the 0xFFFFFFFF size field as an open-ended size and skipped the adjustment). A file that grew by 50 bytes kept a riffSize that was 50 short (0 in the two fixtures, which stated 0 to begin with). | Fixed in ChunkEditor.Build. Test Rf64KeepsItsSizeFieldsAndPatchesDs64. |
| 3 | corrupt output (refusal expected) | Matroska: a Segment (or other master) stating a size within a few bytes of the EBML maximum made the editor throw ArgumentOutOfRangeException when it re-emitted the header (found by the writer-ebml fuzz target, 68-byte repro). | Fixed: typed UnsupportedMediaException with the offset. Repro testdata/fuzz/writer-ebml/ebml-size-overflow.bin, test AnEbmlSegmentThatStatesAnAlmostMaximalSizeIsRefusedNotCrashed. |
| 4 | cosmetic | A tag edit that asked for nothing (TagEdits empty) still rewrote ID3v2 tags: a v2.2 tag was upgraded to v2.4, an extended header dropped, a footer turned into padding. MediaFile.Save() with nothing staged did the same. | Fixed: an empty edit plans one copy of the file. Test AnEmptyEditChangesNothing (20 fixtures). |
| 5 | cosmetic | onemedia tags f --set … --out f (output = input) ended in an unhandled ArgumentException (exit 134, core dump). | Fixed: reported as an error, exit 1. |
| 6 | cosmetic | SaveAs without overwrite checked for an existing file and then replaced it with File.Move(overwrite: true): a file created in between would have been overwritten. | Fixed: the move only overwrites when asked. Test SaveAsWithoutOverwriteNeverReplacesAnExistingFile. |
| 7 | open, by design (documented); stray temp files fixed | Save creates a new file: new inode, new modification time, the owner and group of the saving user; extended attributes, ACLs, SELinux labels and hard links of the original are not carried over. A read-only file in a writable folder is replaced. The folder is not fsynced after the rename. A killed Save left its temporary file behind (up to 63 in the folder after 200 kills). | Documented in docs/writer.md (Safe writer). The BCL has no portable way to copy ownership or xattrs, and none to fsync a directory (opening a directory for flushing is refused on Linux and macOS); calls into the OS are not allowed in the shipped packages. Temp files fixed: they are named .name.onemedia-<host>-<pid>-<random>.tmp and the next Save/SaveAs of the same target removes those whose process is gone (same host) or that are a day old; tests ASaveRemovesTemporaryFilesOfAKilledSaveOfTheSameTarget, ATemporaryFileOfThisProcessIsNotRemoved. |
| 8 | open (low) | A plan is checked against the file by length only. If another program changes the file between planning and writing and leaves its length alone, offsets in the plan point into different structure. Needs a concurrent writer and an equal-length change. | Documented. A fix needs the plan to carry the modification time or a head/tail hash. |
| 9 | open (low), window narrowed | SaveInPlace can tear on a kill or power loss for ranges that take more than one write. Documented since 0.3 (not atomic); measured below. | Documented; Save is the atomic path. The window is narrower: a first pass finds the runs of bytes that differ and a second writes only those, back to back (finding 14). Measured below. |
| 10 | cosmetic | A Matroska file with zero bytes after its Segment is refused with "damage in front of its first Cluster", a message that points at the wrong place. | Fixed: the refusal says how many bytes follow the Segment and from which offset (still a refusal: the file is not edited). Test TrailingZerosAfterTheSegmentAreNamedAsSuch. |
| 11 | cosmetic | PaddingRequiredException.Required is 1 for a shrinking edit that leaves a gap smaller than the smallest padding header (a free box needs 8 bytes; a FLAC padding block 4). The edit can be written with Save. | Fixed: Required is the missing part of a header (5 for a 3-byte gap in MP4; 2 minus the gap for Matroska; 8 or 24 minus the gap for RIFF/Wave64; 4 minus the gap for FLAC). Test ARefusedShrinkWithATinyGapReportsHowMuchIsMissing. |
| 12 | cosmetic | Applying the same tag edit a second time is not always a no-op: the first re-application may normalise the file (WAV id3 chunk, QuickTime key table, AIFF); the third application is a fixed point on every fixture. | Documented in docs/tag-writing.md. |
| 13 | not a writer defect | The tagwrite fuzz target found an MP3 of one frame plus junk plus an APE tag that opened before an edit and not after (No audio frame was found). The frame is confirmed only by what follows it; the original opens through the accident of its tail layout (cut at the end of the junk it does not open). The writer left the audio bytes alone. | Repro testdata/review/mp3-single-frame-before-ape.bin, skipped test explains. Real files hold many frames. |
| 14 | cosmetic/perf (large) | An ID3v2 edit builds the whole tag including padding in memory and rewrites all the padding in place: with 200 MiB of padding (ID3 allows 256 MiB) a 3-byte title change allocates 419.5 MB and writes 209.7 MB. In-place edits of MP4 free boxes also write the whole free box (a 64 MiB box: 67,108,872 bytes written for a cover change), which widens the window for a tear. Breaks the constant-memory claim for pathological padding only. | Fixed: padding is a header piece plus a generated run of zeros in every writer (ID3v2, MP4 free/skip, Matroska Void, RIFF JUNK, FLAC PADDING, ASF padding object), and SaveInPlace writes only the bytes that differ. Numbers below. One limit remains: the ID3v2 tag inside a WAVE/AIFF id3 chunk is still built whole. Tests AHugeId3PaddingCostsNoMemoryAndAnInPlaceEditWritesOnlyTheChange, ACoverChangedInPlaceIntoALargeFreeBoxWritesTheCoverAndNotTheBox, AnInPlaceSaveWritesOnlyTheRunsThatDiffer; PaddingNumbers (ONEMEDIA_WRITERFIX_BIG=1) prints the figures. |
| 15 | cosmetic (known) | Matroska Tags written after the Clusters without a SeekHead entry are correct on disk but our reader does not see them (sweep: no-meta-seek.mkv readback). An ASF embedded-ID3 attribute keeps its Genre after removal (wma_with_ID3_APIC.wma). | Open; the first is in the backlog (Matroska reader and new Tags get no SeekHead entry). |
| 16 | wrong report (mild), design question | A common Date write reports "written" for the field it set (QuickTime year, Matroska DATE_RELEASED), but other date tags stay and other tools prefer them: MediaInfo's Recorded date in tags_keys.mov stays 2023-04-05, MKV DATE_RECORDED stays. The report does not warn. | Fixed as a report note: TagWriteReport.Warnings names each other date tag left in place (with its value) and the movie header's creation time; onemedia tags prints warning: lines. Which fields Date sets is unchanged; choosing them (and the time zone) belongs to the evidence-grade date work. Tests ADateWriteNamesTheDateTagsItLeavesInPlace, AMatroskaDateWriteNamesTheOtherDateTags, AnId3DateWriteNamesTheOriginalReleaseDate. |
| 17 | design note | After a growing edit MKV, MP4, WAV, AIFF and Ogg files keep no padding, so a later in-place edit fits only an equal-size or shrinking-by-8+ value; only ASF (~800 bytes) and ID3 (~1 KiB) leave padding. | Fixed for MP4/MOV (free), Matroska (Void), WAVE/RF64/AIFF (JUNK) and Opus (zero padding in the comment header); FLAC, ID3 and ASF already did. 1024 bytes by default (TagWriteOptions.PaddingBytes; 0 or TagWriteOptions.Minimal for none). Ogg Vorbis and Ogg FLAC headers cannot carry padding and are unchanged. Test AGrowingEditLeavesPaddingSoALaterEditFitsInPlace (9 fixtures) and MinimalOutputLeavesNoPaddingAndTheNextGrowingEditIsRefused. |
Numbers
Reproduce: tools/run-writer-review.sh (build, env, fulldisk, kill, inplacekill, report) and tools/run-writer-review-tests.sh (tests and in/out pairs against ffprobe, MP4Box, mkvalidator, ExifTool). ONEMEDIA_REVIEW_BIG=1 enables the 4 GiB and 200 MiB-padding tests. Oracles: ffprobe 7.0.2, MP4Box 26.07, mkvalidator 0.6.0, ExifTool 13.60, MediaInfo 26.10; a local run with ffprobe 8.0.1 gave the same classes.
1. Untouched. Round trip (plain and header-rewriting) 445 files byte-identical (119 ISO, 91 MKV, 232 RIFF; 3 non-chunk WAVs skipped). Edit sweep: 365 OK, 12 typed refusals (unbounded last chunk or box), 1 damaged input with an ffprobe difference. Tag-write sweeps: ISO 119, MKV 91, MP3 95, FLAC 30, Ogg 37, WAV 87, AIFF 61, ASF 54 files; payload hash failures 0, copy failures 0, ffprobe/validator results worse than before 0. ffprobe packet differences: MP3 8 (ffprobe ends the last packet at APE/Lyrics3 tails), none elsewhere. Readback failures: ASF 1, MKV 1 (finding 15). Empty edit is identity on 20 fixtures. Crafted ISO layouts (moov-last, free 8/9/200 bytes at the end, zeros or junk at the end, 64-bit mdat/moov headers, vendor boxes): 8 bases x 14 variants x 3 titles, all valid or typed, vendor boxes preserved. Matroska unknown-size Segment, Void and vendor elements ok. RIFF odd, JUNK, trailing and vendor chunks ok. FLAC without PADDING grows. Ogg comment of 200k characters over several pages: CRCs and sequence numbers right. Fragmented MP4: tfra offsets and mfro right after growth. 4.3 GB sparse MP4: stco promoted to co64, all offsets right. Final test run: 195 tests, 0 failed, 1 skipped (finding 13); 117 typed refusals; 1845 in/out pairs: ffprobe errors worse 0, validator worse 0, ExifTool warnings worse 0, packets differing 116 (95 cover pairs with the attached-picture stream, 16 APE/Lyrics3 tails, 5 picture removals).
2. Atomic replace. kill -9 of onemedia tags --replace: MP4 560 MB 200 rounds: 183 original / 17 complete / 0 other; MKV 551 MB 150 rounds: 134 / 16 / 0; WAV 499 MB 150 rounds: 75 / 75 / 0. Temp files left behind after a kill: 63, 59 and 9 (in the target folder). Full disk (8 MiB tmpfs): Save and SaveAs raise IOException (no space), original unchanged, temp removed. Read-only folder: UnauthorizedAccessException, nothing left, original intact. Target held open: on Linux the rename succeeds and the holder keeps the old inode. Symlink: link kept, target edited (after the fix). Permissions: mode kept; owner and group become the saver's; hard links broken; xattrs lost; modification time updated; a 0444 file in a writable folder is replaced, while in-place refuses it. Saving over itself with SaveAs: ArgumentException (CLI: exit 1).
3. In place. Title length sweeps (success must equal SaveAs bytes, refusals leave the file unchanged): ID3v2.4 with padding fits 0..1040 of 1200; tags without padding fit 0..16; FLAC fits 0..8158 and 8162 (the 8159..8161 gap cannot hold a 4-byte block header); ASF unpadded only the exact size, after one growth 0..800 and 812; MP4 ilst 0..2 and 10; after one growth: MKV 0..298 and 300, WAV 0..293 and 300..301, AIFF 0..292 and 299..300, MP4 0..292 and 300, Ogg 300 only, MP3 0..1324. Kill -9 during --cover in place into a 64 MiB free after ilst of a 64 MiB moov-last MP4 (20 MB then 24 MB cover, 317 ms uninterrupted, kill at 45..105 % of that), 300 rounds: 2 original, 297 complete, 1 torn (round 62: the file is neither the old nor the new bytes and ffprobe reports 16 error lines). Correction: the harness of that run was broken: it started the tool from a shell function in the background, so the kill hit only the subshell and the tool ran to the end. The true figure for the original code is in section 6 (7 torn of 300). So an interrupted in-place write can tear and leave an unreadable file; docs/writer.md says in-place is not atomic and Save is the safe path. (Two earlier attempts of this check were invalid: the generated file had no padding after ilst, so every edit was refused.)
4. Fuzz, 600 s each, fresh work dirs. Original code: tagwrite crash at 1.94M executions (finding 13), writer-iso 16,126,148 clean, writer-ebml crash at 0.6M (finding 3), writer-riff 11,769,241 clean. After fixes: writer-iso 15,234,116 clean, writer-ebml 4,069,907 clean, writer-riff 18,458,015 clean, tagwrite 5,440,037 clean (601 s, after narrowing its reopen check to containers and streams with two or more confirmed frames).
5. Reports. 15 fixtures: every system line (Created flags, fields not written such as mdta track number and composer, AIFF title/artist/comment only, ID3v1 Latin-1 ? and year-only conversions) matches ExifTool and MediaInfo readback. The only mismatch class is finding 16.
6. Follow-up round (writer fixes), final commit. Worker, .NET 10 Release, ffprobe 7.0.2, MP4Box 26.07, mkvalidator 0.6.0, ExifTool 13.60, MediaInfo 26.10.
- Kill harness fixed.
tools/run-writer-review.shnow starts the tool with( exec dotnet ... ) &, so the kill reaches it. The original code, 300 rounds in-place cover change into a 64 MiBfree(file 67,265,317 bytes, kill at 45..105 % of an uninterrupted run): 107 original, 186 complete, 7 torn. A first rewrite that wrote differing bytes while still comparing (one pass) tore more often (18 of 300), soSaveInPlacenow finds the differing runs first and then writes only those, back to back: 121 original, 176 complete, 3 torn (all three unreadable by ffprobe; uninterrupted run 258 ms, 25,165,894 bytes written for the 24 MB cover against about 67 MB before). Still not atomic: three of 300 kills tore the file, soSavestays the safe path. - Finding 14. ID3v2 with 200 MiB padding, 3-byte title edit in place: allocated 138,504 bytes, written 3 bytes (before: 419.5 MB allocated, 209.7 MB written). MP4 cover 20 MB to 24 MB (patterned, not zeros) into a 64 MiB
free, in place: 24,000,033 bytes written (before: 67,108,872); 140 MB allocated, which scales with the cover (the cover is copied a few times when theilstis built), not with the padding; peak RSS of the whole test process 377 MB, including the generated test files. Peak RSS of a tag edit withSaveAson files of 4 GB and more: moov-last MP4 (4.38 GB) 50.8 MB, MKV (4.45 GB) 52.8 MB, RF64 WAV (4.42 GB) 46.3 MB. - Stray temporary files. After 150
Savekills on a 560 MB MP4: at most 1 temporary file at once and 1 at the end (original code: 136 at once). The nextSaveremoves the stale ones. - Regression checks. Untouched round trips 445 byte-identical (119 ISO, 91 MKV, 232 RIFF). Tag-write sweeps over 574 files in the 8 families: payload hash failures 0, copy failures 0, validator results worse than the input 0; ffprobe differences only the 8 MP3 APE/Lyrics3 tails; readback failures 2 (finding 15). The review pairs: 1858 pairs, ffprobe errors worse 0, validator worse 0, ExifTool warnings worse 0. The base code gives the same counters in every family. Whole suite on the final commit: 1,145 tests, 0 failed, 1 skipped (finding 13); NativeAOT smoke 136 ok, WebAssembly smoke 136 ok, packages pack.
- Fuzz, 300 s each.
writer-iso7,890,494 runs,writer-ebml2,695,327,tagwrite2,302,813, clean.writer-rifffound one new input at about 2.07 M runs, present in the original code too: an untouched RF64 file whose nestedBW64chunk states 0xFFFFFFFF did not come back byte-identical when headers were regenerated (the diagnosticRewriteHeaderspath; the default path copies headers). Fixed inChunkEditor(the marker is kept while the chunk's size is unchanged); regression testRegeneratingHeadersOfAnUntouchedRf64FileKeepsANestedAllOnesSize, repro intestdata/fuzz/writer-riff; the first fix kept the marker for a chunk whose ds64 size ran past the end of the file, and the fuzzer found a plan copying bytes that do not exist (a second input; now only kept inside the file, otherwise the typed refusal as before; testRegeneratingHeadersWithADs64SizePastTheEndOfTheFileIsATypedRefusalOrAValidPlan).writer-riffrerun on the final code: 11,251,826 runs in 301 s, 0 crashes. The suite and the other three targets ran on the commit before these two fixes, which touch onlyChunkEditor.