Repository navigation
[BUG FIX] In-place operations and overwrite with the same bitmap as both operands - #963
Merged
Merged
Conversation
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.
In-place operations and overwrite with the same bitmap as both operands
Passing one bitmap as both arguments of an in-place operation could read freed memory, give a wrong result or trip
an
assert. Each such call is now decided at entry, asroaring_bitmap_and_inplaceandroaring64_bitmap_or_inplacealready did:x | xleavesxas it is,x ^ xandx \ xempty it.These are set identities, true for every set A whatever it holds: union and intersection are idempotent
(A ∪ A = A ∩ A = A); every element of A is also in A, so none is in exactly one of the two (A △ A = ∅) and none
is in A but not in A (A \ A = ∅); copying A onto itself leaves A. So the answer is known from the pointers alone, without reading the containers.
Leaving the check to callers did not work: the headers disagreed (
and_inplaceallowed it,or_inplacehad a TODO,lazy_or_inplaceandoverwritewere silent, others forbade it), the only check was anassertthat releasebuilds drop, and aliasing is often invisible at the call site (a fold over a list holding the accumulator, C++
r = otherthrough references). The result is always defined, and one pointer comparison here replaces a guard at every call site.Note: I do have a follow up PR after this isolated change that improves all the in place operations performance making them all linear
(bug fix)
roaring_bitmap_lazy_or_inplace(a, a, true)no longer reads freed memoryThe header did not forbid
r1 == r2here; it now says it is allowed. With the bitset conversion on, each containerwas converted and freed, then read again as the second operand: a heap use-after-free on array and run containers,
and wrong values when it did not crash. Bitset containers were not converted, so they were not affected.
(bug fix)
roaring_bitmap_lazy_xor_inplace(a, a)no longer reads freed memoryEach emptied container was removed from the array that both operands share, while the walk kept the old length: on
a bitmap of several containers, release builds read and copied containers already freed (a heap use-after-free).
Debug builds stopped at its
assert(x1 != x2). It now empties the bitmap, its capacity kept.(bug fix)
roaring_bitmap_overwrite(a, a)and C++Roaringself-assignment no longer read freed memoryra_overwritefreed the destination's containers, then cloned the source's, which were the same ones.r = ron aRoaringgoes through it. It now leaves the bitmap unchanged (asroaring64_bitmap_overwritealready did) andreturns
true, and the header says so.(bug fix)
roaring64_bitmap_xor_inplace(a, a)androaring64_bitmap_andnot_inplace(a, a)empty the bitmapThe walk erased from the tree it was also reading as the second operand, so values were left behind: about half of
them (67959 of the 142858 in
roaring64_bitmap_from_range(0, 1000000, 7)), in release builds for xor and in everybuild for andnot, which had no
assert.The C++
Roaring64operators^=and-=reach them; their "undefined if&r == this" notes are dropped.(bug fix) 32-bit
xor_inplaceandandnot_inplaceacceptr1 == r2They asserted
r1 != r2in debug builds (release builds gave the right, empty, result); they now empty the bitmap(its capacity kept) in every build, so C++
Roaringr ^= randr -= rwork in debug builds too.or_inplace(a, a)returns at once, and the TODO in its header ("decide whether r1 == r2 ok") becomes "r1 == r2 isallowed"; the 64-bit
or_inplaceandoverwriteheaders now state the same. The C++-=and^=docs(
Roaring,Roaring64,Roaring64Map) now sayr -= randr ^= remptyr.🤖 Written and verified with the aid of Claude Code; I have reviewed every line.