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.

[dfengliu]

@FrankChen021 Thank you — fixed in 73fe920. The marker is now read through a dedicated view captured at the stable original limit (one per V1 closure, one per value file for V2), so it no longer depends on the reused buffer's narrowed limit, and the shared value buffer is not mutated for the marker check at all. Added testSingleThreadedEmptyAndNullValues covering successive lookups over an empty and a null value; it reproduced the IndexOutOfBoundsException before the fix and passes now.

[FrankChen021]

Thanks — I confirmed the fix at 73fe92000d09df0db2dcd38d4c9e78c82b5f0c19. The V1 and V2 single-threaded paths now read null/empty markers through stable views, so the reused buffer’s narrowed limit no longer causes the reported exception. The new successive empty/null regression test covers the case, and I found no remaining actionable issue. Reviewed 2 of 2 changed files.

[FrankChen021]

🟢 Approval recommended

No actionable issues found. The latest stable-marker-view fix and earlier reusable-buffer and relative-offset fixes are correct across the reviewed V1/V2 access paths.

Reviewed 2 of 2 changed files: GenericIndexed.java and GenericIndexedMalformedHeaderTest.java.

Static-only review; no builds, tests, installs, formatters, or long validation commands were run.

---

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