Skip to content

Commit a42ce85

Browse files
committed
fix: validate declared bucket count in TreemapReader
TreemapReader::new previously read only the 8-byte bucket count and trusted the declared size, so a corrupt buffer that declares N buckets but carries fewer (or none) was silently treated as valid. This diverged from deserialize_bitmap, which will rejects it. A lazy check in TreeMapIter::next cannot close this gap, because early-return callers such as bitmap_min stop calling next before reaching the missing trailing Walk all `size` bucket headers in TreemapReader::new so that truncation is rejected up front. The cost paid to keep consistent with deserialize_bitmap on corrupt buffers.
1 parent 70fd795 commit a42ce85

1 file changed

Lines changed: 50 additions & 0 deletions

File tree

src/common/io/src/bitmap/reader.rs

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,31 @@ impl<'a> TreemapReader<'a> {
4747
.read_u64::<LittleEndian>()
4848
.map_err(|_| Error::other("fail to read size"))?;
4949

50+
// Validate that `size` buckets fit in `buf` before returning.
51+
//
52+
// Without this check, the fast path diverges from `deserialize_bitmap`
53+
// on corrupt input. Consider `bitmap_has_all(lhs, corrupt_rhs)` where
54+
// `corrupt_rhs` declares buckets but carries no bucket data. The fast
55+
// path would treat `corrupt_rhs` as empty and return `true`, while
56+
// `deserialize_bitmap(corrupt_rhs)` rejects the same buffer as
57+
// BadBytes.
58+
//
59+
// A lazy check in `TreeMapIter::next` cannot close this gap. It fires
60+
// only when `next` is called. Early-return callers such as
61+
// `bitmap_min` (stops at the first non-empty container) stop calling
62+
// `next` before they ever reach a missing trailing bucket. The check
63+
// must run before iteration begins.
64+
//
65+
// `BitmapReader::decode` reads only the cookie, the last container
66+
// description, and the last offset, then seeks over container bodies.
67+
// This cost is paid to keep consistent with `deserialize_bitmap` on
68+
// corrupted buffers.
69+
let mut probe = buf;
70+
for _ in 0..size {
71+
let header = BitmapReader::decode(probe)?;
72+
probe = &probe[header.buf.len()..];
73+
}
74+
5075
Ok(Self { buf, _size: size })
5176
}
5277

@@ -992,6 +1017,31 @@ mod tests {
9921017
Ok(())
9931018
}
9941019

1020+
// A declared bucket count with no bucket data must be rejected up front.
1021+
#[test]
1022+
fn test_treemap_reader_rejects_truncated_buf() -> io::Result<()> {
1023+
let corrupt = 1u64.to_le_bytes();
1024+
assert!(TreemapReader::new(&corrupt).is_err());
1025+
Ok(())
1026+
}
1027+
1028+
// Partial truncation: the first bucket is intact and non-empty, but the
1029+
// declared count claims more buckets than exist. Eager validation in
1030+
// `TreemapReader::new` catches this before any caller can early-return.
1031+
#[test]
1032+
fn test_treemap_reader_rejects_partial_truncated_buf() -> io::Result<()> {
1033+
let mut tree = RoaringTreemap::new();
1034+
tree.insert(7);
1035+
let mut buf = Vec::new();
1036+
tree.serialize_into(&mut buf)?;
1037+
1038+
// Overwrite the leading u64 to claim 2 buckets while only 1 exists.
1039+
buf[..8].copy_from_slice(&2u64.to_le_bytes());
1040+
1041+
assert!(TreemapReader::new(&buf).is_err());
1042+
Ok(())
1043+
}
1044+
9951045
#[test]
9961046
fn test_intersection() -> io::Result<()> {
9971047
let v1 = create_bitmap(123);

0 commit comments

Comments
 (0)