Copilot commented on code in PR #503:
URL: https://github.com/apache/datasketches-cpp/pull/503#discussion_r3362574252
##########
hll/include/CouponList-internal.hpp:
##########
@@ -146,11 +153,19 @@ CouponList<A>* CouponList<A>::newList(std::istream& is,
const A& allocator) {
const bool oooFlag = ((listHeader[hll_constants::FLAGS_BYTE] &
hll_constants::OUT_OF_ORDER_FLAG_MASK) ? true : false);
const bool emptyFlag = ((listHeader[hll_constants::FLAGS_BYTE] &
hll_constants::EMPTY_FLAG_MASK) ? true : false);
+ const uint32_t couponCount = listHeader[hll_constants::LIST_COUNT_BYTE];
+ // Reject LIST counts larger than the fixed LIST capacity.
+ const uint32_t listCapacity = 1u << hll_constants::LG_INIT_LIST_SIZE;
+ if (couponCount > listCapacity) {
+ throw std::invalid_argument("Attempt to deserialize invalid CouponList
with couponCount > capacity. Found couponCount: "
+ + std::to_string(couponCount)
+ + ", capacity: " +
std::to_string(listCapacity));
+ }
Review Comment:
Same as the byte-array path: allowing `couponCount == listCapacity` can
deserialize an impossible LIST state that later cannot be updated safely (and
can lead to `couponCount_` exceeding the fixed array size if the payload
contains EMPTY entries). Reject `couponCount >= listCapacity` here as well.
##########
hll/include/CouponList-internal.hpp:
##########
@@ -99,6 +99,13 @@ CouponList<A>* CouponList<A>::newList(const void* bytes,
size_t len, const A& al
const bool emptyFlag = ((data[hll_constants::FLAGS_BYTE] &
hll_constants::EMPTY_FLAG_MASK) ? true : false);
const uint32_t couponCount = data[hll_constants::LIST_COUNT_BYTE];
+ // Reject LIST counts larger than the fixed LIST capacity.
+ const uint32_t listCapacity = 1u << hll_constants::LG_INIT_LIST_SIZE;
+ if (couponCount > listCapacity) {
+ throw std::invalid_argument("Attempt to deserialize invalid CouponList
with couponCount > capacity. Found couponCount: "
+ + std::to_string(couponCount)
+ + ", capacity: " +
std::to_string(listCapacity));
+ }
Review Comment:
The new LIST-count validation still allows `couponCount == listCapacity`
(8). A LIST sketch can never legitimately persist in that state:
`CouponList::couponUpdate` promotes as soon as `couponCount_ ==
coupons_.size()`, and if deserialization accepts a full list (or a list with
empties but count==capacity), subsequent `update()` can throw (`Array invalid:
no empties...`) or push `couponCount_` beyond the backing array size. Reject
`couponCount >= listCapacity` to preserve invariants and avoid later
crashes/incorrect estimates from malformed inputs.
--
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]
