fix: validate V1 header count and value offsets in GenericIndexed

[dfengliu]

Description

GenericIndexed V1 trusts the serialized element count and the value offset table verbatim. A malformed header therefore fails with unrelated raw JVM exceptions on access, or silently misreads header bytes as values:

• negative count accepted -> Index>=size type failures on every access
• overflowing count (0x40000001) accepted (valuesOffset arithmetic wraps) -> IllegalArgumentException / IndexOutOfBoundsException
• offset beyond the value region -> BufferUnderflowException
• offset before the value region -> NegativeArraySizeException (new byte[-9] in StringUtils.fromUtf8) and, for other indices, a silent garbage string built from header/marker bytes

All of these are contained by JVM bounds checks, so this is an integrity/robustness issue for corrupted or tampered segment data, in the same class as the recent CompressedBlockReader / FixedIndexed header validation fixes.

Fixed the bug ...

• GenericIndexed.V1 constructor now rejects a negative element count ("size[%s] must be non-negative") and a values offset that overflows or exceeds the buffer ("size[%s] exceeds the available buffer") — computed in long to avoid the same wrap.
• copyBufferAndGet (and the single-threaded bufferedIndexedGetByteBuffer path) now validates that the value range is inside the buffer before reading ("value offsets out of bounds: startOffset[%s], endOffset[%s], limit[%s]"), turning the raw exceptions and silent misreads into descriptive IAEs. Legitimate empty/null values still work (NULL_VALUE_SIZE_MARKER check preserved).

Added GenericIndexedMalformedHeaderTest covering negative/overflow counts, out-of-range offsets (both directions), and a valid-header acceptance case. I verified the new tests fail without the fix (reproducing NegativeArraySizeException, BufferUnderflowException, and silent acceptance of bad counts) and pass with it; mvn validate (checkstyle) and mvn -DstrictCompile test-compile (Error Prone) pass locally, and the existing GenericIndexedTest still passes.

Release note

• Reject malformed GenericIndexed V1 headers (negative/overflowing element count, or value offsets outside the buffer) with a descriptive error instead of raw JVM exceptions or silent misreads.

---

Key changed/added classes in this PR

• GenericIndexed
• GenericIndexedMalformedHeaderTest

---

This PR has:

• been self-reviewed.
• added documentation for new or modified features or behaviors.
• a release note entry in the PR description.
• added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• added or updated version, license, or notice information in licenses.yaml
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
• added integration tests.
• been tested in a test Druid cluster.

[FrankChen021]

🟡 Changes recommended

Fix the reused-buffer bounds check before merging: it rejects valid UTF-8 dictionary lookups after an earlier binary-search probe narrows the buffer limit. The element-count checks do not address this regression in the shared V1/V2 access path.

Reviewed 2 of 2 changed files, with relevant context inspected in 6 supporting source/test files (8 code files total).

Validation: static review plus a narrow JShell reproducer using the extracted bufferedIndexedGetByteBuffer method. For a valid three-value dictionary, the first probe read b and set the limit to 26; the next valid probe failed with startOffset[30], endOffset[31], limit[26]. Restoring the end boundary reproduced the previous successful read of c. No repository build or test suite was run.

Severity	Findings
P0	0
P1	1
P2	0
P3	0
Total	1

---

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

[FrankChen021]

[P1] Validate against a stable bound before reusing the buffer

Finding: The limit of copyValueBuffer is mutable: this method sets it to endOffset, and the UTF8_STRATEGY branch of BufferIndexed.indexOf calls getByteBuffer directly without restoring it. Consequently, a valid sorted dictionary containing a, b, c throws while searching for c: the first probe reads b and lowers the limit to its end, then the next probe for c fails this new check. StringEncodingStrategies supplies exactly this singleThreaded UTF-8 view for ordinary string dictionaries, so valid dictionary lookups and queries can fail; the shared helper affects both V1 and V2. The added tests only exercise the non-buffered get path and miss this regression.

Suggestion: Validate against a stable boundary captured from the original value region rather than the reused buffer's current limit, and add singleThreaded UTF-8 tests covering rightward binary-search probes and successive lookups.

[dfengliu]

@FrankChen021 Thank you for the thorough catch — you were right. The singleThreaded value buffer is reused across calls and its limit narrows to the previous value's end, so the check against copyValueBuffer.limit() broke rightward binary-search probes. Fixed in 9b20197: both V1 and V2 singleThreaded paths now validate against the original value-region limit captured when the view is created (added bufferLimit / valueBufferLimits), and I added testSingleThreadedRightwardProbes plus testSingleThreadedSuccessiveLookups. I verified the new tests reproduce your reproducer against the old check (startOffset[30], endOffset[31], limit[21/26]) and pass with the fix.

[FrankChen021]

🟡 Changes recommended

The stable-bound fix for the original reused-buffer regression is present in both V1 and V2 single-threaded paths, and the new rightward-probe and successive-lookup tests cover that issue. The current head still has one malformed-header validation gap: the shared range check runs after V1 adds its value-region base, so an invalid intermediate header offset can pass on a direct later-index lookup and be decoded as data; the same derived-range gap exists for V2. This is the remaining issue described by the inline finding.

Reviewed 2 of 2 changed files:

• processing/src/main/java/org/apache/druid/segment/data/GenericIndexed.java
• processing/src/test/java/org/apache/druid/segment/data/GenericIndexedMalformedHeaderTest.java

Relevant surrounding code reviewed in GenericIndexedWriter, StringEncodingStrategies, GenericIndexedTest, and StringUtils to confirm the offset layout, UTF-8 access paths, and decoder behavior.

Validation: git diff --check 1afd36ba7d4874bb12624590faf0eee6454fbcba..HEAD passed. No build, test suite, dependency install, or formatter was run.

Severity	Findings
P0	0
P1	0
P2	1
P3	0
Total	1

---

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

[FrankChen021]

[P2] Validate relative header offsets before adding the V1 base

Finding: This check validates only the absolute range passed to it, but V1 adds valuesOffset to each header-derived offset before calling copyBufferAndGet. A malformed two-entry table such as offsets [0, 10] therefore lets a direct get(1) pass with startOffset[valuesOffset + 4] and endOffset[valuesOffset + 10], even though the prior end offset is below the mandatory four-byte value marker; the method then decodes bytes outside the declared element instead of rejecting the header. The same intermediate-offset gap exists in the shared V2 path, and the current tests only access index 0 for the negative-offset case, so malformed headers can still produce the silent misreads this PR is meant to eliminate.

Suggestion: Validate each header-relative end offset before converting it to an absolute position (or carry the value-region base into the validator), and add direct later-index tests for too-small or negative preceding offsets in both normal and single-threaded access paths.

[FrankChen021]

🟡 Changes recommended

The current head fixes the earlier reusable-buffer limit regression and relative-offset gap, but bufferedIndexedGetByteBuffer still reads null/empty markers through the buffer's mutable, previously narrowed limit. After an earlier direct lookup, a later in-range zero-length value can throw IndexOutOfBoundsException instead of returning the value or a descriptive validation error. Restore the stable limit before the marker read and add a regression test before merging.

Reviewed 2 of 2 changed files; surrounding V1/V2 layout, writer, UTF-8 comparator, and buffered access paths were inspected.

Validation: git diff --check 1afd36ba7d4874bb12624590faf0eee6454fbcba..HEAD passed. No build, test suite, dependency installation, or formatter was run.

Severity	Findings
P0	0
P1	0
P2	1
P3	0
Total	1

---

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

[FrankChen021]

[P2] Restore the reusable limit before marker lookup

Finding: This check uses the stable original limit, but the null/empty marker read still uses copyValueBuffer's current limit, which may have been narrowed by an earlier direct getByteBuffer call. A later in-range zero-length value can therefore throw IndexOutOfBoundsException instead of being returned or rejected descriptively.

Suggestion: Read the marker through a view restored to bufferLimit before checking it, and add a successive lookup regression for null or empty values.