jaideeppyne commented on code in PR #226:
URL: https://github.com/apache/datasketches-rust/pull/226#discussion_r3921791280
##########
datasketches/src/cpc/compression.rs:
##########
@@ -403,116 +404,154 @@ impl CompressedState {
let mut next_true_pair = 0;
for i in 0..self.table_num_entries {
let row_col = pairs[i as usize];
- assert_ne!(row_col, u32::MAX);
+ if row_col == u32::MAX {
+ return Err(Error::deserial("CPC hybrid table contains an
invalid pair"));
+ }
let col = row_col & 63;
Review Comment:
The successor #227 removes this hybrid-loop sentinel branch and validates
the sentinel in the low-level pair decoder, with a dedicated regression test.
Since #227 merged and superseded this PR, this specific finding is addressed
there.
##########
datasketches/src/cpc/sketch.rs:
##########
@@ -639,7 +639,54 @@ impl CpcSketch {
)));
}
- let uncompressed = compressed.uncompress(lg_k, num_coupons);
+ // The coupon space of a sketch has `k * 64` cells (`k` rows of 64
columns each), so a
+ // valid sketch can never report more coupons than that. Rejecting
larger values keeps the
+ // flavor arithmetic below from overflowing on corrupt input.
+ if (num_coupons as u64) > 64 * (1u64 << lg_k) {
+ return Err(Error::deserial(format!(
+ "num_coupons ({}) exceeds coupon space for lg_k = {}",
+ num_coupons, lg_k
+ )));
+ }
+
+ // A valid sketch stores a sliding window exactly for the pinned and
sliding flavors, and
+ // stores its coupons in the surprising-value table for the sparse and
hybrid flavors. The
+ // flavor is fully determined by `lg_k` and `num_coupons`, so the
flags must agree with it.
+ let flavor = determine_flavor(lg_k, num_coupons);
+ let window_expected = matches!(flavor, Flavor::Pinned |
Flavor::Sliding);
Review Comment:
The HIP part needs a distinction: `serialize()` sets `has_hip = !merge_flag`
even for an empty sketch, so rejecting that flag solely because `num_coupons ==
0` would reject valid output. The window/flavor check rejects an empty sketch
with a window. The zero-coupon/table-flag case is still permissive in the
successor code I inspected; I am not claiming that part is fixed. This PR was
superseded by #227, so I am leaving that residual finding visible rather than
reopening the old patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
