Bounds-check per-entry key read in compact_tuple_sketch::deserialize - #511
Merged
Conversation
The entry-reading loop reads each entry as an 8-byte key followed by a Summary-width summary. The summary read is passed the remaining capacity and bounds-checks itself, but the key read is an unchecked fixed-size copy_from_mem that relies solely on the pre-loop keys-only reservation, which assumes every entry's summary occupies the width Summary serializes to. When that assumption does not hold (a truncated buffer, or a buffer whose serialized summary width differs from the Summary it is deserialized as) the read cursor advances past the data. num_entries is read from the preamble and is unaffected, so the loop runs the full count and a later key read walks off the end of the buffer: silent on a normal build, a heap-buffer-overflow under AddressSanitizer. Add an ensure_minimum_memory check before the key read, mirroring the remaining-capacity check the summary read already performs and the up-front size validation the compact theta parser does. A malformed buffer now throws std::out_of_range instead of reading out of bounds. Adds a tuple_sketch_test case that deserializes a float-summary sketch as a double-summary sketch and asserts std::out_of_range. Co-authored-by: SavicStefan <50296686+SavicStefan@users.noreply.github.com>
proost
reviewed
Aug 10, 2026
proost
left a comment
Member
There was a problem hiding this comment.
The implementation change looks appropriate to me. But I suggest revising the test case to focus on truncated/malformed-buffer safety.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The entry-reading loop reads each entry as an 8-byte key followed by a Summary-width summary. The summary read is passed the remaining capacity and bounds-checks itself, but the key read is an unchecked fixed-size copy_from_mem that relies solely on the pre-loop keys-only reservation, which assumes every entry's summary occupies the width Summary serializes to.
When that assumption does not hold (a truncated buffer, or a buffer whose serialized summary width differs from the Summary it is deserialized as) the read cursor advances past the data. num_entries is read from the preamble and is unaffected, so the loop runs the full count and a later key read walks off the end of the buffer: silent on a normal build, a heap-buffer-overflow under AddressSanitizer.
Add an ensure_minimum_memory check before the key read, mirroring the remaining-capacity check the summary read already performs and the up-front size validation the compact theta parser does. A malformed buffer now throws std::out_of_range instead of reading out of bounds.
Adds a tuple_sketch_test case that deserializes a float-summary sketch as a double-summary sketch and asserts std::out_of_range.