Artifact 9d800c656b77f2388cb5081035180f58e7228dfe980c2859e80e30dc24af3a2b:
- File
internal/sync/clone_selfroundtrip_test.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: 8440) [annotate] [blame] [check-ins using]
package sync_test import ( "context" "fmt" "math/rand" "net" "path/filepath" "testing" "time" "github.com/danmestas/libfossil/internal/manifest" "github.com/danmestas/libfossil/internal/repo" "github.com/danmestas/libfossil/internal/sync" "github.com/danmestas/libfossil/internal/xfer" "github.com/danmestas/libfossil/simio" ) // blobUUIDs returns every content-bearing blob hash in r. Phantoms (size < 0) // are excluded: they are placeholders for artifacts the repository has heard // named but does not hold, and a clone is not expected to carry them. func blobUUIDs(t *testing.T, r *repo.Repo) map[string]bool { t.Helper() rows, err := r.DB().Query("SELECT uuid FROM blob WHERE size >= 0") if err != nil { t.Fatalf("query blobs: %v", err) } defer rows.Close() out := map[string]bool{} for rows.Next() { var uuid string if err := rows.Scan(&uuid); err != nil { t.Fatalf("scan blob: %v", err) } out[uuid] = true } if err := rows.Err(); err != nil { t.Fatalf("iterate blobs: %v", err) } return out } // incompressibleBytes returns n bytes that zlib cannot shrink, from a fixed // seed so the corpus is identical on every run. Compressible filler would make // the wire body far smaller than the artifact it carries, and the size of the // wire body is the whole point of the fixtures below. func incompressibleBytes(seed int64, n int) []byte { if n <= 0 { panic("incompressibleBytes: n must be positive") } buf := make([]byte, n) if _, err := rand.New(rand.NewSource(seed)).Read(buf); err != nil { panic("incompressibleBytes: " + err.Error()) } return buf } // serveRepo starts an xfer HTTP server for r on a loopback port and returns // its base URL. It waits for the listener to accept a connection rather than // sleeping, and fails the test if the server returns before then. func serveRepo(ctx context.Context, t *testing.T, r *repo.Repo) string { t.Helper() probe, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { t.Fatalf("reserve port: %v", err) } addr := probe.Addr().String() if err := probe.Close(); err != nil { t.Fatalf("release port: %v", err) } errc := make(chan error, 1) go func() { errc <- sync.ServeHTTP(ctx, addr, r, sync.HandleSync) }() deadline := time.Now().Add(10 * time.Second) for { conn, dialErr := net.DialTimeout("tcp", addr, 200*time.Millisecond) if dialErr == nil { conn.Close() return "http://" + addr } select { case serveErr := <-errc: t.Fatalf("ServeHTTP returned before accepting on %s: %v", addr, serveErr) default: } if time.Now().After(deadline) { t.Fatalf("server did not accept on %s within 10s: %v", addr, dialErr) } time.Sleep(10 * time.Millisecond) } } // cloneSelfAndCompare serves srcRepo with this implementation's server, clones // it with this implementation's client over HTTP, and requires the clone to // hold every artifact the source holds. func cloneSelfAndCompare(t *testing.T, srcRepo *repo.Repo, dir string) *sync.CloneResult { t.Helper() want := blobUUIDs(t, srcRepo) if len(want) == 0 { t.Fatal("source repository holds no blobs") } ctx, cancel := context.WithCancel(context.Background()) defer cancel() url := serveRepo(ctx, t, srcRepo) clonePath := filepath.Join(dir, "clone.fossil") cloneRepo, result, err := sync.Clone( ctx, clonePath, &sync.HTTPTransport{URL: url}, sync.CloneOpts{}) if err != nil { t.Fatalf("Clone: %v", err) } defer cloneRepo.Close() got := blobUUIDs(t, cloneRepo) var missing []string for uuid := range want { if !got[uuid] { missing = append(missing, uuid) } } if len(missing) > 0 { t.Fatalf("clone is missing %d of %d artifacts: %v", len(missing), len(want), missing) } return result } // newSelfRoundTripRepo creates an empty source repository under dir. func newSelfRoundTripRepo(t *testing.T, dir string) *repo.Repo { t.Helper() r, err := repo.Create(filepath.Join(dir, "source.fossil"), "testuser", simio.CryptoRand{}, "") if err != nil { t.Fatalf("repo.Create: %v", err) } return r } // TestCloneSelfRoundTrip clones a repository this implementation is serving, // with this implementation's client, over the real HTTP transport, and // requires artifact parity. // // Method: a small corpus, served and cloned, compared by blob-hash set. // // This is a coverage plank, not a guard on any particular defect. It exists // because every other clone test here is either driven by real fossil or wired // through MockTransport, which hands *xfer.Message values straight across and // never encodes a wire body at all. Nothing covered a body this implementation // produced being decoded by this implementation's decoder, which is the only // configuration in which a serialization defect meets a decoder that disagrees // with it. TestCloneSelfRoundTripOversizeArtifact is the actual #104 // regression test; this one is cheap and runs everywhere, including -short. func TestCloneSelfRoundTrip(t *testing.T) { dir := t.TempDir() srcRepo := newSelfRoundTripRepo(t, dir) defer srcRepo.Close() for c := 0; c < 3; c++ { if _, _, err := manifest.Checkin(srcRepo, manifest.CheckinOpts{ Comment: fmt.Sprintf("checkin %d", c), User: "testuser", Files: []manifest.File{ {Name: fmt.Sprintf("text%d.txt", c), Content: []byte(fmt.Sprintf("checkin %d\n", c))}, {Name: fmt.Sprintf("data%d.bin", c), Content: incompressibleBytes(int64(c), 64<<10)}, }, }); err != nil { t.Fatalf("Checkin %d: %v", c, err) } } cloneSelfAndCompare(t, srcRepo, dir) } // TestCloneSelfRoundTripOversizeArtifact is the #104 regression test: it // requires a clone to survive a round whose body is larger than the decoder's // bound used to be. // // Method: one incompressible artifact of 7/8ths of xfer.MaxDecompressedBytes, // served and cloned by this implementation, compared by blob-hash set. // // Why that size, precisely. The defect was never about artifact count or total // corpus size. It was that the decoder's bound on one decompressed message sat // below what one round can emit -- and a round is not bounded by // sync.DefaultCloneBatchBytes. The budget is charged before each artifact and // the artifact that crosses it is sent whole, so a round carries up to the // budget plus one entire artifact of unbounded size. A corpus of many small // artifacts therefore cannot reach the bound however large the corpus grows, // because pagination keeps every round near the budget; only a single large // artifact can. // // What this fixture covers, exactly: a large artifact that LEADS its round, so // the round has accumulated nothing before it. That is the easy case. An // artifact of this size riding behind ordinary filler still exceeds the bound // -- 8, 12, and 15 MB of filler ahead of it all fail on this fixed branch, // while 17 MB passes because it exhausts the budget and pushes the artifact // into a round of its own. That ordering dependence is issue #109, a defect // this fix exposes rather than one it introduces or resolves. This test is // deliberately not a guard on it. // // The size is expressed against MaxDecompressedBytes rather than as a literal // so it tracks the bound: 7/8ths clears any bound an eighth below the current // one -- which the bound this replaced was -- while leaving an eighth of // headroom beneath the current one. func TestCloneSelfRoundTripOversizeArtifact(t *testing.T) { if testing.Short() { t.Skip("allocates an artifact of 7/8ths of xfer.MaxDecompressedBytes") } const artifactBytes = xfer.MaxDecompressedBytes / 8 * 7 if artifactBytes <= 2*sync.DefaultCloneBatchBytes { t.Fatalf("artifact of %d bytes does not exceed twice the clone batch budget of %d", artifactBytes, sync.DefaultCloneBatchBytes) } if artifactBytes >= xfer.MaxDecompressedBytes { t.Fatalf("artifact of %d bytes cannot fit under the decode bound of %d", artifactBytes, xfer.MaxDecompressedBytes) } dir := t.TempDir() srcRepo := newSelfRoundTripRepo(t, dir) defer srcRepo.Close() if _, _, err := manifest.Checkin(srcRepo, manifest.CheckinOpts{ Comment: "one oversize artifact", User: "testuser", Files: []manifest.File{ {Name: "oversize.bin", Content: incompressibleBytes(1, artifactBytes)}, }, }); err != nil { t.Fatalf("Checkin: %v", err) } result := cloneSelfAndCompare(t, srcRepo, dir) if result.BlobsRecvd < 2 { t.Errorf("BlobsRecvd = %d, want >= 2 (artifact and manifest)", result.BlobsRecvd) } }