Skip to content

Commit da16bd4

Browse files
committed
fix: align fast paths with deserialize_bitmap on corrupt input
The fast path silently diverged from deserialize_bitmap: 1. Truncation: declared N buckets but fewer present was silently accepted; TreemapReader::new now walks all `size` headers up front. 2. Trailing data: bytes beyond declared buckets were decoded as undeclared buckets; `buf` is now sliced to the consumed range. 3. Mixed Small/Large paths in bitmap_has_any/has_all skipped roaring-side validation when small was empty; the reader is now built once before the loop and reused via the new TreemapReader::contains method (replaces reader::bitmap_contains). Adds regression tests for all three cases.
1 parent 70fd795 commit da16bd4

2 files changed

Lines changed: 118 additions & 26 deletions

File tree

src/common/io/src/bitmap.rs

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -868,7 +868,7 @@ pub fn bitmap_contains(buf: &[u8], value: u64) -> Result<bool> {
868868
}
869869
validate_serialized_bitmap_header_and_length(buf)?;
870870
if let Some(roaring_buf) = as_roaring(buf) {
871-
Ok(reader::bitmap_contains(roaring_buf, value)?)
871+
Ok(reader::TreemapReader::new(roaring_buf)?.contains(value)?)
872872
} else {
873873
Ok(SmallReader::new(buf)?.contains(value))
874874
}
@@ -943,8 +943,9 @@ pub fn bitmap_has_all(lhs: &[u8], rhs: &[u8]) -> Result<bool> {
943943
// lhs is HybridLarge/Legacy, rhs is HybridSmall: probe
944944
let roaring_buf = as_roaring(lhs).unwrap();
945945
let rhs_small = SmallReader::new(rhs)?;
946+
let tree = reader::TreemapReader::new(roaring_buf)?;
946947
for i in 0..rhs_small.len() {
947-
if !reader::bitmap_contains(roaring_buf, rhs_small.values[i])? {
948+
if !tree.contains(rhs_small.values[i])? {
948949
return Ok(false);
949950
}
950951
}
@@ -977,8 +978,9 @@ pub fn bitmap_has_any(lhs: &[u8], rhs: &[u8]) -> Result<bool> {
977978
} else {
978979
(as_roaring(rhs).unwrap(), SmallReader::new(lhs)?)
979980
};
981+
let tree = reader::TreemapReader::new(roaring_buf)?;
980982
for i in 0..small.len() {
981-
if reader::bitmap_contains(roaring_buf, small.values[i])? {
983+
if tree.contains(small.values[i])? {
982984
return Ok(true);
983985
}
984986
}
@@ -1677,6 +1679,40 @@ mod tests {
16771679
assert_eq!(decoded.into_iter().collect::<Vec<_>>(), vec![1, 5, 42]);
16781680
}
16791681

1682+
// A corrupt large buffer paired with an empty HybridSmall must surface
1683+
// the decoding error, not silently return `false`/`true`. Without eager
1684+
// validation of the roaring side, the mixed-path loop runs zero times
1685+
// (small side is empty) and the corrupt large side slips through.
1686+
#[test]
1687+
fn has_any_corrupt_large_with_empty_small_is_rejected() {
1688+
let corrupt_large = 1u64.to_le_bytes(); // declares 1 bucket, no data
1689+
let empty_small = [
1690+
HYBRID_MAGIC[0],
1691+
HYBRID_MAGIC[1],
1692+
HYBRID_VERSION,
1693+
HYBRID_KIND_SMALL,
1694+
0, // 0 values
1695+
];
1696+
1697+
assert!(bitmap_has_any(&corrupt_large, &empty_small).is_err());
1698+
assert!(bitmap_has_any(&empty_small, &corrupt_large).is_err());
1699+
}
1700+
1701+
#[test]
1702+
fn has_all_corrupt_large_with_empty_small_is_rejected() {
1703+
let corrupt_large = 1u64.to_le_bytes();
1704+
let empty_small = [
1705+
HYBRID_MAGIC[0],
1706+
HYBRID_MAGIC[1],
1707+
HYBRID_VERSION,
1708+
HYBRID_KIND_SMALL,
1709+
0,
1710+
];
1711+
1712+
assert!(bitmap_has_all(&corrupt_large, &empty_small).is_err());
1713+
assert!(bitmap_has_all(&empty_small, &corrupt_large).is_err());
1714+
}
1715+
16801716
// Tests for minimum deserialize bitmap functions.
16811717
//
16821718
// Cover public functions by comparing against RoaringTreemap:

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

Lines changed: 79 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,17 @@ 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` and bound `buf` to the
51+
// declared range, so the fast path matches `deserialize_bitmap` on
52+
// corrupt input (truncation rejected, trailing data ignored).
53+
let mut probe = buf;
54+
for _ in 0..size {
55+
let header = BitmapReader::decode(probe)?;
56+
probe = &probe[header.buf.len()..];
57+
}
58+
let consumed = buf.len() - probe.len();
59+
let buf = &buf[..consumed];
60+
5061
Ok(Self { buf, _size: size })
5162
}
5263

@@ -56,6 +67,26 @@ impl<'a> TreemapReader<'a> {
5667
offset: 0,
5768
}
5869
}
70+
71+
pub fn contains(&self, value: u64) -> io::Result<bool> {
72+
let prefix = (value >> 32) as u32;
73+
let container_key = ((value >> 16) & 0xFFFF) as u16;
74+
let low16 = (value & 0xFFFF) as u16;
75+
76+
for bitmap_result in self.iter() {
77+
let bitmap = bitmap_result?;
78+
if bitmap.prefix() < prefix {
79+
continue;
80+
}
81+
if bitmap.prefix() > prefix {
82+
return Ok(false);
83+
}
84+
return Ok(bitmap
85+
.find_container(container_key)?
86+
.is_some_and(|c| c.contains(low16)));
87+
}
88+
Ok(false)
89+
}
5990
}
6091

6192
pub struct TreeMapIter<'a> {
@@ -853,27 +884,6 @@ pub(crate) fn bitmap_len_above(buf: &[u8], threshold: usize) -> io::Result<bool>
853884
Ok(false)
854885
}
855886

856-
pub(crate) fn bitmap_contains(buf: &[u8], value: u64) -> io::Result<bool> {
857-
let prefix = (value >> 32) as u32;
858-
let container_key = ((value >> 16) & 0xFFFF) as u16;
859-
let low16 = (value & 0xFFFF) as u16;
860-
861-
let tree = TreemapReader::new(buf)?;
862-
for bitmap_result in tree.iter() {
863-
let bitmap = bitmap_result?;
864-
if bitmap.prefix() < prefix {
865-
continue;
866-
}
867-
if bitmap.prefix() > prefix {
868-
return Ok(false);
869-
}
870-
return Ok(bitmap
871-
.find_container(container_key)?
872-
.is_some_and(|c| c.contains(low16)));
873-
}
874-
Ok(false)
875-
}
876-
877887
pub(crate) fn bitmap_min(buf: &[u8]) -> io::Result<Option<u64>> {
878888
let tree = TreemapReader::new(buf)?;
879889
for bitmap_result in tree.iter() {
@@ -992,6 +1002,51 @@ mod tests {
9921002
Ok(())
9931003
}
9941004

1005+
// A declared bucket count with no bucket data must be rejected up front.
1006+
#[test]
1007+
fn test_treemap_reader_rejects_truncated_buf() -> io::Result<()> {
1008+
let corrupt = 1u64.to_le_bytes();
1009+
assert!(TreemapReader::new(&corrupt).is_err());
1010+
Ok(())
1011+
}
1012+
1013+
// Partial truncation: the first bucket is intact and non-empty, but the
1014+
// declared count claims more buckets than exist. Eager validation in
1015+
// `TreemapReader::new` catches this before any caller can early-return.
1016+
#[test]
1017+
fn test_treemap_reader_rejects_partial_truncated_buf() -> io::Result<()> {
1018+
let mut tree = RoaringTreemap::new();
1019+
tree.insert(7);
1020+
let mut buf = Vec::new();
1021+
tree.serialize_into(&mut buf)?;
1022+
1023+
// Overwrite the leading u64 to claim 2 buckets while only 1 exists.
1024+
buf[..8].copy_from_slice(&2u64.to_le_bytes());
1025+
1026+
assert!(TreemapReader::new(&buf).is_err());
1027+
Ok(())
1028+
}
1029+
1030+
// Trailing data after the declared buckets must be ignored, matching
1031+
// `deserialize_bitmap`. A buffer that declares 0 buckets but carries a
1032+
// valid bucket trailer must decode as an empty treemap, not as one with
1033+
// an undeclared bucket.
1034+
#[test]
1035+
fn test_treemap_reader_ignores_trailing_data() -> io::Result<()> {
1036+
let mut tree = RoaringTreemap::new();
1037+
tree.insert(7);
1038+
let mut buf = Vec::new();
1039+
tree.serialize_into(&mut buf)?;
1040+
1041+
// Overwrite the leading u64 to claim 0 buckets; the rest is trailing.
1042+
buf[..8].copy_from_slice(&0u64.to_le_bytes());
1043+
1044+
let reader = TreemapReader::new(&buf)?;
1045+
let count = reader.iter().filter(|r| r.is_ok()).count();
1046+
assert_eq!(count, 0);
1047+
Ok(())
1048+
}
1049+
9951050
#[test]
9961051
fn test_intersection() -> io::Result<()> {
9971052
let v1 = create_bitmap(123);
@@ -1283,10 +1338,11 @@ mod tests {
12831338
#[test]
12841339
fn test_bitmap_contains() -> io::Result<()> {
12851340
for_each_fixture(|name, buf, tree, miss_value| {
1341+
let reader = TreemapReader::new(buf).unwrap();
12861342
// Test a known-present value (min if exists)
12871343
if let Some(hit) = tree.min() {
12881344
let expected = tree.contains(hit);
1289-
let actual = bitmap_contains(buf, hit).unwrap();
1345+
let actual = reader.contains(hit).unwrap();
12901346
assert_eq!(
12911347
actual, expected,
12921348
"bitmap_contains hit: fixture={name}, val={hit}"
@@ -1295,7 +1351,7 @@ mod tests {
12951351
// Test a known-absent value
12961352
let miss = miss_value;
12971353
let expected = tree.contains(miss);
1298-
let actual = bitmap_contains(buf, miss).unwrap();
1354+
let actual = reader.contains(miss).unwrap();
12991355
assert_eq!(
13001356
actual, expected,
13011357
"bitmap_contains miss: fixture={name}, val={miss}"

0 commit comments

Comments
 (0)