Artifact 2a67594223b5b803522be8d22bfa0f9748aa19d07d46cb05a7b23f2f7e31a4f7:
- File
internal/xfer/message.go
— part of check-in
[c892c7a622]
at
2026-07-20 03:33:23
on branch trunk
— fix: never parse a compressed body as cards (#104) (#111)
* fix: never parse a compressed body as cards (#104)
A clone served by libfossil failed in libfossil's own client with
xfer: decode card 16303: xfer: empty line after split
on a response that contained 202 cards. The index, the card, and the fault were all fictional.
xfer.Decode tried three body framings in order -- raw zlib, the §4.1 4-byte-length container, then uncompressed card text -- and treated any decompression failure as "this is not that framing". decompressBounded failed a well-formed container purely because its decompressed length passed a 50 MB bound, so the fallback chain handed the still-compressed bytes to the card parser. The parser read compressed noise as cards for sixteen thousand iterations before one of them split into no fields, and reported that as the fault. Captured at offset 15,084,752 of the failing body, the "card" the parser was reading is DEFLATE output:
"...\xd9\x34\x50\xe7\xcf\xa0\xf8\xd5\xd5\x87l\xff\t\xde\xfb\xe3..."
Two changes. Once a body is recognized as a zlib stream, a failure to decode it is reported rather than reinterpreted -- oversize, truncated, or corrupt, the error now names the transport failure instead of an arbitrary card position. Both compressed framings are still attempted, since a length prefix can coincidentally parse as a zlib header, and uncompressed bodies still reach the card parser; only compressed ones no longer can.
The bound itself was also wrong, and is the reason this was reachable at all. §4.1 places no limit on the decompressed size of the container; the 50 MB was a local memory guard that sat below what this implementation's own server emits in one clone round, so two libfossil peers could not clone from each other even though each could clone from fossil. It is now 64 MiB and coupled to sync.DefaultCloneBatchBytes by a compile-time guard that fails the build, rather than the clone, if the budget is raised past half of it.
The defect predates the size-bounded clone batching of #100. #100 shrank rounds enough to hide it on the corpus that exposed it; it did not touch the decoder.
Regression coverage, in two layers. internal/xfer pins the decode behaviour directly: an oversize container, a truncated one, a large in-bounds one, and an uncompressed body. internal/sync adds the configuration no existing test covered -- libfossil serving and libfossil cloning across a real HTTP encode/decode, on a corpus of multi-megabyte incompressible artifacts that crosses the server's per-round budget. Every prior clone test was either served by fossil or wired through MockTransport, which passes *xfer.Message values across untouched and never encodes a wire body at all. Artifact size is the other half of the reproduction: this failed on wapp, whose artifacts are large, and not on pikchr, whose artifacts are small.
Verified against the wapp corpus: the round trip that failed now completes with 1038 of 1038 artifacts on both ends.
* fix: make the #104 fixture reach the bound, and pin the bound from both sides
Review found the flagship regression test passed against pre-fix code. It did. Its corpus was 21.6 MB spread over twelve artifacts, and pagination keeps every round near the 16 MB budget however large such a corpus grows, so the round never came near the 50 MiB bound it was supposed to cross. The 25-line rationale reasoned about crossing DefaultCloneBatchBytes, which is not the quantity that matters and never was; a permanent comment asserting a mechanism that does not exist is worse than no comment.
The quantity that matters is one artifact. A round is charged against the budget before each artifact and sends whole the artifact that crosses it, so a round carries budget plus one entire artifact of unbounded size. A single artifact larger than bound-minus-budget is the only corpus that can exceed the bound, and it is also the one path by which a round can still exceed the current bound -- so the fixture guards the live risk and reproduces the historical one with the same bytes. It is now one incompressible artifact of 7/8ths of MaxDecompressedBytes, sized against the constant so it tracks it, behind testing.Short.
Verified against a pre-fix tree with both test files dropped in:
TestCloneSelfRoundTripOversizeArtifact FAIL Clone: handleFile aad935c2...: UUID mismatch for received file: expected aad935c2..., got 501b4814...
which is the sharper end of this defect: pre-fix did not merely misreport, it accepted the compressed bytes as cards and produced an artifact whose content did not hash to its name.
The cheap end-to-end round trip is kept as a separate test and described honestly as a coverage plank -- the first test anywhere here to have libfossil decode a body libfossil produced -- rather than as a guard on anything.
Second finding, message.go: the claim that format 1 is the "Fossil HTTP sync wire format" is false. Every application/x-fossil body carries the §4.1 length prefix, request side and reply side, both through blob_compress. Format 1 matches nothing canonical emits and is kept as a tolerance, now labelled as one.
Its safety was also load-bearing on a constant with no stated relationship to it. For a container of decompressed length N, prefix byte 0 is N>>24, and a zlib CMF requires CM==8 in the low nibble, so no container under 128 MiB can present a valid header at offset 0. Raising MaxDecompressedBytes past that point would leave decompressContainer's prefer-what-decodes ordering as the only remaining defense -- a narrower margin than this code was written against, and #98 is exactly the change that might want a different bound. That is now a build failure rather than a trap, matching the guard already on the other side. Verified to fire at 0x08000000 and to permit 0x07FFFFFF.
Also: the MaxDecompressedBytes doc claimed to stop a hostile body from exhausting an embedding process's memory. It does not -- the transport reads the body whole before Decode sees it. It bounds expansion only, and now says so.
Test hygiene: the server handshake polls for a live listener instead of sleeping 200ms and dropping ServeHTTP's error, and the 32 MiB and 64 MiB xfer allocations skip under -short like their sync counterpart.
* fix: correct three claims about what the #104 guards cover
The fix is unchanged. Three statements about it were wrong, and a comment that misstates a mechanism is worse than no comment because the next reader trusts it.
The ordering claim was the serious one. decompressContainer was described as "preferring whichever framing actually decodes, so a spurious header match still loses to a real container". It does not. Format 1 returns immediately on success and format 2 is reached only when format 1 fails; the error comparison at the end selects which failure to report when both have already failed, and protects nothing when format 1 spuriously succeeds. Probed: a bare zlib body returns from format 1 with format 2 never attempted.
That makes the aliasing guard weaker than its comment claimed, and in a direction neither review had reached: the aliasing byte is the top byte of the length the PEER declared, and this decoder never reads that field -- it slices past it and counts bytes actually inflated. No value of MaxDecompressedBytes constrains what a peer declares. A container declaring 0x081D0000 while carrying 46 bytes presents a parsing zlib header at offset 0, measured.
What actually saves a real container in that case is the stream, not arithmetic: after two prefix bytes are eaten as a header, inflate must still parse the remaining two plus the real stream as DEFLATE and match its Adler-32. Measured `flate: corrupt input` at all three aliasing lengths probed, so format 1 fails, format 2 runs, and the container decodes correctly.
The guard is kept because it states something true and useful -- Encode writes uint32(len(raw)) as the prefix, so holding the bound below the threshold means no body libfossil emits can carry an aliasing prefix -- but it is now documented as an invariant on our output, not a defence of our input, with the airtight alternatives named and left to their own issues.
Second, "a single artifact larger than this bound cannot be cloned" is true but not the operative threshold. A round already holding bytes has that much less room, so the ceiling is the bound minus what the round accumulated first. Measured on this branch with a 58,720,256-byte artifact: 8, 12 and 15 MB of filler ahead of it all fail, and 17 MB passes because it exhausts the budget and pushes the artifact into a round of its own. Non-monotonic in the filler, which is worth knowing. That is #109, a defect this fix exposes rather than causes; the fixture's claim to guard it is withdrawn and it now says plainly that it covers the leading-artifact case only.
Third, the changelog's "pin the bound from both sides" overclaimed. The guards pin the bound against the batch budget and against the aliasing threshold. Neither certifies a round.
Rebased onto 8b6da9a. The internal/content -race failure reported last round was not a flake and not unrelated -- this branch predated #107, which rewrote available.go. TestIsAvailable_CyclicChainTerminates goes from exceeding its 30s budget to 0.06s across that rebase.
---------
Co-authored-by: Dan Mestas <[email protected]> (user: [email protected], size: 10678) [annotate] [blame] [check-ins using]
package xfer import ( "bufio" "bytes" "compress/zlib" "encoding/binary" "errors" "fmt" "io" ) // Message is a sequence of cards forming one xfer request or response. type Message struct { Cards []Card } // MaxDecompressedBytes caps how far one message may expand during // decompression. It bounds the expansion only. The compressed body itself // arrives through the transport, which reads it whole before calling Decode // (sync.HTTPTransport.Exchange), so this is not a bound on the memory a // hostile peer can cost the process -- it stops a small body from inflating // without limit, nothing more. Bounding the body on the way in belongs to the // transport and is not done today. // // It is a local resource guard, not a protocol rule: §4.1 places no limit on // the decompressed size of the compressed container. This implementation is // deliberately stricter than canonical here, which sizes its output buffer // from the attacker-supplied length prefix (blob.c blob_uncompress); the bound // below counts bytes actually inflated and ignores the advertised length. // // Because it is a guard and not a rule, it has to sit above anything a // conforming peer will send, or a legitimate exchange fails -- which is how // issue #104 arose, with the bound below what this implementation's own server // emitted in one clone round. Two constraints fix the range: // // - Above: a clone round carries sync.DefaultCloneBatchBytes of expanded // content plus the whole artifact that crossed that budget, so the bound // must clear the budget twice over. Enforced at compile time in // internal/sync. // - Below: it must stay under zlibCMFAliasBytes, so that no length prefix // this implementation writes can present as a zlib header. Enforced at // compile time immediately below. Read that constant's comment before // trusting the guard: it bounds what we emit, not what we accept. // // Consequence worth naming, and it is not the simple one. An artifact larger // than this bound can never be cloned, because the server must send it whole // to make progress. But the real threshold is lower and is not a property of // the artifact alone: a round already holding bytes has that much less room, // so the ceiling is this bound minus whatever the round accumulated before the // artifact. With a 16 MB budget an artifact much above 48 MiB clones only when // it happens to lead its round, and fails when ordinary artifacts precede it. // That ordering dependence is issue #109; it is a defect this fix exposes // rather than one this fix introduces or resolves. const MaxDecompressedBytes = 64 << 20 // 64 MiB // zlibCMFAliasBytes is the declared length at which the first byte of a §4.1 // container's big-endian length prefix can itself be a valid zlib CMF byte. // RFC 1950 requires CM == 8 in the low nibble, so the lowest aliasing prefix // byte is 0x08, reached at a declared length of 0x08000000. // // It matters because Decode attempts unprefixed zlib before the prefixed // container, and format 1 returns immediately if it succeeds -- format 2 is // consulted only when format 1 fails. A prefix that presents as a zlib header // therefore gets first refusal on a legitimate container. // // Be precise about what the guard below is worth, because it is less than it // looks: // // - It does NOT bound aliasing on input. The aliasing byte comes from the // length the *peer* declared, and this decoder never reads that field -- // it slices past it and counts bytes actually inflated. No value of // MaxDecompressedBytes constrains what a peer declares. A peer declaring // 0x081D0000 while sending 46 bytes produces a parsing header at offset 0, // confirmed by probe. // - What actually protects a real container in that case is the rest of the // stream, not arithmetic: after consuming two prefix bytes as a header, // inflate must still parse the remaining two prefix bytes plus the real // zlib stream as DEFLATE and match its Adler-32. It does not -- measured // `flate: corrupt input` on all three aliasing lengths probed -- so format // 1 fails, format 2 runs, and the container decodes correctly. The // residual risk is a stream that both parses as DEFLATE and hits a 32-bit // checksum by chance. // - What the guard DOES buy is narrow and real: this implementation's own // Encode writes uint32(len(raw)) as the prefix, so holding the bound below // this threshold means a body libfossil emits can never carry an aliasing // prefix. It is an invariant on our output, not a defence of our input. // // The airtight fix is to stop guessing the framing -- select on Content-Type // per §4, or reject a declared length above the bound before attempting format // 1. Both are redesigns of framing selection and neither belongs in #104. const zlibCMFAliasBytes = 0x0800_0000 // 128 MiB // Compile-time guards on MaxDecompressedBytes. Each subtraction underflows and // fails the build if the bound leaves its valid range, rather than letting a // clone fail at runtime on a message this implementation itself produced. const ( _ = uint(zlibCMFAliasBytes - 1 - MaxDecompressedBytes) _ = uint(MaxDecompressedBytes - 1) // must be positive. ) // errNotZlib marks a payload whose first two bytes are not a zlib header // (RFC 1950). It is the only decompression outcome that licenses the caller to // try a different framing: once a stream is recognized as zlib, a failure to // decode it is a real failure and must be reported, never reinterpreted. var errNotZlib = errors.New("xfer: not a zlib stream") // Encode serializes all cards and zlib-compresses the result. // Uses Fossil's compression format: 4-byte big-endian uncompressed size prefix // followed by standard zlib data. func (m *Message) Encode() ([]byte, error) { if m == nil { panic("xfer.Message.Encode: m must not be nil") } raw, err := m.EncodeUncompressed() if err != nil { return nil, err } var zbuf bytes.Buffer // 4-byte big-endian uncompressed size prefix (Fossil's blob_compress format). var sizePrefix [4]byte binary.BigEndian.PutUint32(sizePrefix[:], uint32(len(raw))) zbuf.Write(sizePrefix[:]) zw := zlib.NewWriter(&zbuf) if _, err := zw.Write(raw); err != nil { return nil, fmt.Errorf("xfer: message zlib write: %w", err) } if err := zw.Close(); err != nil { return nil, fmt.Errorf("xfer: message zlib close: %w", err) } return zbuf.Bytes(), nil } // EncodeUncompressed serializes all cards without zlib compression. func (m *Message) EncodeUncompressed() ([]byte, error) { var buf bytes.Buffer for i, c := range m.Cards { if err := EncodeCard(&buf, c); err != nil { return nil, fmt.Errorf("xfer: encode card %d (%T): %w", i, c, err) } } return buf.Bytes(), nil } // Decode decodes an xfer message. It tries three formats in order: // 1. Unprefixed zlib. No Fossil wire format produces this: every // application/x-fossil body carries the §4.1 length prefix, on both the // request side (http.c) and the reply side (cgi.c), both through // blob_compress. It is kept as a tolerance for a peer that omits the // prefix, not because canonical emits it. // 2. 4-byte BE length prefix + zlib (the §4.1 compressed container, which is // also what Encode produces, and what every Fossil peer sends). // 3. Uncompressed card data (clone protocol v3, x-fossil-uncompressed). // // Compressed bytes are never handed to the card parser. A payload that is // recognizable as zlib but fails to decode — corrupt stream, or larger than // MaxDecompressedBytes — is reported as the transport failure it is. Falling // through to format 3 in that case parses the still-compressed body as card // text, which yields thousands of nonsense cards and finally an arbitrary // syntax error thousands of cards deep, naming neither the real fault nor the // real position (issue #104). func Decode(data []byte) (*Message, error) { if len(data) == 0 { return &Message{}, nil } raw, err := decompressContainer(data) if err == nil { return DecodeUncompressed(raw) } if !errors.Is(err, errNotZlib) { return nil, err } // Format 3: the body is not zlib under either framing, so it is either // uncompressed card data or garbage. The card parser tells them apart. return DecodeUncompressed(data) } // decompressContainer decompresses a message body under either compressed // framing. Format 1 wins outright if it decodes; format 2 is tried only when // format 1 fails. The error comparison at the end therefore decides nothing // about which framing is used -- it only selects which failure to report when // both have already failed. It returns errNotZlib only when neither framing is // a zlib stream at all. func decompressContainer(data []byte) ([]byte, error) { // Format 1: unprefixed zlib. See Decode; a tolerance, not a Fossil format. rawDirect, errDirect := decompressBounded(data) if errDirect == nil { return rawDirect, nil } if len(data) < 4 { return nil, errDirect } // Format 2: 4-byte BE length prefix + zlib (§4.1). rawPrefixed, errPrefixed := decompressBounded(data[4:]) if errPrefixed == nil { return rawPrefixed, nil } // Both framings failed. Report the one that was actually recognized as // zlib; a length prefix can coincidentally parse as a zlib header, so // "format 1 failed" alone does not mean the body is corrupt. if errors.Is(errDirect, errNotZlib) { return nil, errPrefixed } return nil, errDirect } // decompressBounded decompresses one zlib stream with a size cap. // It returns errNotZlib when data does not begin with a zlib header, and a // descriptive error when a real zlib stream fails to decode or exceeds // MaxDecompressedBytes. func decompressBounded(data []byte) ([]byte, error) { zr, err := zlib.NewReader(bytes.NewReader(data)) if err != nil { return nil, fmt.Errorf("%w: %v", errNotZlib, err) } defer zr.Close() lr := io.LimitReader(zr, MaxDecompressedBytes+1) raw, err := io.ReadAll(lr) if err != nil { return nil, fmt.Errorf("xfer: decompress message: %w", err) } if len(raw) > MaxDecompressedBytes { return nil, fmt.Errorf( "xfer: decompressed message exceeds %d bytes", MaxDecompressedBytes) } return raw, nil } // DecodeUncompressed decodes cards from uncompressed data. func DecodeUncompressed(data []byte) (*Message, error) { r := bufio.NewReader(bytes.NewReader(data)) msg := &Message{} for { card, err := DecodeCard(r) if err == io.EOF { return msg, nil } if err != nil { return nil, fmt.Errorf("xfer: decode card %d: %w", len(msg.Cards), err) } msg.Cards = append(msg.Cards, card) } }