Repository navigation
s2: fix LZ4 block conversion writing past dst - #1244
Conversation
The literal bound in the S2-output converters ignored the 1-5 byte literal header, leaving too little room for a following copy with a 5-byte repeat (up to 8 bytes). The assembly wrote a few bytes past cap(dst) before returning ErrDstTooSmall; the pure-Go path panicked. Matches over ~16.8 MB chained 5-byte repeat chunks with no check, so the overrun grew with the match length. Tighten the existing literal bound so a copy always fits, and check the limit only before each extra repeat chunk. The Snappy-output converters already checked every chunk. Add an unexported noAsm field so tests reach the pure-Go path without build tags.
FuzzLZ4Block gave the converter 2*len(data)+4096 bytes, which S2 output never comes close to, and asm writes past cap(dst) are invisible without a guard. Convert again with every variant into need +/- a deterministic amount up to len(data), with 16 guard bytes after cap, and check the guard and that the result agrees with the large-dst run. On the unfixed code this found an overrun within minutes; the input is added to the seed corpus.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe LZ4 and LZ4s converters update destination-capacity checks and repeat emission. Converter types can bypass assembly. AMD64 and ARM64 conversion paths receive corresponding updates, and tests cover output equivalence and writes beyond destination capacity. ChangesLZ4 conversion
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Suggested reviewers: Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes strengthen bounds handling for caller-provided buffers without adding a new entrypoint or privilege boundary. Low-level implementations on two architectures and incomplete security verification leave residual uncertainty. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
FuzzLZ4Block covers the assembly and pure-Go converters in one run, so add the assembly variants only when hasAsm is set, and run the fuzzer in CI without the noasm,nounsafe pass, on both amd64 and arm64.
Alternative to #1243. That change fixes only the pure-Go path, and does it with a check on every sequence. The assembly has the same bug. It doesn't reject these inputs up front: it writes past
cap(dst)first and returnsErrDstTooSmallafterwards, so the corruption goes unnoticed.(1) s2: stop LZ4 block conversion from writing past dst (a3d8c86)
LZ4Converter.ConvertBlockandLZ4sConverter.ConvertBlock(S2 output) could write pastcap(dst):d+ll, but not the 1–5 byte literal header. After a long literal only 10−h bytes (Go) or 9−h bytes (asm) are left, but a copy with a 5-byte repeat needs up to 8. The assembly writes a few bytes pastcap(dst); the pure-Go path panics.This adds no checks to the common path:
len(dst)-13in Go. In asm the literal limit is computed once before the loop, so the check staysLEAQ/CMPQ/JAEon amd64 and gains no instruction on arm64. dst now needs 3–4 more bytes of spare capacity; buffers sized withMaxEncodedLenare unaffected.emitRepeatin the generator gets an optionaldstLimit; only the converters pass one, so the encoders' generated code is unchanged.The extra register makes amd64
cvtLZ4BlockAsm/cvtLZ4sBlockAsmuse BP (frame$0→$8), which renames registers through both functions; that is most of the.sdiff. The Snappy-output converters already checked every chunk and are unchanged.An unexported
noAsmfield lets tests reach the pure-Go path without build tags.Test:
TestLZ4ConvertBlockDstBoundsins2/lz4convert_test.goconverts crafted blocks into each dst size near the limit, for all eight converter/path combinations, with guard bytes pastcap(dst). It fails before the fix with and withoutnoasm.(2) s2: fuzz LZ4 conversion with under- and oversized dst (a6b91cf)
FuzzLZ4Blockgave the converter2*len(data)+4096bytes, which S2 output never comes close to, and an asm write pastcap(dst)isn't visible anyway. It now converts again with every variant intoneed ± crc32(data)%len(data)bytes with 16 guard bytes after the cap, and checks the guard and that the result agrees with the large-dst run. On the unfixed code it found an overrun within minutes: a block that is corrupt as LZ4s, which overran dst before the corruption was detected. That input is added totestdata/fuzz/FuzzLZ4Block.zip. Each exec now does 16 more conversions.Test:
FuzzLZ4Blockins2/lz4convert_test.go.Testing:
go test ./s2/...with default,noasm,nounsafeandnounsafe,noasm;go vetfor amd64 and arm64; the regenerated assembly is byte-identical to the committed files.FuzzLZ4Blockran 5 minutes on the default build (which now covers both asm and Go) and 2 minutes withnoasm; longer fuzz runs are in progress. Not run on arm64 hardware locally.Not addressed here: for literals of 8 bytes or less the assembly loads 8 bytes from
src, so a short final literal reads up to 7 bytes past the end ofsrc.Reported by @Raghuvardhan-Reddy78
Summary by CodeRabbit