Skip to content

Guard composite-index remove against short key arrays - #776

Merged
fh-ms merged 1 commit into
mainfrom
fix/composite-index-remove-short-key-aioobe
Jul 23, 2026
Merged

Guard composite-index remove against short key arrays#776
fh-ms merged 1 commit into
mainfrom
fix/composite-index-remove-short-key-aioobe

Conversation

@fh-ms

@fh-ms fh-ms commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Problem

Removing an entity from a GigaMap that carries a hashing composite bitmap index throws ArrayIndexOutOfBoundsException when the removed entity's key array is shorter than the widest key array the index has ever seen.

A composite index keeps one sub-index per key-array position and grows subIndices to the longest key array added so far (positions are never removed). The add, query, contains and change paths each guard every position with isEmpty(keys, i) (which tolerates i >= keys.length), but AbstractCompositeBitmapIndex.internalRemoveForKeys iterated all sub-indices unconditionally and passed the key array straight through, so a sub-index at a trailing position read keys[position] out of bounds.

Reproducer

Entity A has two tags, so the index grows to two sub-indices. Entity B has one tag (position 1 skipped on add via isEmpty). removeById(B) then reads keys[1] on a length-1 array and throws ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1.

Fix

Make remove symmetric with add and query: iterate by index and skip empty positions via isEmpty(keys, i). Unlike the add and change paths, remove must not call ensureSubIndices — no bit was ever set for an empty position, so there is nothing to remove there. This mirrors the existing guard on internalHandleChanged, which was previously hardened against the same hazard.

Tests

Added a regression test (CompositeIndexNullHandlingTest.variableLengthCompositeKeyRemoveShouldWork) that removes an entity whose composite key array is shorter than the widest one previously indexed. It reproduces the exact Index 1 out of bounds for length 1 failure on the unpatched code and passes after the fix. The full gigamap module suite passes (1079 tests, 1 slow test skipped by default).

A hashing composite bitmap index keeps one sub-index per key-array position and grows to the longest key array ever added, never shrinking. The add, query, contains and change paths each guard every position with isEmpty(keys, i) (which tolerates i >= keys.length), but internalRemoveForKeys iterated all sub-indices unconditionally and passed the key array straight through, so a sub-index at a trailing position read keys[position] out of bounds. Removing an entity whose key array is shorter than the widest one seen therefore threw ArrayIndexOutOfBoundsException.

Make remove symmetric with add and query by iterating by index and skipping empty positions via isEmpty(keys, i). Unlike add and change, remove must not call ensureSubIndices: no bit was ever set for an empty position, so there is nothing to remove there.

Add a regression test covering removal of an entity with a variable-length composite key that is shorter than the widest key array previously indexed.
@fh-ms
fh-ms requested a review from Copilot July 23, 2026 14:04
@fh-ms fh-ms added bug Something isn't working GigaMap labels Jul 23, 2026

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.

Pull request overview

Fixes a crash in GigaMap composite bitmap index removal when an entity’s composite key array is shorter than the longest key array previously indexed, by making the remove path apply the same per-position “empty” guarding used by add/query/change.

Changes:

  • Hardened AbstractCompositeBitmapIndex.internalRemoveForKeys to skip “empty” (including out-of-range) key positions instead of iterating sub-indices unconditionally.
  • Added a regression test covering variable-length composite key arrays during removal.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/AbstractCompositeBitmapIndex.java Makes composite-index removal symmetric with add/query by guarding per-position with isEmpty(keys, i) to avoid out-of-bounds access.
gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/indexer/CompositeIndexNullHandlingTest.java Adds a regression test reproducing and preventing the variable-length composite-key removal crash.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@fh-ms
fh-ms requested a review from zdenek-jonas July 23, 2026 14:08
@fh-ms
fh-ms merged commit 2571ba6 into main Jul 23, 2026
16 checks passed
@fh-ms
fh-ms deleted the fix/composite-index-remove-short-key-aioobe branch July 23, 2026 14:42
@fh-ms fh-ms added this to the 4.2.0 milestone Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working GigaMap

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants