Repository navigation
fix: size compressed stream reads for LZW expansion; issue remainders (#21, #18, #16) - #23
Merged
Merged
Conversation
- readStream bounds a compressed stream message by the largest valid LZW form of a maxDecompressedBytes payload (1.5x plus clear/end codes, ~60 MiB) instead of the 40 MiB plaintext cap; plaintext messages keep the 40 MiB cap through a limited source. Senders compress regardless of gain, so v0.8.0 refused legitimate push/pull states of ~29-40 MiB with incompressible content. RED: a 32.7 MB state (20,000 nodes with random meta + 20 MiB random user state) compressed to 43.7 MB and failed with 'compress.Buf: msgpack: truncated input'. - Regression tests for #21 (compress.Buf and node fields declaring multi-gigabyte lengths are refused with bounded allocation) and #16 (LocalNode reports StateLeft after Leave). - CONTRIBUTING.md and PR template adapted from upstream hashicorp#375 (#18). Fixes #21 Fixes #18
- #21 tests declare lengths inside the field limits (60 MiB compress.Buf, 1 MiB Meta) with per-case allocation budgets; mutating readInto to allocate the declared size fails both (62.9 MB and 1.06 MB allocated), where the previous version still passed. - Senders refuse, with ErrMessageTooLarge, encrypted stream messages whose ciphertext exceeds the 20 MiB receivers accept (push/pull state or SendReliable). RED: SendReliable of 20 MiB returned 'read/write on closed pipe' after a 10 s timeout. - The incompressible-state test skips under the race detector (16 s there, no concurrency to check). - readStream no longer allocates a limited source for encrypted messages; PR template uses an absolute CONTRIBUTING link; CHANGELOG range and encrypted-limit notes corrected.
…compress only when smaller
…to the initiator, pin LZW budget
…m user-state limit
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
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.
Description
Closes the remainder of the open issues after v0.8.0 (#22).
readStreamcapped the whole connection read at the 40 MiB plaintext limit. Senders compress stream messages whether or not that shrinks them, and LZW expands incompressible input (measured 1.37×; provably at most 1.5× plus clear and end codes), so a legitimate push/pull of ~29–40 MiB with incompressible content was refused. A compressed message is now read againstmaxCompressedStreamBytes(the largest valid LZW form of amaxDecompressedBytespayload, ~60 MiB) — the derivation Raw stream decoding permits unbounded wire-driven allocations (compress.Buf and per-node fields decoded before any cap) #21 asked for — while plaintext messages keep the 40 MiB cap through a limited source. Decompression stays capped at 40 MiB.compress.Bufdeclares 60 MiB, or a node'sMeta1 MiB — within the field limits, so only allocation that grows with received bytes saves memory — but carries 1 KiB is refused with under 4 MiB / 512 KiB allocated. Mutating the decoder to allocate the declared size up front fails both (62.9 MB / 1.06 MB). (The bounds themselves shipped in v0.8: std-only module, hardened wire path, owned codec, slog/metrics, synctest #22.)ErrMessageTooLargeinstead of writing a message that is dropped (RED:SendReliableof 20 MiB returnedread/write on closed pipeafter a 10 s timeout).TestEncryptedStreamSenderLimitderives the exact boundary: the largest message whose ciphertext is exactly 20 MiB is delivered, one byte more is refused.ErrMessageTooLarge(before: an unwrapped error, or nothing for the user state), and a responder whose state is too large answers the initiator with that error, soJoinreports the cause instead ofEOF(RED:push/pull error EOF).LocalNode().StatereportsStateLeftafterLeave(the shadowing itself was removed in v0.8: std-only module, hardened wire path, owned codec, slog/metrics, synctest #22).CONTRIBUTING.mdand the PR template adapted from upstream docs: Add contributing guide and LLM PR template link. hashicorp/memberlist#375 (the security model was ported in v0.8: std-only module, hardened wire path, owned codec, slog/metrics, synctest #22).Fixes #21. Fixes #18.
How has this been tested?
RED before the fix —
TestReadStreamAcceptsIncompressibleCompressedState, a 32.7 MB state (20,000 nodes with 512 random meta bytes plus 20 MiB random user state) compressed to 43.7 MB:GREEN after.
TestReadStreamBoundsPlainMessagestill refuses a >40 MiB plaintext message. The large-state test skips under the race detector (no concurrency; 16 s there).Review loop: round 1 (independent reviewer) verified the LZW bound (Go's writer: ≤ 1.5N + 1.5N/3838 + 5 bytes; 40 MiB → ≤ 62.93 MB ≤
maxCompressedStreamBytes63.02 MB; measured worst practical input 1.409×) and every stream path, and requested the discriminating #21 tests and the encrypted sender limit above. Round 2 requested an exact-boundary sender test, a plaintext cap edge test (TestReadStreamPlaintextCapEdge: exactly 40 MiB accepted, +1 refused) and compress-only-when-smaller. Mutation checks: sender limit on plaintext length,>=for>, plaintext cap +1 and always-compress each fail their test. Round 3 (verdict MERGE) confirmed every stream form and upstream interop (upstream accepts plaintext streams whatever its compression setting) and raised three Low findings, fixed: the initiator's bareEOF, the inconsistent error identity, and no test pinningmaxCompressedStreamBytesto the LZW worst case (TestCompressedStreamBudgetCoversLZWWorstCase: 62,930,968 needed ≤ 63,021,056; mutating the budget to 45 MB or 1.5N+1 fails it). Mutating out the error response or the user-state check fails its test. Round 4 (verdict MERGE, delta review) verified nothing is written before anyErrMessageTooLargereturn, that upstream initiators already handleerrMsg, and the LZW arithmetic againstcompress/lzw(clear every 3,838 data codes; comment and test corrected); README notes that Mori refuses to send user state above 20 MiB, which upstream receivers accept.make cipasses (fmt, go fix, vet, lint, cross, deps, tidy, tests on amd64 and 386, race, bench smoke).Checklist
make cipasses.Unreleased.