Skip to content

fix(scan): check bitset length in PreviousSet and NextSet, avoid shift-by-64 and signed overflow - #245

Open
mazkev wants to merge 1 commit into
bits-and-blooms:masterfrom
mazkev:fix/previous-set-bounds
Open

mazkev wants to merge 1 commit into
bits-and-blooms:masterfrom
mazkev:fix/previous-set-bounds

Conversation

@mazkev

@mazkev mazkev commented Nov 13, 2026

Copy link
Copy Markdown

Summary

This PR addresses boundary inconsistencies across scan/search methods (PreviousSet, NextSet, NextSetMany), eliminates an undefined shift-by-64 in PreviousSet/PreviousClear, and fixes a signed integer cast in wordsNeededUnbound.

Problem Details & Fixes

  1. Logical length bounds check in PreviousSet, NextSet, and NextSetMany:

    • In PR Keep PreviousClear within the bitset length #235 (fix/previous-clear-logical-length), PreviousClear was updated to check if i >= b.length { return 0, false } so queries beyond the logical length are cleanly rejected.
    • However, PreviousSet, NextSet, and NextSetMany did not perform this check.
    • For example, with b := New(10).Set(5):
      • b.PreviousSet(20) returned (5, true) because index 20 falls within the first 64-bit backing word.
      • b.PreviousSet(64) returned (0, false) because word 1 was not allocated in b.set.
      • In both cases the queried index was out of bounds (>= b.length). Returning different results based solely on backing slice allocation boundaries is unexpected.
    • Added if i >= b.length { return 0, false } to PreviousSet, NextSet, and NextSetMany to match PreviousClear, NextClear, and Test.
  2. Shift-by-64 in PreviousSet and PreviousClear:

    • Both methods previously used word & ((1 << (wordsIndex(i) + 1)) - 1). When wordsIndex(i) == 63, this evaluated 1 << 64 on a uint64.
    • Updated to word & (allBits >> (wordSize - 1 - wordsIndex(i))), avoiding shifting by 64 bits and working portably across compilers and architectures without arithmetic subtraction.
  3. Signed integer conversion in wordsNeededUnbound:

    • wordsNeededUnbound had (int(i) + wordMask) >> log2WordSize. For large values of i where the high bit was set (e.g. 1 << 63), converting int(i) produced a negative integer, causing negative return values.
    • Restored to int((i + wordMask) >> log2WordSize).

Tests Added

  • TestPreviousSetBeyondLength: Verifies that PreviousSet returns (0, false) for indices beyond the bitset's logical length.
  • TestNextSetBeyondLength: Verifies that NextSet returns (0, false) for indices beyond length.
  • TestNextSetManyBeyondLength: Verifies that NextSetMany returns empty results for indices beyond length.
  • TestWordsNeededUnboundSignedCast: Verifies non-negative return values for large uint inputs.
  • TestPreviousSetWordBoundary: Verifies correct behavior when wordsIndex(i) == 63.
  • All existing tests and fuzz tests pass (go test ./... and go vet ./... across amd64 and 386).

This branch has not been deployed

No deployments
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.

1 participant