fix(fileformat): make header readers consistent + accept chunked reads + fuzz#1006
Merged
Merged
Conversation
Fuzzing the two header parsers against each other surfaced two real
robustness bugs:
1. Header.Read used a single r.Read for the 8-byte magic. io.Reader.Read
is permitted to return fewer than 8 bytes with a nil error (sockets,
TLS, io.Pipe, bufio over a slow source), so a valid header arriving in
chunks was rejected with "expected to read 8 bytes". Use io.ReadFull.
2. ReadHeaderFromBytes did not apply the trailing-0x00 -> 0x20 backward-
compatibility normalization that Header.Read does, so an older NUL-
padded magic was accepted via the streaming path but rejected as
"unknown magic" via the byte-slice path — the same reader/writer
disagreement class that has crashed the core sidecar before.
Extract the normalization into a shared normalizeMagic helper and apply
it in both readers. Existing error-message assertions are preserved.
Tests:
- TestHeaderRead_AcceptsChunkedReader (bug 1): every valid magic parses
when delivered one byte at a time via iotest.OneByteReader.
- TestReadHeaderFromBytes_BackwardCompatibility (bug 2): a NUL-padded
magic is accepted with the correct file type.
- FuzzHeaderParsersAgree: differential fuzz asserting ReadHeaderFromBytes
and Header.Read (over a one-byte-at-a-time reader) agree on validity
and file type. ~1.8M execs, no failures.
- FuzzReadHeaderFromBytes: no-panic, ~1.3M execs.
Third in the deserializer-fuzzing series (after the utxopersister and
block-parse allocation fixes).
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: Analysis Summary:
|
|
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-06-01 15:12 UTC |
ordishs
approved these changes
Jun 1, 2026
ordishs
left a comment
Collaborator
There was a problem hiding this comment.
Approve. Tight, well-scoped fix — both robustness bugs are real and the regression tests genuinely fail before / pass after.
Verified locally:
go test -race ./pkg/fileformat/— passgo vet— cleanFuzzHeaderParsersAgree— ~3.5M execs, no crashers
Correctness:
io.ReadFullis the right primitive (ErrUnexpectedEOF on partial, EOF on zero — both hit the error path).normalizeMagicis a faithful extraction; closes the reader/writer drift betweenHeader.ReadandReadHeaderFromByteswithout widening accepted input (normalized magic must still be in the map).- Existing error-message assertions still hold.
Minor non-blocking follow-up: TestHeader_Read_UnknownMagic passes a 7-byte input, so it actually exercises the short-read path rather than the unknown-magic path its name implies (pre-existing). Worth a rename or padding to 8 bytes later.
sugh01
approved these changes
Jun 2, 2026
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
3 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.
Third in the deserializer-fuzzing series (after #1004 and #1005). I expected the fileformat header to be low-yield "insurance" (it already has a length guard) — but a differential fuzz of its two parsers surfaced two real robustness bugs.
Bugs
1.
Header.Readused a singler.Readfor the 8-byte magic.io.Reader.Readis allowed to return fewer than 8 bytes with anilerror — sockets, TLS conns,io.Pipe, andbufioover a slow source all do this. The code then hitn != 8and rejected a perfectly valid header that simply arrived in chunks ("expected to read 8 bytes, got 1"). A file orbytes.Readerfills 8 in one call so it was invisible there, but streaming readers fail. → useio.ReadFull.2.
ReadHeaderFromByteslacked the trailing-0x00→0x20normalization thatHeader.Readhas. Older files padded the magic with NULs instead of spaces.Header.Readnormalizes them (see the existingTestHeader_Read_BackwardCompatibility);ReadHeaderFromBytesdid not — so the same file was accepted via the streaming path and rejected as"unknown magic"via the byte-slice path. That reader/writer disagreement is the same class that has crashed thecoresidecar before.Fix
Extract the normalization into a shared
normalizeMagichelper and apply it in both readers; switchHeader.Readtoio.ReadFull. Existing error-message assertions are preserved.Tests
TestHeaderRead_AcceptsChunkedReader(bug 1): every valid magic parses when delivered one byte at a time viaiotest.OneByteReader.TestReadHeaderFromBytes_BackwardCompatibility(bug 2): a NUL-padded magic is accepted with the correct file type.FuzzHeaderParsersAgree: differential fuzz assertingReadHeaderFromBytesandHeader.Read(over a one-byte-at-a-time reader) agree on validity and file type — catches both normalization drift and short-read mishandling.FuzzReadHeaderFromBytes: no-panic.Verification
got 1,unknown magic: [66 45 49 46 48 0 0 0]), pass after.FuzzHeaderParsersAgree~1.8M execs,FuzzReadHeaderFromBytes~1.3M execs — no crashers.pkg/fileformat-race,go vet,gofmt,gci— clean. Existing tests unchanged.Completes the planned three-target series. Net: 3 PRs, 4 distinct bugs (two unbounded-allocation/OOM, two header-parser inconsistencies), each pinned by a fuzz target + regression test.