Skip to content

Fix CouponList deserialization count validation bug#503

Merged
leerho merged 2 commits into
apache:masterfrom
jihuayu:codex/couponlist-deserialize-bugfix
Jun 5, 2026
Merged

Fix CouponList deserialization count validation bug#503
leerho merged 2 commits into
apache:masterfrom
jihuayu:codex/couponlist-deserialize-bugfix

Conversation

@jihuayu
Copy link
Copy Markdown
Member

@jihuayu jihuayu commented Jun 5, 2026

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

Copy link
Copy Markdown
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for your contribution!

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens CouponList (LIST-mode) deserialization by validating the declared coupon count against the fixed LIST backing-array capacity, preventing malformed serialized data from causing unsafe reads/writes during deserialization. It also adds regression tests covering both byte-array and stream deserialization entry points.

Changes:

  • Add LIST coupon-count capacity validation in CouponList::newList() for both byte-array and stream paths.
  • Add new unit tests that mutate serialized bytes/streams to declare an oversized LIST coupon count and assert std::invalid_argument.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
hll/test/CouponListTest.cpp Adds regression tests for malformed LIST coupon-count handling in bytes and stream deserialization.
hll/include/CouponList-internal.hpp Adds LIST coupon-count validation during deserialization (bytes + stream).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread hll/include/CouponList-internal.hpp Outdated
Comment thread hll/include/CouponList-internal.hpp Outdated
@jihuayu jihuayu force-pushed the codex/couponlist-deserialize-bugfix branch from 8213aac to c10bc43 Compare June 5, 2026 12:56
@jihuayu jihuayu force-pushed the codex/couponlist-deserialize-bugfix branch from 0a8efef to 805eaf3 Compare June 5, 2026 13:16
@jihuayu jihuayu requested review from Copilot and tisonkun June 5, 2026 13:18
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

Comment thread hll/include/CouponList-internal.hpp
Comment thread hll/include/CouponList-internal.hpp
Comment thread hll/test/CouponListTest.cpp
Comment thread hll/test/CouponListTest.cpp
@jihuayu
Copy link
Copy Markdown
Member Author

jihuayu commented Jun 5, 2026

Hi @tisonkun. Following Copilot's suggestion, I reject cases where couponCount >= listCapacity, and updated the test cases to cover this scenario.

@leerho leerho merged commit 75934fa into apache:master Jun 5, 2026
17 checks passed
@leerho
Copy link
Copy Markdown
Member

leerho commented Jun 5, 2026

@jihuayu , @tisonkun
Thank you both for helping me close this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants