This is an automated email from the ASF dual-hosted git repository.
FrankChen021 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/master by this push:
new 8803f6cbb68 fix: validate V1 header count and value offsets in
GenericIndexed (#20447)
8803f6cbb68 is described below
commit 8803f6cbb684890621aad152ee6cf4c5b110aa46
Author: dfengliu <[email protected]>
AuthorDate: Tue Oct 20 20:18:56 2026 +0800
fix: validate V1 header count and value offsets in GenericIndexed (#20447)
* fix: validate V1 header count and value offsets in GenericIndexed
A crafted V1 header let a negative or overflowing element count through,
and a malformed offset table moved reads out of the value region, surfacing
as raw JVM exceptions (BufferUnderflowException, NegativeArraySizeException,
IndexOutOfBoundsException) or silent misreads of header bytes. Reject
non-negative counts and out-of-range offsets with descriptive errors.
* fix: validate GenericIndexed offsets against a stable bound
The singleThreaded value buffer is reused across calls and its limit is
narrowed to the previous value's end. Bounds-checking against the mutable
limit made rightward binary-search probes in the UTF-8 dictionary lookup
path fail for valid sorted dictionaries. Validate against the original
value-region limit captured when the view is created, and add regression
tests for rightward probes and successive lookups.
* fix: validate relative header offsets in GenericIndexed
Validate each element's offsets against the header table before adding the
V1 base offset. Tables whose previous end offset falls below the four-byte
value-size marker (e.g. [0, 10]) previously let get() decode bytes outside
the declared element; they are now rejected with a descriptive error on all
four read paths (V1/V2, cached and single-threaded).
* fix: read the null/empty marker through a stable view in GenericIndexed
The singleThreaded value buffer is reused and its limit is narrowed by
earlier lookups, so the absolute null/empty marker read could throw
IndexOutOfBoundsException for a later in-range zero-length or null value.
Read the marker through a dedicated view captured at the stable original
limit, and add a successive-lookup regression test.
---
.../apache/druid/segment/data/GenericIndexed.java | 55 +++++-
.../data/GenericIndexedMalformedHeaderTest.java | 193 +++++++++++++++++++++
2 files changed, 243 insertions(+), 5 deletions(-)
diff --git
a/processing/src/main/java/org/apache/druid/segment/data/GenericIndexed.java
b/processing/src/main/java/org/apache/druid/segment/data/GenericIndexed.java
index 1225800c4b8..e888b18e3a7 100644
--- a/processing/src/main/java/org/apache/druid/segment/data/GenericIndexed.java
+++ b/processing/src/main/java/org/apache/druid/segment/data/GenericIndexed.java
@@ -19,6 +19,7 @@
package org.apache.druid.segment.data;
+import com.google.common.base.Preconditions;
import com.google.common.primitives.Ints;
import org.apache.druid.collections.ResourceHolder;
import org.apache.druid.common.utils.SerializerUtils;
@@ -282,9 +283,16 @@ public abstract class GenericIndexed<T> implements
CloseableIndexed<T>, Serializ
)
{
super(strategy, allowReverseLookup, buffer.getInt());
+ Preconditions.checkArgument(size >= 0, "size[%s] must be non-negative",
size);
this.theBuffer = buffer;
this.headerOffset = theBuffer.position();
- this.valuesOffset = theBuffer.position() + size * Integer.BYTES;
+ final long valuesOffsetLong = (long) headerOffset + (long) size *
Integer.BYTES;
+ Preconditions.checkArgument(
+ valuesOffsetLong <= theBuffer.limit(),
+ "size[%s] exceeds the available buffer",
+ size
+ );
+ this.valuesOffset = (int) valuesOffsetLong;
}
@Nullable
@@ -304,6 +312,7 @@ public abstract class GenericIndexed<T> implements
CloseableIndexed<T>, Serializ
startOffset = theBuffer.getInt(headerOffset + headerPosition) +
Integer.BYTES;
endOffset = theBuffer.getInt(headerOffset + headerPosition +
Integer.BYTES);
}
+ validateRelativeOffsets(index != 0, startOffset, endOffset);
return copyBufferAndGet(theBuffer, valuesOffset + startOffset,
valuesOffset + endOffset);
}
@@ -311,6 +320,8 @@ public abstract class GenericIndexed<T> implements
CloseableIndexed<T>, Serializ
public BufferIndexed singleThreaded()
{
final ByteBuffer copyBuffer = theBuffer.asReadOnlyBuffer();
+ final int bufferLimit = copyBuffer.limit();
+ final ByteBuffer markerView = theBuffer.asReadOnlyBuffer();
return new BufferIndexed()
{
@Nullable
@@ -330,7 +341,8 @@ public abstract class GenericIndexed<T> implements
CloseableIndexed<T>, Serializ
startOffset = theBuffer.getInt(headerOffset + headerPosition) +
Integer.BYTES;
endOffset = theBuffer.getInt(headerOffset + headerPosition +
Integer.BYTES);
}
- return bufferedIndexedGetByteBuffer(copyBuffer, valuesOffset +
startOffset, valuesOffset + endOffset);
+ validateRelativeOffsets(index != 0, startOffset, endOffset);
+ return bufferedIndexedGetByteBuffer(copyBuffer, markerView,
valuesOffset + startOffset, valuesOffset + endOffset, bufferLimit);
}
@Override
@@ -407,6 +419,7 @@ public abstract class GenericIndexed<T> implements
CloseableIndexed<T>, Serializ
startOffset = headerBuffer.getInt(headerPosition) + Integer.BYTES;
endOffset = headerBuffer.getInt(headerPosition + Integer.BYTES);
}
+ validateRelativeOffsets(relativePositionOfIndex != 0, startOffset,
endOffset);
int fileNum = index >> logBaseTwoOfElementsPerValueFile;
return copyBufferAndGet(valueBuffers[fileNum], startOffset, endOffset);
}
@@ -415,8 +428,12 @@ public abstract class GenericIndexed<T> implements
CloseableIndexed<T>, Serializ
public BufferIndexed singleThreaded()
{
final ByteBuffer[] copyValueBuffers = new
ByteBuffer[valueBuffers.length];
+ final ByteBuffer[] markerViews = new ByteBuffer[valueBuffers.length];
+ final int[] valueBufferLimits = new int[valueBuffers.length];
for (int i = 0; i < valueBuffers.length; i++) {
copyValueBuffers[i] = valueBuffers[i].asReadOnlyBuffer();
+ markerViews[i] = valueBuffers[i].asReadOnlyBuffer();
+ valueBufferLimits[i] = copyValueBuffers[i].limit();
}
return new BufferIndexed()
@@ -440,8 +457,9 @@ public abstract class GenericIndexed<T> implements
CloseableIndexed<T>, Serializ
startOffset = headerBuffer.getInt(headerPosition) + Integer.BYTES;
endOffset = headerBuffer.getInt(headerPosition + Integer.BYTES);
}
+ validateRelativeOffsets(relativePositionOfIndex != 0, startOffset,
endOffset);
int fileNum = index >> logBaseTwoOfElementsPerValueFile;
- return bufferedIndexedGetByteBuffer(copyValueBuffers[fileNum],
startOffset, endOffset);
+ return bufferedIndexedGetByteBuffer(copyValueBuffers[fileNum],
markerViews[fileNum], startOffset, endOffset, valueBufferLimits[fileNum]);
}
@Override
@@ -559,11 +577,30 @@ public abstract class GenericIndexed<T> implements
CloseableIndexed<T>, Serializ
return IndexedIterable.create(this).iterator();
}
+ private static void validateRelativeOffsets(boolean hasPrevious, int
startOffset, int endOffset)
+ {
+ if (endOffset < startOffset || (hasPrevious && startOffset - Integer.BYTES
< Integer.BYTES)) {
+ throw new IAE(
+ "value offsets out of bounds: startOffset[%s], endOffset[%s]",
+ startOffset,
+ endOffset
+ );
+ }
+ }
+
@Nullable
protected T copyBufferAndGet(ByteBuffer valueBuffer, int startOffset, int
endOffset)
{
ByteBuffer copyValueBuffer = valueBuffer.asReadOnlyBuffer();
int size = endOffset - startOffset;
+ if (startOffset < Integer.BYTES || endOffset < startOffset || endOffset >
copyValueBuffer.limit()) {
+ throw new IAE(
+ "value offsets out of bounds: startOffset[%s], endOffset[%s],
limit[%s]",
+ startOffset,
+ endOffset,
+ copyValueBuffer.limit()
+ );
+ }
if (size == 0 && (copyValueBuffer.get(startOffset - Integer.BYTES) ==
NULL_VALUE_SIZE_MARKER)) {
return null;
}
@@ -599,10 +636,18 @@ public abstract class GenericIndexed<T> implements
CloseableIndexed<T>, Serializ
}
@Nullable
- ByteBuffer bufferedIndexedGetByteBuffer(ByteBuffer copyValueBuffer, int
startOffset, int endOffset)
+ ByteBuffer bufferedIndexedGetByteBuffer(ByteBuffer copyValueBuffer,
ByteBuffer markerView, int startOffset, int endOffset, int bufferLimit)
{
int size = endOffset - startOffset;
- if (size == 0 && (copyValueBuffer.get(startOffset - Integer.BYTES) ==
NULL_VALUE_SIZE_MARKER)) {
+ if (startOffset < Integer.BYTES || endOffset < startOffset || endOffset
> bufferLimit) {
+ throw new IAE(
+ "value offsets out of bounds: startOffset[%s], endOffset[%s],
limit[%s]",
+ startOffset,
+ endOffset,
+ bufferLimit
+ );
+ }
+ if (size == 0 && (markerView.get(startOffset - Integer.BYTES) ==
NULL_VALUE_SIZE_MARKER)) {
return null;
}
diff --git
a/processing/src/test/java/org/apache/druid/segment/data/GenericIndexedMalformedHeaderTest.java
b/processing/src/test/java/org/apache/druid/segment/data/GenericIndexedMalformedHeaderTest.java
new file mode 100644
index 00000000000..90191466092
--- /dev/null
+++
b/processing/src/test/java/org/apache/druid/segment/data/GenericIndexedMalformedHeaderTest.java
@@ -0,0 +1,193 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment.data;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+
+public class GenericIndexedMalformedHeaderTest
+{
+ private static ByteBuffer buildV1(int numElements, int[] offsets, byte[]
values, int numBytesUsedOverride)
+ {
+ final int valuesBytes = values.length;
+ final int numBytesUsed = numBytesUsedOverride >= 0 ? numBytesUsedOverride
: 8 + numElements * 4 + valuesBytes;
+ final ByteBuffer buffer =
ByteBuffer.allocate(64).order(ByteOrder.BIG_ENDIAN);
+ buffer.put((byte) 0x1); // VERSION_ONE
+ buffer.put((byte) 0x0); // allowReverseLookup
+ buffer.putInt(numBytesUsed);
+ buffer.putInt(numElements);
+ for (int offset : offsets) {
+ buffer.putInt(offset);
+ }
+ buffer.put(values);
+ buffer.flip();
+ buffer.limit(buffer.capacity());
+ return buffer;
+ }
+
+ @Test
+ public void testNegativeCountRejected()
+ {
+ final IllegalArgumentException e = Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> GenericIndexed.read(buildV1(-5, new int[]{}, new byte[]{1, 2,
3}, 8), GenericIndexed.STRING_STRATEGY, null)
+ );
+ Assertions.assertTrue(e.getMessage().contains("must be non-negative"),
e.getMessage());
+ }
+
+ @Test
+ public void testOverflowCountRejected()
+ {
+ final IllegalArgumentException e = Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> GenericIndexed.read(buildV1(0x40000001, new int[]{}, new
byte[]{}, 8), GenericIndexed.STRING_STRATEGY, null)
+ );
+ Assertions.assertTrue(e.getMessage().contains("exceeds the available
buffer"), e.getMessage());
+ }
+
+ @Test
+ public void testHugeOffsetRejected()
+ {
+ GenericIndexed<String> indexed = GenericIndexed.read(
+ buildV1(2, new int[]{6, 1000}, new byte[]{0, 0, 0, 2, 'a', 'b', 0, 0,
0, 2, 'c', 'd'}, -1),
+ GenericIndexed.STRING_STRATEGY,
+ null
+ );
+ Assertions.assertEquals("ab", indexed.get(0));
+ final IllegalArgumentException e = Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> indexed.get(1)
+ );
+ Assertions.assertTrue(e.getMessage().contains("out of bounds"),
e.getMessage());
+ }
+
+ @Test
+ public void testNegativeOffsetRejected()
+ {
+ GenericIndexed<String> indexed = GenericIndexed.read(
+ buildV1(2, new int[]{-5, 10}, new byte[]{0, 0, 0, 2, 'x', 'y', 0, 0,
0, 2, 'p', 'q'}, -1),
+ GenericIndexed.STRING_STRATEGY,
+ null
+ );
+ final IllegalArgumentException e = Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> indexed.get(0)
+ );
+ Assertions.assertTrue(e.getMessage().contains("out of bounds"),
e.getMessage());
+ }
+
+ @Test
+ public void testValidHeaderReads()
+ {
+ GenericIndexed<String> indexed = GenericIndexed.read(
+ buildV1(1, new int[]{6}, new byte[]{0, 0, 0, 2, 'h', 'i'}, -1),
+ GenericIndexed.STRING_STRATEGY,
+ null
+ );
+ Assertions.assertEquals(1, indexed.size());
+ Assertions.assertEquals("hi", indexed.get(0));
+ }
+
+ @Test
+ public void testSingleThreadedRightwardProbes()
+ {
+ GenericIndexed<String> indexed = GenericIndexed.fromIterable(
+ java.util.Arrays.asList("a", "b", "c"),
+ GenericIndexed.STRING_STRATEGY
+ );
+ GenericIndexed<String>.BufferIndexed bi = indexed.singleThreaded();
+
+ Assertions.assertDoesNotThrow(() -> bi.getByteBuffer(0));
+ // A rightward binary-search probe must not be rejected by the bounds check
+ // after an earlier probe narrowed the reused buffer's limit.
+ Assertions.assertDoesNotThrow(() -> bi.getByteBuffer(2));
+ Assertions.assertDoesNotThrow(() -> bi.getByteBuffer(1));
+ }
+
+ @Test
+ public void testSingleThreadedSuccessiveLookups()
+ {
+ GenericIndexed<String> indexed = GenericIndexed.fromIterable(
+ java.util.Arrays.asList("a", "b", "c"),
+ GenericIndexed.STRING_STRATEGY
+ );
+ GenericIndexed<String>.BufferIndexed bi = indexed.singleThreaded();
+
+ Assertions.assertDoesNotThrow(() -> bi.getByteBuffer(1));
+ final ByteBuffer second = bi.getByteBuffer(2);
+ Assertions.assertEquals(1, second.remaining());
+ Assertions.assertEquals((byte) 'c', second.get());
+ final ByteBuffer first = bi.getByteBuffer(0);
+ Assertions.assertEquals(1, first.remaining());
+ Assertions.assertEquals((byte) 'a', first.get());
+ }
+
+ @Test
+ public void testIntermediateOffsetGapRejected()
+ {
+ GenericIndexed<String> indexed = GenericIndexed.read(
+ buildV1(2, new int[]{0, 10}, new byte[]{0, 0, 0, 2, 'x', 'y', 0, 0, 0,
2, 'p', 'q'}, -1),
+ GenericIndexed.STRING_STRATEGY,
+ null
+ );
+ // offsets[0] == 0 leaves the first value without its four-byte size
marker,
+ // so a direct get(1) must reject the header instead of decoding outside
the element.
+ final IllegalArgumentException e = Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> indexed.get(1)
+ );
+ Assertions.assertTrue(e.getMessage().contains("out of bounds"),
e.getMessage());
+ }
+
+ @Test
+ public void testSingleThreadedEmptyAndNullValues()
+ {
+ GenericIndexed<String> indexed = GenericIndexed.fromIterable(
+ java.util.Arrays.asList("a", "", null),
+ GenericIndexed.STRING_STRATEGY
+ );
+ GenericIndexed<String>.BufferIndexed bi = indexed.singleThreaded();
+
+ // An earlier direct lookup narrows the reused buffer's limit; later
empty/null
+ // marker reads must not throw IndexOutOfBoundsException because of it.
+ Assertions.assertDoesNotThrow(() -> bi.getByteBuffer(0));
+ final ByteBuffer empty = bi.getByteBuffer(1);
+ Assertions.assertEquals(0, empty.remaining());
+ Assertions.assertNull(bi.getByteBuffer(2));
+ }
+
+ @Test
+ public void testSingleThreadedIntermediateOffsetGapRejected()
+ {
+ GenericIndexed<String> indexed = GenericIndexed.read(
+ buildV1(2, new int[]{0, 10}, new byte[]{0, 0, 0, 2, 'x', 'y', 0, 0, 0,
2, 'p', 'q'}, -1),
+ GenericIndexed.STRING_STRATEGY,
+ null
+ );
+ final IllegalArgumentException e = Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> indexed.singleThreaded().getByteBuffer(1)
+ );
+ Assertions.assertTrue(e.getMessage().contains("out of bounds"),
e.getMessage());
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
