Skip to content

Fix integer underflow in BITCOUNT ... BIT byte-boundary mask - #1994

Merged
vazois merged 1 commit into
mainfrom
vazois/bitcount
Jul 29, 2026
Merged

Fix integer underflow in BITCOUNT ... BIT byte-boundary mask#1994
vazois merged 1 commit into
mainfrom
vazois/bitcount

Conversation

@vazois

@vazois vazois commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes an integer underflow in BITCOUNT ... BIT (BitmapManagerBitCount.cs) that caused non-zero counts to be returned for unset bits whenever a BIT-mode range ended on a byte boundary (endOffset % 8 == 7).

Root cause

BitIndexCount(byte* value, long startOffset, long endOffset) computed the inclusive right bit index as:

int rightBitIndex = (int)((endOffset + 1) & 7);   // endOffset=7 -> 0 (wrong)

When this wraps to 0, the inner BitIndexCount(byte, start, end) builds mask = (byte)((1<<0) - (1<<start)), a wrapping byte-mask underflow (e.g. 0x81). AND-ed with the bit-reversed payload this reports set bits that are not set. For byte 0x80, BITCOUNT key 7 7 BIT returned 1 even though GETBIT key 7 == 0.

Fix

Use the same formula already used (correctly) in BitmapManagerBitPos.cs:

int rightBitIndex = (int)(endOffset & 7) + 1;      // endOffset=7 -> 8 (correct)

Tests

  • Added BitmapBitCountBitBoundaryUnderflowTest covering single-bit and boundary-crossing BIT ranges on byte 0x80.
  • Updated BitmapBitCountLongBitOffsetParsingTest: its previous expectation of 8 was an artifact of the underflow. Under Garnet''s modulo-wrapping negative-offset semantics the clamped start wraps to bit index 1, so bits 1..7 of 0xFF = 7.
  • All BITCOUNT/BITPOS tests pass.

Copilot AI review requested due to automatic review settings July 29, 2026 21:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Ready to approve

The fix is narrowly scoped, matches the described root cause, and is covered by a focused regression test plus an updated expectation aligned to the documented offset semantics.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR fixes a correctness bug in Garnet’s bitmap BITCOUNT ... BIT implementation (BitmapManagerBitCount.cs) where ranges ending exactly on a byte boundary could return a non-zero count for bits that are actually unset (due to an integer underflow in the boundary mask calculation).

Changes:

  • Fixes the inclusive “right bit index” calculation for BIT-mode ranges that end on a byte boundary.
  • Adds a regression test validating single-bit and boundary-ending BIT ranges on a 0x80 payload.
  • Updates a long negative-offset parsing expectation that previously passed only because of the underflow behavior.
File summaries
File Description
test/standalone/Garnet.test.complexstring/GarnetBitmapTests.cs Adds a targeted regression test for the byte-boundary underflow and updates the long negative-offset BITCOUNT expectation.
libs/server/Resp/Bitmap/BitmapManagerBitCount.cs Corrects rightBitIndex computation to avoid wrapping to 0 on endOffset % 8 == 7, preventing mask underflow.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@vazois
vazois merged commit 52aef6e into main Jul 29, 2026
161 of 162 checks passed
@vazois
vazois deleted the vazois/bitcount branch July 29, 2026 23:23
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.

3 participants