From 35ae344f168aeb5d37f25f747ec323fc218ef5ab Mon Sep 17 00:00:00 2001 From: tison Date: Mon, 31 Aug 2026 18:20:17 +0800 Subject: [PATCH 1/7] refactor(req): borrow items from the public iterator Returning owned items cloned every retained value while the iterator was documented as zero-allocation. Borrow the items from the sketch so callers can inspect heap-backed values without hidden cloning or allocation. --- datasketches/src/req/iter.rs | 8 ++++---- tests-integration/tests/req_test/structure.rs | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/datasketches/src/req/iter.rs b/datasketches/src/req/iter.rs index ba5c0f3..b36c2f4 100644 --- a/datasketches/src/req/iter.rs +++ b/datasketches/src/req/iter.rs @@ -24,7 +24,7 @@ use crate::req::compactor::Compactor; /// Provides access to all items in the sketch along with their weights, /// which depend on the level of the compactor they're stored in. /// -/// Zero-allocation implementation that works directly with slices. +/// Items are borrowed from the sketch, so iteration does not clone or allocate. pub struct ReqSketchIterator<'a, T> { compactors: &'a [Compactor], current_level: usize, @@ -64,14 +64,14 @@ impl<'a, T: Clone + Ord> ReqSketchIterator<'a, T> { } } -impl Iterator for ReqSketchIterator<'_, T> { - type Item = (T, u64); +impl<'a, T: Clone + Ord> Iterator for ReqSketchIterator<'a, T> { + type Item = (&'a T, u64); fn next(&mut self) -> Option { loop { if let Some(ref mut level_iter) = self.current_level_iter { if let Some(item) = level_iter.next() { - return Some((item.clone(), self.current_weight)); + return Some((item, self.current_weight)); } } diff --git a/tests-integration/tests/req_test/structure.rs b/tests-integration/tests/req_test/structure.rs index e14cd79..8eff732 100644 --- a/tests-integration/tests/req_test/structure.rs +++ b/tests-integration/tests/req_test/structure.rs @@ -41,8 +41,8 @@ fn iterator_weights_sum_to_n_and_items_are_in_range() { for (item, weight) in sketch.iter() { assert_that!(weight, ge(1)); - assert_that!(item, ge(*sketch.min_item().expect("non-empty sketch"))); - assert_that!(item, le(*sketch.max_item().expect("non-empty sketch"))); + assert_that!(*item, ge(*sketch.min_item().expect("non-empty sketch"))); + assert_that!(*item, le(*sketch.max_item().expect("non-empty sketch"))); } } @@ -54,7 +54,7 @@ fn small_sketch_iterator_reports_unit_weights() { sketch.update(req_f64(i as f64)); } - let items: Vec<(ReqF64, u64)> = sketch.iter().collect(); + let items: Vec<(&ReqF64, u64)> = sketch.iter().collect(); assert_eq!(items.len(), 10); let weights: Vec<_> = items.iter().map(|&(_, weight)| weight).collect(); assert_that!(weights, each(eq(&1))); From fb4363437fb5befdc476ad9a21ad2146368029ed Mon Sep 17 00:00:00 2001 From: tison Date: Mon, 31 Aug 2026 18:21:34 +0800 Subject: [PATCH 2/7] fix(frequencies): validate string lengths before allocation String lengths are read from untrusted sketch images. Check the declared length against the remaining payload before allocating so a four-byte length field cannot trigger an allocation much larger than the supplied input. --- CHANGELOG.md | 1 + datasketches/src/frequencies/serialization.rs | 11 +++++++++-- tests-integration/tests/serde_tests/frequencies.rs | 10 ++++++++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f1f1bc..c8a9269 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ All significant changes to this project will be documented in this file. * T-Digest CDF and PMF queries now accept an empty split-point slice and return the single all-values bin instead of panicking. * Bloom filter deserialization now rejects malformed images with inconsistent counts or payload lengths, while valid images with a dirty cached count are restored correctly. * `FrequentItemsSketch` now enforces the cross-language map-size limit of `2^30` consistently. Oversized construction returns `InvalidArgument`, and malformed or oversized serialized images return `InvalidData` instead of panicking or attempting excessive allocation. +* `FrequentItemsSketch` now rejects an encoded string length that exceeds the remaining input before allocating the string buffer. * T-Digest compression now supports `k = u16::MAX` without overflowing. * T-Digest rejects truncated serialized payloads before allocating, and updating a deserialized digest no longer allows its buffered state to grow without bound. * Compact HLL4 images now restore all register values correctly. diff --git a/datasketches/src/frequencies/serialization.rs b/datasketches/src/frequencies/serialization.rs index f1f8f0c..5ef3bf2 100644 --- a/datasketches/src/frequencies/serialization.rs +++ b/datasketches/src/frequencies/serialization.rs @@ -56,9 +56,16 @@ impl FrequentItemValue for String { fn deserialize_value(cursor: &mut SketchSlice<'_>) -> Result { let len = cursor.read_u32_le().map_err(|_| { Error::insufficient_data("failed to read string item length".to_string()) - })?; + })? as usize; + + let remaining = cursor.remaining().len(); + if len > remaining { + return Err(Error::insufficient_data(format!( + "string item length ({len}) exceeds the remaining {remaining} bytes" + ))); + } - let mut slice = vec![0; len as usize]; + let mut slice = vec![0; len]; cursor.read_exact(&mut slice).map_err(|_| { Error::insufficient_data("failed to read string item bytes".to_string()) })?; diff --git a/tests-integration/tests/serde_tests/frequencies.rs b/tests-integration/tests/serde_tests/frequencies.rs index 6801b57..71bd62c 100644 --- a/tests-integration/tests/serde_tests/frequencies.rs +++ b/tests-integration/tests/serde_tests/frequencies.rs @@ -78,6 +78,16 @@ fn test_items_round_trip() { assert_eq!(restored.maximum_error(), sketch.maximum_error()); } +#[test] +fn test_string_deserialize_rejects_length_larger_than_input() { + let bytes = 1024u32.to_le_bytes(); + let mut cursor = SketchSlice::new(&bytes); + + let error = String::deserialize_value(&mut cursor).unwrap_err(); + assert_eq!(error.kind(), ErrorKind::InvalidData); + assert_that!(error.message(), contains_substring("exceeds the remaining")); +} + #[test] fn test_non_clone_item_round_trip() { let mut sketch = FrequentItemsSketch::::new(32).unwrap(); From e73bb2a2ba3e51843e229e4de38e2231c05ced6c Mon Sep 17 00:00:00 2001 From: tison Date: Mon, 31 Aug 2026 18:22:40 +0800 Subject: [PATCH 3/7] fix(req): validate serialized extrema against retained items REQ estimation images store stream extrema separately from retained items. Reject images whose extrema do not bound those items so min_item and max_item cannot expose values contradicted by the sketch state. --- CHANGELOG.md | 1 + datasketches/src/req/sketch.rs | 14 +++++++++++++- tests-integration/tests/serde_tests/req.rs | 5 +++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c8a9269..7249fad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ All significant changes to this project will be documented in this file. * Bloom filter deserialization now rejects malformed images with inconsistent counts or payload lengths, while valid images with a dirty cached count are restored correctly. * `FrequentItemsSketch` now enforces the cross-language map-size limit of `2^30` consistently. Oversized construction returns `InvalidArgument`, and malformed or oversized serialized images return `InvalidData` instead of panicking or attempting excessive allocation. * `FrequentItemsSketch` now rejects an encoded string length that exceeds the remaining input before allocating the string buffer. +* REQ deserialization now rejects serialized extrema that do not bound every retained item. * T-Digest compression now supports `k = u16::MAX` without overflowing. * T-Digest rejects truncated serialized payloads before allocating, and updating a deserialized digest no longer allows its buffered state to grow without bound. * Compact HLL4 images now restore all register values correctly. diff --git a/datasketches/src/req/sketch.rs b/datasketches/src/req/sketch.rs index af93c6c..a1f8b20 100644 --- a/datasketches/src/req/sketch.rs +++ b/datasketches/src/req/sketch.rs @@ -689,9 +689,21 @@ where } } - if n == 0 || min_item.is_none() || max_item.is_none() { + if n == 0 { return Err(Error::deserial("non-empty REQ sketch contains no items")); } + let (Some(min), Some(max)) = (&min_item, &max_item) else { + return Err(Error::deserial("non-empty REQ sketch contains no items")); + }; + if compactors + .iter() + .flat_map(Compactor::iter) + .any(|item| item < min || item > max) + { + return Err(Error::deserial( + "REQ retained item falls outside the min/max range", + )); + } let expected_raw_items = num_levels == 1 && n <= RAW_ITEMS_THRESHOLD; if raw_items != expected_raw_items { diff --git a/tests-integration/tests/serde_tests/req.rs b/tests-integration/tests/serde_tests/req.rs index 0138664..b097ed0 100644 --- a/tests-integration/tests/serde_tests/req.rs +++ b/tests-integration/tests/serde_tests/req.rs @@ -462,6 +462,11 @@ fn deserialize_rejects_invalid_extrema_and_raw_nan() { reversed[20..24].copy_from_slice(&1.0f32.to_le_bytes()); assert_invalid_data(&reversed); + let mut extrema_exclude_retained_items = estimation_image(12, 1_000); + extrema_exclude_retained_items[16..20].copy_from_slice(&500.0f32.to_le_bytes()); + extrema_exclude_retained_items[20..24].copy_from_slice(&500.0f32.to_le_bytes()); + assert_invalid_data(&extrema_exclude_retained_items); + let mut raw_nan = vec![2u8, 1, 17, 8 | 16, 12, 0, 1, 1]; raw_nan.extend_from_slice(&f32::NAN.to_le_bytes()); assert_invalid_data(&raw_nan); From 456e89c44756ccb90df6fb6857bd536b8b05b250 Mon Sep 17 00:00:00 2001 From: tison Date: Mon, 31 Aug 2026 18:25:05 +0800 Subject: [PATCH 4/7] docs(tdigest): state mutable query panic contracts The mutable query methods perform the same input assertions as TDigest, but their own API docs omitted those contracts. Document them at the call sites so users do not need to follow cross-links to discover when a query can panic. --- datasketches/src/tdigest/sketch.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/datasketches/src/tdigest/sketch.rs b/datasketches/src/tdigest/sketch.rs index b70ce51..f55c426 100644 --- a/datasketches/src/tdigest/sketch.rs +++ b/datasketches/src/tdigest/sketch.rs @@ -384,6 +384,11 @@ impl TDigestMut { /// Returns the cumulative distribution approximation described by [`TDigest::cdf`]. /// + /// # Panics + /// + /// Panics if `split_points` is not unique, not monotonically increasing, or contains `NaN` + /// values. + /// /// # Examples /// /// ``` @@ -408,6 +413,11 @@ impl TDigestMut { /// Returns the probability mass approximation described by [`TDigest::pmf`]. /// + /// # Panics + /// + /// Panics if `split_points` is not unique, not monotonically increasing, or contains `NaN` + /// values. + /// /// # Examples /// /// ``` @@ -432,6 +442,10 @@ impl TDigestMut { /// Returns the normalized rank described by [`TDigest::rank`]. /// + /// # Panics + /// + /// Panics if `value` is `NaN`. + /// /// # Examples /// /// ``` @@ -466,6 +480,10 @@ impl TDigestMut { /// Returns the quantile described by [`TDigest::quantile`]. /// + /// # Panics + /// + /// Panics if `rank` is outside `[0.0, 1.0]`. + /// /// # Examples /// /// ``` From 430ff960c523e9154c31fb319f2815f6316373eb Mon Sep 17 00:00:00 2001 From: tison Date: Mon, 31 Aug 2026 18:26:06 +0800 Subject: [PATCH 5/7] refactor(req): type the confidence level parameter REQ confidence bounds support only one, two, or three standard deviations, but a u8 accepted every value and differed from the rest of the crate. Reuse NumStdDev so invalid confidence levels are unrepresentable at the call site. --- CHANGELOG.md | 2 +- datasketches/src/common/num_std_dev.rs | 6 ++--- datasketches/src/req/sketch.rs | 13 ++++++----- tests-integration/tests/req_test/bounds.rs | 24 +++++++++++--------- tests-integration/tests/req_test/property.rs | 5 ++-- 5 files changed, 27 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7249fad..42d9eef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,7 @@ All significant changes to this project will be documented in this file. ### New features * `TDigest` can now be serialized and deserialized directly without converting through `TDigestMut` at the call site. -* Add Relative Error Quantiles (REQ) sketches behind the `req` feature, including configurable high- or low-rank accuracy, rank, quantile, PMF, and CDF queries, merging, totally ordered custom item types, the `ReqFloat` adapter for non-NaN floating-point values, and C++/Java-compatible serialization. +* Add Relative Error Quantiles (REQ) sketches behind the `req` feature, including configurable high- or low-rank accuracy, rank, quantile, PMF, and CDF queries, typed rank confidence bounds, merging, totally ordered custom item types, the `ReqFloat` adapter for non-NaN floating-point values, and C++/Java-compatible serialization. ### Performance improvements diff --git a/datasketches/src/common/num_std_dev.rs b/datasketches/src/common/num_std_dev.rs index 2aa908f..99ef21f 100644 --- a/datasketches/src/common/num_std_dev.rs +++ b/datasketches/src/common/num_std_dev.rs @@ -31,9 +31,9 @@ static DELTA_OF_NUM_STD_DEVS: [f64; 4] = [ /// Number of standard deviations for confidence bounds. /// /// This enum specifies the number of standard deviations to use when computing -/// upper and lower bounds for cardinality estimates. Higher values provide wider -/// confidence intervals with greater certainty that the true cardinality falls -/// within the bounds. +/// upper and lower bounds for sketch estimates. Higher values provide wider +/// confidence intervals with greater certainty that the true value falls within +/// the bounds. #[repr(u8)] #[derive(Debug, Clone, Copy, PartialEq)] pub enum NumStdDev { diff --git a/datasketches/src/req/sketch.rs b/datasketches/src/req/sketch.rs index a1f8b20..5826ce3 100644 --- a/datasketches/src/req/sketch.rs +++ b/datasketches/src/req/sketch.rs @@ -21,6 +21,7 @@ use crate::codec::SketchBytes; use crate::codec::SketchSlice; use crate::codec::assert::insufficient_data; use crate::codec::family::Family; +use crate::common::NumStdDev; use crate::error::Error; use crate::req::DEFAULT_K; use crate::req::INITIAL_SECTIONS_PER_COMPACTOR; @@ -351,25 +352,25 @@ where Ok(()) } - /// Returns the lower bound for the rank of a quantile at `num_std_dev` confidence. - pub fn rank_lower_bound(&self, rank: f64, num_std_dev: u8) -> f64 { + /// Returns the lower bound for the normalized `rank` at the requested confidence level. + pub fn rank_lower_bound(&self, rank: f64, num_std_dev: NumStdDev) -> f64 { self.compute_rank_lower_bound( self.k, self.compactors.len() as u8, rank, - num_std_dev, + num_std_dev.as_u8(), self.n, matches!(self.rank_accuracy, RankAccuracy::HighRank), ) } - /// Returns the upper bound for the rank of a quantile at `num_std_dev` confidence. - pub fn rank_upper_bound(&self, rank: f64, num_std_dev: u8) -> f64 { + /// Returns the upper bound for the normalized `rank` at the requested confidence level. + pub fn rank_upper_bound(&self, rank: f64, num_std_dev: NumStdDev) -> f64 { self.compute_rank_upper_bound( self.k, self.compactors.len() as u8, rank, - num_std_dev, + num_std_dev.as_u8(), self.n, matches!(self.rank_accuracy, RankAccuracy::HighRank), ) diff --git a/tests-integration/tests/req_test/bounds.rs b/tests-integration/tests/req_test/bounds.rs index b89b743..5caab94 100644 --- a/tests-integration/tests/req_test/bounds.rs +++ b/tests-integration/tests/req_test/bounds.rs @@ -17,6 +17,7 @@ //! Rank error bounds and sigma coverage for ReqSketch. +use datasketches::common::NumStdDev; use datasketches::error::Error; use datasketches::req::RankAccuracy; use datasketches::req::ReqSketch; @@ -39,7 +40,8 @@ fn bounds_are_nested_and_in_unit_interval() { } for rank in [0.01, 0.05, 0.1, 0.25, 0.5, 0.75, 0.9, 0.95, 0.99, 0.999] { - let bounds: Vec<(f64, f64)> = (1..=3u8) + let bounds: Vec<(f64, f64)> = [NumStdDev::One, NumStdDev::Two, NumStdDev::Three] + .into_iter() .map(|sigma| { ( sketch.rank_lower_bound(rank, sigma), @@ -76,8 +78,8 @@ fn theoretical_error_bounds_cover_uniform_quantiles() -> Result<(), Error> { ] { let true_quantile = req_f64(rank * (n - 1) as f64); let estimated_rank = sketch.rank(&true_quantile, SearchCriteria::Inclusive)?; - let lower = sketch.rank_lower_bound(rank, 3); - let upper = sketch.rank_upper_bound(rank, 3); + let lower = sketch.rank_lower_bound(rank, NumStdDev::Three); + let upper = sketch.rank_upper_bound(rank, NumStdDev::Three); assert_that!(estimated_rank, all!(ge(lower), le(upper)), "rank: {rank}"); } @@ -95,10 +97,10 @@ fn hra_and_lra_bounds_are_tighter_at_their_target_end() -> Result<(), Error> { lra.update(req_f64(i as f64)); } - let hra_error = - (rank - hra.rank_lower_bound(rank, 2)).max(hra.rank_upper_bound(rank, 2) - rank); - let lra_error = - (rank - lra.rank_lower_bound(rank, 2)).max(lra.rank_upper_bound(rank, 2) - rank); + let hra_error = (rank - hra.rank_lower_bound(rank, NumStdDev::Two)) + .max(hra.rank_upper_bound(rank, NumStdDev::Two) - rank); + let lra_error = (rank - lra.rank_lower_bound(rank, NumStdDev::Two)) + .max(lra.rank_upper_bound(rank, NumStdDev::Two) - rank); if rank >= 0.75 { assert_that!(hra_error, le(lra_error)); @@ -121,8 +123,8 @@ fn exact_mode_bounds_are_tight() { assert!(!sketch.is_estimation_mode()); for rank in [0.1, 0.25, 0.5, 0.75, 0.9] { - let lower = sketch.rank_lower_bound(rank, 2); - let upper = sketch.rank_upper_bound(rank, 2); + let lower = sketch.rank_lower_bound(rank, NumStdDev::Two); + let upper = sketch.rank_upper_bound(rank, NumStdDev::Two); assert_that!((upper - lower) / 2.0, lt(0.05)); } } @@ -156,8 +158,8 @@ fn high_rank_accuracy_matches_tight_thresholds() { } for rank in [0.9, 0.99, 0.999] { - let lower = sketch.rank_lower_bound(rank, 3); - let upper = sketch.rank_upper_bound(rank, 3); + let lower = sketch.rank_lower_bound(rank, NumStdDev::Three); + let upper = sketch.rank_upper_bound(rank, NumStdDev::Three); assert_that!(rank, all!(ge(lower), le(upper))); } } diff --git a/tests-integration/tests/req_test/property.rs b/tests-integration/tests/req_test/property.rs index e993c4d..9df8c2c 100644 --- a/tests-integration/tests/req_test/property.rs +++ b/tests-integration/tests/req_test/property.rs @@ -17,6 +17,7 @@ //! Property-based ReqSketch tests. +use datasketches::common::NumStdDev; use datasketches::req::ReqSketch; use datasketches::req::SearchCriteria; use quickcheck::Gen; @@ -56,8 +57,8 @@ fn prop_quantile_rank_consistency() { // interval for the target rank (plus a small cushion for snapping to a // stored item). This scales with k and n, unlike a fixed slack, so it // actually constrains the result instead of always passing. - let lower = sketch.rank_lower_bound(rank, 3) - 0.02; - let upper = sketch.rank_upper_bound(rank, 3) + 0.02; + let lower = sketch.rank_lower_bound(rank, NumStdDev::Three) - 0.02; + let upper = sketch.rank_upper_bound(rank, NumStdDev::Three) + 0.02; assert!( (lower..=upper).contains(&recovered), "rank {rank} -> quantile {quantile} -> recovered {recovered}, expected within [{lower:.4}, {upper:.4}]" From 97c74ed8e621b4d8ec05dc5c9de9542d5be649f3 Mon Sep 17 00:00:00 2001 From: tison Date: Mon, 31 Aug 2026 18:27:23 +0800 Subject: [PATCH 6/7] refactor(frequencies): remove impossible count assertions Update counts are unsigned and zero already returns early, so the following "not negative" assertions were always true. Remove the dead checks and their misleading signed-count error message. --- datasketches/src/frequencies/sketch.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/datasketches/src/frequencies/sketch.rs b/datasketches/src/frequencies/sketch.rs index 7a52045..f96bc90 100644 --- a/datasketches/src/frequencies/sketch.rs +++ b/datasketches/src/frequencies/sketch.rs @@ -365,7 +365,6 @@ impl FrequentItemsSketch { if count == 0 { return; } - assert!(count > 0, "count may not be negative"); self.stream_weight += count; self.hash_map.adjust_or_put_value(item, count); self.maybe_resize_or_purge(); @@ -418,7 +417,6 @@ impl FrequentItemsSketch { if count == 0 { return; } - assert!(count > 0, "count may not be negative"); self.stream_weight += count; self.hash_map.adjust_or_put_value_ref(item, count); self.maybe_resize_or_purge(); From 0d7194126afea1b128b45f017e0b8b70f3f1ed7c Mon Sep 17 00:00:00 2001 From: tison Date: Mon, 31 Aug 2026 20:04:29 +0800 Subject: [PATCH 7/7] docs(changelog): omit unreleased REQ hardening REQ is new since the latest release, so its deserialization fix is part of the initial 0.5.0 contract rather than a user-visible change between releases. Keep the changelog focused on the feature's final behavior. --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42d9eef..153a92f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,7 +39,6 @@ All significant changes to this project will be documented in this file. * Bloom filter deserialization now rejects malformed images with inconsistent counts or payload lengths, while valid images with a dirty cached count are restored correctly. * `FrequentItemsSketch` now enforces the cross-language map-size limit of `2^30` consistently. Oversized construction returns `InvalidArgument`, and malformed or oversized serialized images return `InvalidData` instead of panicking or attempting excessive allocation. * `FrequentItemsSketch` now rejects an encoded string length that exceeds the remaining input before allocating the string buffer. -* REQ deserialization now rejects serialized extrema that do not bound every retained item. * T-Digest compression now supports `k = u16::MAX` without overflowing. * T-Digest rejects truncated serialized payloads before allocating, and updating a deserialized digest no longer allows its buffered state to grow without bound. * Compact HLL4 images now restore all register values correctly.