Repository navigation
[2026.10.7] btree: bound the compressed decoder's varint reads (over-read fix) - #265
Merged
Merged
Conversation
…er-read fix)
__bam_defdecompress (the default compressed-btree key/data decoder) read each
varint's leading byte -- via __db_decompress_count_int -- and the magic byte
*s at offset 0 BEFORE checking the compressed buffer was long enough:
s = compressed->data;
if (*s == CMP_INT_SPARE_VAL) ... (*s, no size check at all)
size = __db_decompress_count_int(s); (reads s[0])
if (size > compressed->size) return (EINVAL); (the check is AFTER the read)
A compressed btree's pages come from disk, so a corrupt or truncated
compressed item is a trust boundary (db_verify, a hostile file, DB->get over a
damaged page). A short item over-reads up to one byte past compressed->size at
each of the three varint sites, and a zero-length item reads *s with no check
at all -- a heap over-read on hostile input.
Fix: before every __db_decompress_count_int, require the current offset
(tracked in `size`, which matches s's displacement) to be strictly inside
compressed->size; and require compressed->size >= 1 before reading *s. Every
__db_decompress_int32 read was already covered by the following
size > compressed->size check, so only the lookahead and the magic byte needed
guarding.
Proved over every bounded input by test/cbmc/harness_bt_decompress.c
(-DB1_FIXED verifies; the unmodified code's over-read is the CBMC
counterexample). Regression test test/db/bt_decompress_bounds.c places a
tightly-sized compressed item flush against a PROT_NONE guard page, so the
over-read faults on ANY build, not only under AddressSanitizer: it FAILs on
master (SIGSEGV in both the size-0 and the truncated-varint case) and PASSes
with this fix (clean EINVAL). SKIPs on a build without --enable-compression,
since the decoder is not compiled in then. MANIFEST entry + executable mode
added.
No format, log-record, ABI or region change.
(cherry picked from commit 2e99a1f)
Contributor
Coccinelle convention checksNo new violations. ✅ Resolved since baseline (3) -- update dist/cocci/baseline.txt to lock these in. |
Contributor
ABI diff vs
|
…nt reads The previous commit stopped the decoder reading a varint past the end of the item, but the values it decodes are from disk too. With prefix 4 and suffix 0xFFFFFFFE, prefix + suffix wraps to 2, "size += suffix" wraps back under compressed->size, every check passes, and memcpy copies about 4 GB. That happens on both the normal and the duplicate (0xFC) path. __bamc_start_decompress had the same hole one level up: it decoded the first record's length and copied that many bytes out of the page with no check. A compressed btree with one corrupted length byte crashed a plain DB->get. Bound prefix against the previous key or data, suffix and the data length against what is left of the item, and prefix + suffix against UINT32_MAX, before any of them is summed. Check the first record's length the way __bam_compress_salvage already does. bt_decompress_bounds gains three cases: the wrap on each path (C, D), and a real compressed database with its first length corrupted on disk (E, public API only). On the dev box (Debian 12, --enable-compression --enable-diagnostic): master fails A-E with SIGSEGV; the previous commit alone fails C, D and E; this commit passes all five. (cherry picked from commit e067d65)
Collaborator
Author
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.
Backport of #264 (master
fix/bt-decompress-bounds, commit 2e99a1f) tolts/2026.10.7, cherry-picked with-x.What
__bam_defdecompressread each varint's leading byte — and the magic byte*sat offset 0 — before checking the compressed buffer was long enough, so a
corrupt/truncated compressed btree item (a page from disk) over-reads past its
end. See #264 for the full root cause.
Fix
Bound each varint offset strictly inside
compressed->sizebefore the read,and require
compressed->size >= 1before*s. No format, log-record, ABI orregion change.
LTS gates
0x841f4ea8byte-identical between the base(
lts/2026.10.7) and this backport — the fix touches onlybt_compress.clogic and test files, no struct/format change.
test/db/run_bt_decompress_bounds.shFAILs(
case Aandcase Bboth die on SIGSEGV via thePROT_NONEguard page);on this backport it PASSes (clean
EINVAL). Both built--enable-compression --enable-diagnostic.(cherry picked from commit 2e99a1f84…).The conflict was in
test/MANIFESTonly (the LTS line has no typed/dtrace/dd-stripes db entries); resolved by keeping the LTS structure and adding the
one
run_bt_decompress_boundsentry.