Repository navigation
Conversation
merge_higher_levels() moved the items of this sketch into a work buffer, freed the item storage, and only then allocated the new storage. If that allocation failed, items_ was left pointing at freed memory. The capacity after compression depends only on the level sizes, so a new helper, kll_helper::predict_compress(), computes it before any item moves. The merge now allocates the new item storage and reserves the levels first, and checks that general_compress() matches the prediction. The new test fails each allocation of a merge in turn, for several target and source sizes, and checks that the target stays usable and nothing leaks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Report for CI Build 43288534927Warning No base build found for commit Coverage: 82.892%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
This branch has not been deployed
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.
Problem
kll_sketch::merge()doesn't keep the basic exception guarantee when an allocation fails while merging levels above level zero.merge_higher_levels()moves this sketch's items into a work buffer, frees the item storage, and only then allocates the new storage. If that allocation throws,items_still points at the freed storage, and the sketch's levels still describe items that were moved out. Using or destroying the sketch afterwards is a use-after-free. ASan reports it with an allocator that fails on purpose.Only the application's allocator can trigger this, by failing during a merge in estimation mode. The serialized format is not involved. The same code is in 5.3.0 and 5.3.1.
Fix
The number of levels and items after compression depends only on the level sizes, not on the item values. A new helper,
kll_helper::predict_compress(), computes that result from the level sizes, mirroring the level loop ofgeneral_compress().merge_higher_levels()now does all of its allocations before it moves any items:levels_.reserve()It then checks that
general_compress()produced the predicted result. The merge results are unchanged.Tests
kll_sketch_merge_failure_test.cpp. It fails each allocation of a merge in turn. Targets are empty, in exact mode, or in estimation mode, and sources are smaller or larger than the target, passed as lvalues and as rvalues. After each failure, the test checks that the target can still be queried, updated, merged into and round-tripped. It also checks that an lvalue source is unchanged and that nothing leaks. Without the fix, this test fails under ASan with the use-after-free.Related
🤖 Generated with Claude Code