state: harden FromBytes against truncated input + 100% coverage

[poolpOrg]

Why

Five *FromBytes routines in repository/state read from a bytes.Buffer using bbuf.Next(N) without length checks. On truncated input, binary.LittleEndian.Uint16 / Uint32 / Uint64 indexes past the slice and panics. This is reachable from production code paths (BlobExists, GetSubpartForBlob, every List*, insertOrUpdateConfiguration, mergeFromCache, ListPackfileEntries) any time the cache returns a short blob — making a corrupted on-disk state file a process-killer.

Repro on main:

buf := []byte{0x05} // declares keyLen=5 then nothing else
ConfigurationEntryFromBytes(buf)
// panic: runtime error: index out of range [1] with length 0

What changed

Production code

• DeleteEntryFromBytes, DeltaEntryFromBytes, PackfileEntryFromBytes, ColouredEntryFromBytes: validate len(buf) against XxxSerializedSize up front, then decode from a single slice with no further short-read exposure. Returns a clean error instead of panicking.
• ConfigurationEntryFromBytes (variable-length entry): explicit length checks at each step (keyLen, valueLen, timestamp). Also copies the decoded Value so callers don't end up aliased to a pooled buffer.
• deserializeFromStream / v100: the *FromBytes calls inside the stream loop are now guaranteed-safe by the surrounding length check, so the defensive double-checks on the FromBytes return are removed as dead code.
• PutState: drops the dead Metadata.ToBytes error path (msgpack on a fixed struct cannot fail).
• state_test.go mock fix: PutDeleted was writing to c.coloured instead of c.deleted — this was masking the GetDeletedEntries iterator in serialization tests.

New tests — state_corruption_test.go, 80+ tests organized around corruption surfaces

• FromBytes truncation sweep: for every fixed-size FromBytes (Delete, Delta, Packfile, Coloured), feed every prefix length 0..N-1 plus nil and assert: no panic, error returned. ConfigurationEntryFromBytes gets its own sweep plus crafted entries where the declared keyLen / valueLen overflow the buffer.
• No-alias check for ConfigurationEntry.Value.
• deserializeFromStream / v100 error branches: short header, missing entry type, truncated length field, wrong-length entries for every kind, truncated body for every kind, invalid DELETE embedded type, unknown entry type (must be skipped, not error), every truncation point in the metadata trailer.
• Cache-error propagation: erroring stub caches drive Put errors on every entry kind through deserializeFromStream, deserializeFromStreamv100, and mergeFromCache. Has*-error paths drive every error-yield branch in ListSnapshots / ListObjectsOfType / ListOrphanDeltas and the HasPackfile error branch in BlobExists / GetSubpartForBlob.
• MergeState / MergeStateFromCache / PutState / UpdateSerialOr cache error paths.
• insertOrUpdateConfiguration: newer-wins, older-ignored, equal-timestamp-no-op, corrupt stored bytes => error, Put errors on both insert and update branches.
• Iterator break paths: List* consumers that break after one item exercise the if !yield return short-circuits.
• Round-trip property: serialize -> deserialize -> serialize is byte-identical (anti-corruption invariant).
• Byte-flip fuzz: every position of a known-good stream gets XOR'd; deserializer must error or accept, never panic. Random-garbage sweep for lengths 0..199. Same fuzz against deserializeFromStreamv100.
• FromStream dispatch: unknown version takes the v100 branch.

Coverage

repository/state goes from 68.2% → 100% of statements.

Test plan

• go test ./repository/state/ — 153 tests pass
• go test ./... — full suite passes
• go vet ./... — clean

Known follow-up

ListSnapshots, ListObjectsOfType, ListOrphanDeltas are missing a continue after yielding a decode error — they then call HasPackfile(de.Location.Packfile) on the zero DeltaEntry and may yield it as a real result. Not fixed here to keep this PR focused on the panic surface; happy to follow up in a separate PR.

[poolpOrg]

THIS IS NOT MEANT TO BE MERGED.

This is meant to discuss the finding.

[mathieu-plak]

@claude review

[claude]

🔴 ET_CONFIGURATION allocates length bytes up front without bounds — 4 GiB DoS on a corrupted state file. In both deserializeFromStream (state.go:726) and deserializeFromStreamv100 (state.go:857), the ET_CONFIGURATION arm does ce_buf := make([]byte, length) with length being a uint32 read directly from the (attacker-controlled) stream. Every other entry type in the same switch was hardened by this PR to gate on length != XxxSerializedSize before allocating into a pre-sized fixed buffer; ET_CONFIGURATION is the only branch left that allocates length bytes up front, so a 37-byte crafted state file with length=0xFFFFFFFF forces an immediate ~4 GiB allocation before io.ReadFull errors out — exactly the "corrupted on-disk state file a process-killer" threat model the PR description names. Fix is one-liner mirroring the existing pattern: cap length at the structural maximum for a legitimate ConfigurationEntry (1 + 255 + 2 + 65535 + 8 = 65801 bytes) before make, in both deserializers.

Extended reasoning...

What the bug is

Inside the per-entry switch of both deserializeFromStream (state.go:684-744) and deserializeFromStreamv100 (state.go:815-875), the ET_CONFIGURATION case is the only branch that allocates an entry buffer using the attacker-controlled length field from the wire, with no upper bound:

case ET_CONFIGURATION:
    ce_buf := make([]byte, length)    // length is a uint32 from the stream
    if n, err := io.ReadFull(r, ce_buf); err != nil { ... }

The PR explicitly hardens every other entry type in the same loop — ET_DELETE (state.go:670), ET_LOCATIONS (state.go:680), ET_COLOURED (state.go:691), ET_PACKFILE (state.go:710) — with a if length != XxxSerializedSize { return error } guard before any read, and reads into a pre-allocated fixed-size buffer (del_buf, de_buf, coloured_buf, pe_buf). ET_CONFIGURATION sits inside that same hardening loop but skips the guard entirely.

Why existing safeguards don't catch it

• ConfigurationEntryFromBytes performs its defensive checks after the allocation — it protects against bad content but not against bad declared size.
• The new TestDeserializeFromStream_BadConfigContent only exercises length=1, which never approaches the dangerous regime.
• TestDeserializeFromStream_AllByteFlipsSafe does flip bytes in the length field, but it only asserts no-panic via notPanic. A successful 4 GiB allocation followed by a clean io.ReadFull EOF error does not panic — it returns an error and notPanic is satisfied. On CI hosts with < 4 GiB free RAM this may surface as intermittent OOM-killed test runs rather than a clear failure.

Concrete step-by-step proof

Craft this 37-byte payload:

bytes  0..31  : zeroed (the 32-byte parent header — accepted by ReadFull)
byte   32     : 0x05            (ET_CONFIGURATION)
bytes  33..36 : FF FF FF FF     (length = 4 294 967 295, little-endian uint32)

Trace through deserializeFromStream in state.go:

1. Line ~669 reads the 32-byte header into ls.Metadata.Parent — OK.
2. Line ~676 reads one byte into et_buf, gets 0x05 → entryType = ET_CONFIGURATION.
3. Line ~688 calls readUint32() which reads bytes 33–36 → length = 0xFFFFFFFF = 4 294 967 295.
4. Switch dispatches to state.go:725 ET_CONFIGURATION → ce_buf := make([]byte, 4294967295) allocates and zeroes ~4 GiB before any validation.
5. Line 728 io.ReadFull(r, ce_buf) then fails on the short stream and returns an error — but the 4 GiB has already been committed.

On hosts with less than 4 GiB free this is an immediate OOM-kill of the kloset process. On larger hosts it's a multi-second stall and severe memory pressure on co-tenants (page-cache eviction, swap thrash). The same shape applies verbatim to deserializeFromStreamv100 at state.go:857, no header byte needed.

Reachability — production callers

This is reachable along exactly the surfaces the PR description names:

• repository/repository.go:386 — MergeState is called with a reader returned by r.GetState over remote storage.
• repository/repository.go:422 — MergeState is called with an on-disk state file via OpenStateFromStateFile.

A malicious remote storage backend, a corrupted disk, or a tampered state file all reach this branch.

Fix

Same pattern the PR already uses for the fixed-size entries — cap before allocate. The legitimate maximum for a ConfigurationEntry is structurally bounded because keyLen is a uint8 (max 255) and valueLen is a uint16 (max 65535):

1 (keyLen u8) + 255 (max key) + 2 (valueLen u16) + 65535 (max value) + 8 (timestamp) = 65801 bytes

Any length larger than that cannot round-trip from ConfigurationEntry.ToBytes and should be rejected:

case ET_CONFIGURATION:
    const maxConfigEntrySize = 1 + 255 + 2 + 65535 + 8
    if length > maxConfigEntrySize {
        return fmt.Errorf("configuration entry length %d exceeds max %d", length, maxConfigEntrySize)
    }
    ce_buf := make([]byte, length)
    ...

Apply the same one-liner at state.go:857 inside deserializeFromStreamv100. While here, it would also be worth strengthening TestDeserializeFromStream_AllByteFlipsSafe (and the v100 counterpart) so that an arbitrarily-large declared length produces a fast error rather than an attempted allocation — e.g. by asserting that decode completes within a small memory budget, or by adding a targeted length=0xFFFFFFFF regression test alongside the existing length=1 case.

[claude]

🟡 TestBlobExists_HasPackfileError at state_corruption_test.go:1001-1026 does not exercise the path its name advertises — the wrapped errCache with hasPackfileErr is constructed and immediately discarded (_ = wrapped), and NewLocalState(cache) is called with the plain deltaInjector whose underlying mockStateCache has no packfile registered, so HasPackfile returns (false, nil) and the assertion exists == false holds for the trivial "packfile missing" reason, not the error reason. The actual HasPackfile-error coverage is provided by TestBlobExists_HasPackfileErrorContinues immediately below using hasPackfileErrInjector. Either delete this test as redundant, or compose a cache that yields both a valid DeltaEntry from GetDelta and an erroring HasPackfile so it really drives the named branch.

Extended reasoning...

What the test claims vs what it does. The test is named TestBlobExists_HasPackfileError and the file-level intent (see the PR description's "HasPackfile-error paths" bullet) is to cover the if err != nil { continue } branch on the cache.HasPackfile call inside BlobExists (state.go:1004 in the modified file). The test body builds a wrapped := &errCache{mockStateCache: cache.mockStateCache, hasPackfileErr: errors.New("boom")} (line 1014) — clearly intending to install the erroring HasPackfile — then on line 1018 writes _ = wrapped, dropping the wrapper on the floor. The author's own inline comment (lines 1015-1017) confesses: "Easier — just verify BlobExists returns false when HasPackfile errors using a direct test." But the test that follows is not that direct test — it's just the missing-packfile path.\n\nStep-by-step proof that the error branch is not exercised. Walk it on the actual modified BlobExists (state.go:1000-1018):\n\n1. Line 1022 calls NewLocalState(cache) — note cache, not wrapped. cache is the bare deltaInjector built on line 1010-1013.\n2. BlobExists iterates ls.cache.GetDelta(...). deltaInjector.GetDelta yields one entry with the encoded de whose Location.Packfile == pfMAC == {0xC0}.\n3. DeltaEntryFromBytes succeeds (de.ToBytes was used).\n4. ls.cache.HasPackfile(pfMAC) is called. deltaInjector does not override HasPackfile — it inherits it from the embedded *mockStateCache. mockStateCache.HasPackfile (state_test.go:213-216) is a simple map lookup: _, exists := m.packfiles[packfile]; return exists, nil. The test never inserts pfMAC into packfiles, so this returns (false, nil) — the no-error branch.\n5. ok is false, the if ok && !coloured predicate is false, the loop ends, BlobExists returns false.\n6. require.False(t, exists) passes — but for the missing-packfile reason, not the HasPackfile-error reason.\n\nNotice the hasPackfileErr: errors.New("boom") setup on wrapped is never consulted by any call path the test triggers. The dead variable wrapped is a compile-time-only construct here.\n\nWhy this matters / impact. Test-quality, not production. The named branch is covered separately by TestBlobExists_HasPackfileErrorContinues (lines 1043-1058 of the same file), which uses hasPackfileErrInjector that genuinely overrides HasPackfile to return (false, errors.New("boom")). So coverage of the production code is fine; the issue is that future readers (and go vet -unused consumers, and the 100% coverage claim in the PR description) will see a named "HasPackfile error" test that doesn't test that. Dead code in tests rots — if someone later changes BlobExists to return the HasPackfile error instead of continue-ing past it, this test still passes (false positive).\n\nHow to fix. Two clean options:\n\n1. Delete it — the immediately-following TestBlobExists_HasPackfileErrorContinues already covers the branch with the right scaffolding. Remove lines 1001-1026.\n2. Rename + minimize — if the intent is to also document the missing-packfile path, rename the test to TestBlobExists_MissingPackfile, drop the wrapped setup and the misleading author comment, and keep only the bare assertion.\n\nGiven that the PR is explicitly NOT meant to be merged (see poolpOrg's first comment), this is just worth flagging so the cleanup happens before any follow-up PR lands the real fix.