You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by "jihuayu (via GitHub)" <gi...@apache.org> on 2026/06/05 11:32:25 UTC
[PR] Fix CouponList deserialization count validation bug [datasketches-cpp]
jihuayu opened a new pull request, #503:
URL: https://github.com/apache/datasketches-cpp/pull/503
This fixes a CouponList deserialization bug where LIST-mode serialized data could declare a coupon count larger than the fixed LIST capacity.
The change validates the declared LIST count before reading coupon data, and applies the check to both byte-array and stream deserialization paths.
## Tests
- `cmake --build build/Release --target hll_test`
- `./build/Release/hll/test/hll_test`
- `ctest --test-dir build/Release -R hll_test --output-on-failure`
- `ctest --test-dir build/ASan -R hll_test --output-on-failure`
cc @tisonkun
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] Fix CouponList deserialization count validation bug [datasketches-cpp]
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on PR #503:
URL: https://github.com/apache/datasketches-cpp/pull/503#issuecomment-4633448522
@jihuayu , @tisonkun
Thank you both for helping me close this!
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] Fix CouponList deserialization count validation bug [datasketches-cpp]
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho merged PR #503:
URL: https://github.com/apache/datasketches-cpp/pull/503
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] Fix CouponList deserialization count validation bug [datasketches-cpp]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on code in PR #503:
URL: https://github.com/apache/datasketches-cpp/pull/503#discussion_r3362913550
##########
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 at or above 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:
I think this repetition makes the code easier to read, and extracting it into a function is unnecessary.
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] Fix CouponList deserialization count validation bug [datasketches-cpp]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on PR #503:
URL: https://github.com/apache/datasketches-cpp/pull/503#issuecomment-4631911635
Hi @tisonkun. Following Copilot's suggestion, I reject cases where `couponCount >= listCapacity`, and updated the test cases to cover this scenario.
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] Fix CouponList deserialization count validation bug [datasketches-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #503:
URL: https://github.com/apache/datasketches-cpp/pull/503#discussion_r3362891086
##########
hll/test/CouponListTest.cpp:
##########
@@ -180,4 +181,67 @@ TEST_CASE("coupon list: check corrupt stream data", "[coupon_list]") {
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
}
+TEST_CASE("coupon list: rejects malformed coupon count in bytes", "[coupon_list]") {
+ // Reject declared LIST counts at or beyond the fixed LIST capacity.
+ uint8_t lgK = 8;
+ hll_sketch sk1(lgK);
+ sk1.update(1);
+ sk1.update(2);
+ const auto validBytes = sk1.serialize_compact();
+
+ const uint8_t listCapacity = static_cast<uint8_t>(1u << hll_constants::LG_INIT_LIST_SIZE);
+ const uint8_t malformedCount = listCapacity;
Review Comment:
Casting the computed LIST capacity to `uint8_t` can silently truncate if `LG_INIT_LIST_SIZE` ever changes such that the capacity exceeds 255, making this test potentially ineffective/incorrect. Prefer keeping `listCapacity`/`malformedCount` as `uint32_t` (or `size_t`) and only cast to `uint8_t` at the point where you write into the serialized count byte.
##########
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 at or above 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 same validation logic and error message are duplicated in both `newList` overloads. Consider extracting a small shared helper (e.g., `validate_coupon_count_or_throw(couponCount)`) or a shared function to format the error message once, so future changes don’t risk the two code paths diverging.
##########
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 at or above 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 same validation logic and error message are duplicated in both `newList` overloads. Consider extracting a small shared helper (e.g., `validate_coupon_count_or_throw(couponCount)`) or a shared function to format the error message once, so future changes don’t risk the two code paths diverging.
##########
hll/test/CouponListTest.cpp:
##########
@@ -180,4 +181,67 @@ TEST_CASE("coupon list: check corrupt stream data", "[coupon_list]") {
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
}
+TEST_CASE("coupon list: rejects malformed coupon count in bytes", "[coupon_list]") {
+ // Reject declared LIST counts at or beyond the fixed LIST capacity.
+ uint8_t lgK = 8;
+ hll_sketch sk1(lgK);
+ sk1.update(1);
+ sk1.update(2);
+ const auto validBytes = sk1.serialize_compact();
+
+ const uint8_t listCapacity = static_cast<uint8_t>(1u << hll_constants::LG_INIT_LIST_SIZE);
+ const uint8_t malformedCount = listCapacity;
+ auto sketchBytes = validBytes;
+ // Keep the input long enough to validate the declared LIST count.
+ const size_t requiredLen = hll_constants::LIST_INT_ARR_START
+ + malformedCount * sizeof(uint32_t);
+ if (sketchBytes.size() < requiredLen) {
+ sketchBytes.resize(requiredLen, 0);
+ }
+ sketchBytes[hll_constants::LIST_COUNT_BYTE] = malformedCount;
+
+ REQUIRE_THROWS_AS(
+ hll_sketch::deserialize(sketchBytes.data(), sketchBytes.size()),
+ std::invalid_argument);
+ REQUIRE_THROWS_AS(
+ CouponList<std::allocator<uint8_t>>::newList(
+ sketchBytes.data(), sketchBytes.size(), std::allocator<uint8_t>()),
+ std::invalid_argument);
+}
+
+TEST_CASE("coupon list: rejects malformed coupon count in stream", "[coupon_list]") {
+ uint8_t lgK = 8;
+ hll_sketch sk1(lgK);
+ sk1.update(1);
+ sk1.update(2);
+
+ const uint8_t listCapacity = static_cast<uint8_t>(1u << hll_constants::LG_INIT_LIST_SIZE);
+ const uint8_t malformedCount = listCapacity;
+ std::stringstream ss(std::ios::in | std::ios::out | std::ios::binary);
+ sk1.serialize_compact(ss);
+
+ const size_t requiredLen = hll_constants::LIST_INT_ARR_START
+ + malformedCount * sizeof(uint32_t);
+ // Keep the stream long enough to validate the declared LIST count.
+ ss.seekp(0, std::ios::end);
+ const auto endPos = static_cast<size_t>(ss.tellp());
+ if (endPos < requiredLen) {
+ std::vector<char> pad(requiredLen - endPos, 0);
+ ss.write(pad.data(), pad.size());
Review Comment:
`std::stringstream::write` takes a `std::streamsize` count, but `pad.size()` is `size_t`; with stricter warning levels this can trigger sign-conversion warnings. Consider casting `pad.size()` to `std::streamsize` for warning-free builds.
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] Fix CouponList deserialization count validation bug [datasketches-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org