fix(fsst): correct decoder output-buffer size contract to 8x#7589
Open
LuciferYang wants to merge 3 commits into
Open
fix(fsst): correct decoder output-buffer size contract to 8x#7589LuciferYang wants to merge 3 commits into
LuciferYang wants to merge 3 commits into
Conversation
`decompress_bulk` writes a full 8-byte word per code and advances the output cursor by only the symbol length, relying on a later write (or spare buffer capacity) to cover the slack. The maximum symbol length is 8, so a 1-byte code can expand to 8 bytes and the decoded output can be up to 8x the compressed input. The final symbol has no following write, so the output buffer must have room for a full 8-byte store at its last code — i.e. it must be at least 8x the input. The size guard in `FsstDecoder::init` only required 3x, and the public `decompress` doc likewise claimed "at least 3 times". That guard would pass a 3x-sized buffer that then gets written out of bounds by the decode loop. It is not reachable today because all three in-tree callers (`FsstPerValueDecompressor`, `FsstMiniBlockDecompressor`, `FsstPageDecoder`) already allocate `in_buf.len() * 8`, so the guard is a no-op for them. But the contract was undocumented and the guard was unsound for any future caller that trusted the "3 times" wording. Changes (no behavior change for existing callers, no extra allocation): - Raise the guard from `* 3` to `* 8` — a threshold comparison, not an allocation. Existing 8x callers still pass; a <8x buffer is now rejected instead of silently overflowing. - Fix the misleading "3 times" comment and pub-doc to "8 times" and explain why. - Add `// SAFETY:` comments to the unaligned load/store blocks in `fsst_unaligned_load_unchecked` and `decompress_bulk`, documenting the now-explicit invariants (>=8 readable bytes for loads; out_buf >= 8x in_buf for the 8-byte writes). - Add a regression test asserting a 1-byte-short output buffer is rejected and an exact-8x buffer succeeds.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The SAFETY comments added in the previous commit overstated two things: - The load-safety note claimed compress_bulk is kept in bounds by a sentinel byte. The actual guarantee is the read offset staying < 511 in a 520-byte buffer (>= 8 bytes remain); the sentinel serves the FSST terminator logic, not load bounds. - The closure note claimed in_end is bounded by compressed_strs.len(). in_end is a caller-provided offset that the decoder does not validate, so the unchecked u32 read is only sound for well-formed offsets. State that as a relied-upon precondition rather than an established fact, and clarify that the scalar paths use bounds-checked indexing. Comment-only; no behavior change.
…e + docs Follow-up from multi-persona review of the decoder output-buffer contract: - Guard against 32-bit usize overflow in the 8x check: use `in_buf.len().checked_mul(8).is_none_or(|needed| needed > out_buf.len())` so a >= 512 MiB input can't wrap `len * 8` and bypass the check. - Refine the SAFETY comments to honestly disclose their preconditions: `lens[code]` is loaded verbatim from the (untrusted) symbol table and is not re-validated <= 8, and `in_end` (from the offsets) is a trusted, unchecked precondition. Reconcile the inline u32-read comment with the closure-level one (both now describe `in_end <= len` as trusted, not proven). Hardening the decoder against corrupt on-disk structures is a separate concern, tracked outside this PR. - Update the benchmark example's decode buffer from 3x to 8x so it satisfies the corrected guard (a 3x buffer would now be rejected).
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.
What
The FSST decoder needs its output buffer to be at least
8xthe compressed input, but the size guard only required3xand the publicdecompressdoc claimed "at least 3 times". This raises the guard to8x(with 32-bit overflow safety), fixes the docs, documents theunsafeinvariants, and aligns the in-tree example.Why
decompress_bulkwrites a full 8-byte word per code but advances the output cursor by only the symbol length, relying on a later write (or spare capacity) to overwrite the slack — the standard FSST decode trick:MAX_SYMBOL_LENGTHis 8, so a 1-byte code expands to at most 8 bytes and the whole decoded output is at most8xthe input. The last code of a run is the tightest case: it has no following write, so its 8-byte store must still land inside the buffer. Both facts require the buffer to be at least8x.The guard in
FsstDecoder::initwas:3xis too weak: it accepts a buffer between3xand8xthat the decode loop then writes out of bounds. This is not reachable from the in-tree decompressors (FsstPerValueDecompressor,FsstMiniBlockDecompressor, and the v2.0FsstPageDecoder) — they already allocatein_buf.len() * 8, so the3xcheck never fires for them. But the crate's ownbenchmarkexample allocated3xand would now be rejected, and the guard was unsound for any future caller that trusted the "3 times" wording.Changes
* 3to* 8, usingchecked_mul(8)so a>= 512 MiBinput can't wraplen * 8on a 32-bit target and bypass the check (overflow is treated as "too small"). This is a threshold comparison, not an allocation — no extra memory: existing8xcallers still pass, and a<8xbuffer is now rejected instead of silently overflowing.decompresspub-doc to8x, with the 8-byte-write reason.// SAFETY:comments to the unaligned load/store blocks infsst_unaligned_load_uncheckedanddecompress_bulk, per the repo convention of documenting everyunsafeblock. The comments honestly disclose which conditions are proven (loop guards, the8xoutput-buffer check, the symbol-table size check) versus which are trusted preconditions for well-formed input (lens[code] <= 8and offset values are loaded verbatim from the symbol table / offsets and are not re-validated on decode). Hardening the decoder against corrupt on-disk structures is a separate concern, out of scope here.benchmarkexample's decode buffer from3xto8xso it satisfies the corrected guard.test_decompress_rejects_undersized_output_bufferasserting a 1-byte-short buffer is rejected and an exact-8xbuffer succeeds (fails against the old3xguard).Test plan
cargo test -p fsst— 4/4 pass (incl. new test).cargo test -p lance-encoding fsst— 12/12 pass (all consumers).cargo fmt -p fsst --check— clean.cargo clippy -p fsst --tests --examples -- -D warnings— clean.