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: [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]
