Guard the in-place operations against self-application - #31
Merged
Conversation
roaring_bitmap_xor_inplace, roaring_bitmap_andnot_inplace,
roaring_bitmap_lazy_xor_inplace and roaring64_bitmap_xor_inplace assert that
their two operands are distinct, so rb.Xor(rb) aborted the whole process
rather than emptying the bitmap:
Assertion failed: (x1 != x2), function roaring_bitmap_xor_inplace
Under -DNDEBUG the assertion is compiled out and the same call corrupts
memory instead. The remaining in-place routines are inconsistent about it:
and_inplace guards on both widths, or_inplace and overwrite guard only on
the 64-bit side, and the 64-bit andnot_inplace guards nowhere.
Rather than document a precondition per method, answer the aliased case in
the wrapper: And and Or and Assign become no-ops, Xor and AndNot empty the
bitmap. That is what the set algebra calls for, and it costs one pointer
comparison.
Also stop turning an empty range into a panic. roaring_bitmap_from_range
returns a null pointer both for a zero step and for max <= min, and only
the former is a programming error, so FromRange and FromRange64 now return
an empty bitmap for an empty range as their documentation promised.
Two documentation gaps: ReadFrozenView64 needs the buffer length to be
exactly the length written, and MoveFrom32 takes the containers without
regard for copy-on-write sharing.
Claude-Session: https://claude.ai/code/session_01NzKGBVi8pbqkwocqktcjLU
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
1 participant
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.
Follow-up to #30. This was ready as a second commit on that branch but did not make it in before the merge, so
mastercurrently aborts the process onrb.Xor(rb).The crash
roaring_bitmap_xor_inplace,roaring_bitmap_andnot_inplace,roaring_bitmap_lazy_xor_inplaceandroaring64_bitmap_xor_inplaceassert that their two operands are distinct:Under
-DNDEBUGthe assertion is compiled out and the same call corrupts memory instead, which is the worse outcome.The C library is not consistent about this, so there is no single rule a caller could learn:
and_inplaceor_inplaceoverwritexor_inplaceandnot_inplacelazy_xor_inplaceRather than document a precondition per method, the wrapper now answers the aliased case itself, which costs one pointer comparison:
And,OrandAssignbecome no-ops,XorandAndNotempty the bitmap. That is what the set algebra says the answer is.Empty ranges
roaring_bitmap_from_rangereturns a null pointer both for a zero step and formax <= min. Only the first is a programming error, but I had guarded only the first, soFromRange(5, 5, 1)panicked with"C code returned a null pointer."instead of returning the empty bitmap its documentation promises. Fixed on both widths.Documentation
ReadFrozenView64needs the buffer length to be exactly the length written (the 32-bit doc said so, the 64-bit one did not), andMoveFrom32takes the containers without regard for copy-on-write sharing, so it should not be used on a bitmap that has been cloned with copy-on-write enabled.Testing
New tests cover every aliased combination on both widths and the empty-range cases. Full suite passes, including under
-race.https://claude.ai/code/session_01NzKGBVi8pbqkwocqktcjLU