Skip to content

[ISSUE #8262]Fix bitsarray off by one#10506

Merged
ShannonDing merged 3 commits into
apache:developfrom
SummCoder:fix-bitsarray-off-by-one
Jun 15, 2026
Merged

[ISSUE #8262]Fix bitsarray off by one#10506
ShannonDing merged 3 commits into
apache:developfrom
SummCoder:fix-bitsarray-off-by-one

Conversation

@SummCoder

Copy link
Copy Markdown
Contributor

Which Issue(s) This PR Fixes

Fixes #8262

Brief Description

This PR fixes an off-by-one error in the boundary checks of
BitsArray.checkBytePosition and BitsArray.checkBitPosition
in the filter module.

The two methods use > instead of >= when checking whether
a position is out of bounds:

checkBytePosition: if (bytePos > byteLength()) // should be >=
checkBitPosition: if (bitPos > bitLength()) // should be >=

For a byte array of length N, valid indices are 0 through N-1.
Position N is the first invalid index, so the check should use >=.

With the old code, position N passes the check and causes
ArrayIndexOutOfBoundsException instead of the intended
IllegalArgumentException with a descriptive message.

Production impact if not fixed: Minimal. getByte/setByte are
only called internally by xor/or/and with guaranteed safe
bounds. getBit/setBit positions come from BloomFilterData hash
functions which always produce in-range values. No known production
trigger exists.

Impact of the fix: Zero behavioral change for all valid positions
(0 to length-1). Only the boundary case is corrected.

Additionally, this PR adds unit tests to improve code coverage
for the filter and auth modules:

  • BitsArrayTest (27 tests): covers create (bitLength, bytes,
    bytes+bitLength), setBit/getBit, setByte/getByte, xor,
    or, and (array-level and bit-level), not, clone,
    toString, large bit arrays, and boundary/edge cases including
    regression tests for the fixed checks.

  • PlainAccessConfigTest (12 tests): covers getters/setters,
    topicPerms/groupPerms, equals (same object, identical
    configs, different fields, null, different class), hashCode
    consistency, and toString.

How Did You Test This Change?

  1. Ran BitsArrayTest with the fixed code — all 27 tests pass.
  2. Verified the regression tests correctly expect
    IllegalArgumentException at boundary positions.
  3. Verified with the old code that the same boundary positions
    cause ArrayIndexOutOfBoundsException instead.
  4. Ran mvn test -pl filter -Dtest=BitsArrayTest and
    mvn test -pl auth -Dtest=PlainAccessConfigTest — both
    BUILD SUCCESS.

Replace five outdated commands (updateAclConfig, deleteAccessConfig,
updateGlobalWhiteAddr, clusterAclConfigVersion, getAclConfig) with
the six commands available in RocketMQ 5.5.0:

  createAcl, updateAcl, deleteAcl, getAcl, listAcl, copyAcl

Verified: all five old commands return 'sub command not exist' on
RocketMQ 5.5.0. Each new command documented with parameters and
usage examples from actual mqadmin -h output.

Fixes apache#10502
**Bug fix**
checkBytePosition and checkBitPosition used '>' instead of '>=',
allowing positions equal to array length to pass validation and
cause ArrayIndexOutOfBoundsException instead of the intended
IllegalArgumentException.

**Production impact if not fixed**
Minimal. getByte/setByte are only called internally by xor/or/and
with safe bounds. getBit/setBit positions come from BloomFilterData
hash functions which always produce in-range values. The incorrect
check has no known production trigger.

**Impact of fix**
Zero negative impact. All valid positions (0 to length-1) unchanged.
Edge case now correctly throws IllegalArgumentException instead of
ArrayIndexOutOfBoundsException.

**Tests added**
- BitsArrayTest: 27 tests covering create, bit/byte ops, boundary,
  including regression tests for the fixed checks
- PlainAccessConfigTest: 12 tests for getters/setters, equals/hashCode

References apache#8262
@codecov-commenter

codecov-commenter commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.11%. Comparing base (91cb333) to head (a024794).
⚠️ Report is 5 commits behind head on develop.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop   #10506      +/-   ##
=============================================
- Coverage      48.13%   48.11%   -0.02%     
- Complexity     13355    13365      +10     
=============================================
  Files           1377     1377              
  Lines         100707   100707              
  Branches       13010    13010              
=============================================
- Hits           48477    48457      -20     
- Misses         46296    46302       +6     
- Partials        5934     5948      +14     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@RockteMQ-AI RockteMQ-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.

Review by github-manager-bot

Summary

Fixes an off-by-one error in BitsArray.checkBytePosition() and BitsArray.checkBitPosition() where > should be >= for boundary checks. Adds comprehensive tests for the boundary conditions. Fixes #8262.

Findings

  • [Info] BitsArray.java:44,52 — The fix from > to >= is correct. For a byte array of length N, valid indices are 0 to N-1. Position N should trigger IllegalArgumentException, not fall through to cause ArrayIndexOutOfBoundsException.
  • [Info] BitsArrayTest.java — Test additions are thorough: testCheckBytePositionBoundary() and testCheckBitPositionBoundary() specifically test the boundary values (N-1 should pass, N should throw). The refactored test methods (testGetByte, testSetByte, etc.) improve overall test coverage.
  • [Info] FilterTest.java — New test class for BitsArray filter operations provides good integration-level coverage.

Suggestions

  • The large diff (455+ lines) is mostly test additions and refactoring, which is appropriate for a bug fix that needs comprehensive boundary testing.
  • Consider adding a test for BitsArray with size 0 or 1 to verify edge cases at the minimum boundary.

Clean bug fix with excellent test coverage. Fixes #8262. 👍


Automated review by github-manager-bot

@SummCoder SummCoder changed the title Fix bitsarray off by one [ISSUE: #8262]Fix bitsarray off by one Jun 15, 2026
@SummCoder SummCoder changed the title [ISSUE: #8262]Fix bitsarray off by one [ISSUE #8262]Fix bitsarray off by one Jun 15, 2026
@ShannonDing ShannonDing merged commit 4082e31 into apache:develop Jun 15, 2026
10 checks passed
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.

[Enhancement] Increase the code test coverage of the project to more than 50% with the help of Tongyi Lingma's capabilities.

4 participants