Skip to content

Fix BinaryIndexerString collision on NUL-containing keys - #689

Merged
fh-ms merged 2 commits into
mainfrom
gigamap-binaryindexerstring-nul-guard
Jun 2, 2026
Merged

Fix BinaryIndexerString collision on NUL-containing keys#689
fh-ms merged 2 commits into
mainfrom
gigamap-binaryindexerstring-nul-guard

Conversation

@fh-ms

@fh-ms fh-ms commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Summary

BinaryIndexerString packs a string's UTF-8 bytes into longs without encoding the byte length. Trailing 0x00 (NUL) bytes occupy the high bytes of the last long and
contribute nothing, so they vanish during packing — and a long that ends up fully 0L is additionally remapped to the Long.MAX_VALUE "all-null" sentinel. As a result:

  • "alpha" and "alpha\u0000" pack to the same index key, and
  • every NUL-only string of length 1..8 maps to the identical key [Long.MAX_VALUE].

An exact-match query for one therefore returns the other. This contradicts the index's own equality notion — Condition#test(entity) (the in-memory predicate the same condition uses) reports no match — so the index disagrees with a linear scan. It's a false-positive only (no missed results, no data loss), but a genuine exact-match correctness gap, and it survives the empty-string-sentinel fix from #688 (the empty string has its own distinct sentinel; NUL strings still don't).

Fix

Reject keys containing the NUL character (U+0000) with an IllegalArgumentException in fillCarrier. Both the add path (index → indexValue → fillCarrier) and the query path (is → isValue → indexValue → fillCarrier) converge there, so NUL is rejected on both adding an entity and forming a query condition.

This mirrors the existing family pattern where BinaryIndexerLong rejects the reserved Long.MAX_VALUE. It turns a silent wrong-result into a loud failure, requires no change to the persistent format (no migration of existing GigaMaps), and leaves NUL-free strings and the empty string fully supported. Lenient "trim trailing garbage" behavior, if desired, belongs at the input — not as a silent, partial, NUL-only normalization inside the index.

Changes

  • BinaryIndexerString.java — guard at the top of fillCarrier throwing IllegalArgumentException for NUL keys; updated Javadoc.
  • BinaryIndexerStringNulRejectionTest.java (new) — verifies NUL-only, trailing-NUL, and embedded-NUL keys are rejected on both add and query, and that normal keys / the empty string still work. NUL strings are built at runtime so the source contains no raw NUL bytes.
  • BinaryIndexerStringTest.java — the existing test stored an 8-NUL string (now rejected); replaced that scenario with a normal value.
  • docs/.../indexing/bitmap/types.adoc — documented the limitation alongside the BinaryIndexerLong/Long.MAX_VALUE note, with a code example showing how to normalize NUL out (cutAtNul / stripNul) on both the indexed value and the query key.

Trade-off

Breaks callers that currently store NUL-containing strings (rare). They must strip or cut at the terminator before indexing — see the new docs example.

BinaryIndexerString packs UTF-8 bytes into longs without encoding the byte length, so trailing 0x00 (NUL) bytes vanish during packing: "alpha" and "alpha\u0000", or NUL-only strings of different length, collided the same index key. An exact-match query then returned entities that an in-memory Condition#test scan rejects.

Reject keys containing the NUL character (U+0000) with an IllegalArgumentException in fillCarrier, which both the add and query paths go through. This mirrors how BinaryIndexerLong rejects the reserved Long.MAX_VALUE and turns a silent false-positive into a loud failure, with no change to the persistent format. NUL-free strings and the empty string (#688 sentinel) are unaffected.

This comment was marked as outdated.

This comment was marked as outdated.

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

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

Comment thread docs/modules/gigamap/pages/indexing/bitmap/types.adoc Outdated
- BinaryIndexerString: collapse the stray trailing blank lines before the interface's closing brace to a single one, matching neighboring types.
- types.adoc: clarify that NUL rejection applies to every operation that derives the index key (add, query, update, remove — the latter two re-index), and add an upgrade WARNING for storages that already contain NUL-valued keys.
@fh-ms
fh-ms requested a review from zdenek-jonas June 2, 2026 10:49
@fh-ms
fh-ms merged commit 4767513 into main Jun 2, 2026
14 checks passed
@fh-ms
fh-ms deleted the gigamap-binaryindexerstring-nul-guard branch June 2, 2026 11:53
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants