laskoviymishka commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3534757148
##########
api/src/main/java/org/apache/iceberg/variants/SerializedArray.java:
##########
@@ -35,30 +35,47 @@ static SerializedArray from(VariantMetadata metadata,
byte[] bytes) {
return from(metadata,
ByteBuffer.wrap(bytes).order(ByteOrder.LITTLE_ENDIAN), bytes[0]);
}
+ @VisibleForTesting
static SerializedArray from(VariantMetadata metadata, ByteBuffer value, int
header) {
+ return from(metadata, value, header, 0);
+ }
+
+ static SerializedArray from(VariantMetadata metadata, ByteBuffer value, int
header, int depth) {
Preconditions.checkArgument(
value.order() == ByteOrder.LITTLE_ENDIAN, "Unsupported byte order: big
endian");
BasicType basicType = VariantUtil.basicType(header);
Preconditions.checkArgument(
basicType == BasicType.ARRAY, "Invalid array, basic type: " +
basicType);
- return new SerializedArray(metadata, value, header);
+ return new SerializedArray(metadata, value, header, depth);
}
private final VariantMetadata metadata;
private final ByteBuffer value;
private final int offsetSize;
private final int offsetListOffset;
private final int dataOffset;
+ private final int depth;
private final VariantValue[] array;
- private SerializedArray(VariantMetadata metadata, ByteBuffer value, int
header) {
+ private SerializedArray(VariantMetadata metadata, ByteBuffer value, int
header, int depth) {
this.metadata = metadata;
this.value = value;
+ this.depth = depth;
this.offsetSize = 1 + ((header & OFFSET_SIZE_MASK) >> OFFSET_SIZE_SHIFT);
int numElementsSize = ((header & IS_LARGE) == IS_LARGE) ? 4 : 1;
+ Preconditions.checkArgument(
+ value.remaining() >= HEADER_SIZE + numElementsSize,
+ "Invalid variant array: buffer too small for element count field");
int numElements = ByteBuffers.readLittleEndianUnsigned(value, HEADER_SIZE,
numElementsSize);
+ Preconditions.checkArgument(
+ numElements >= 0, "Invalid variant array: negative element count %s",
numElements);
this.offsetListOffset = HEADER_SIZE + numElementsSize;
- this.dataOffset = offsetListOffset + ((1 + numElements) * offsetSize);
+ long offsetTableEnd = (long) offsetListOffset + ((long) numElements + 1L)
* offsetSize;
+ Preconditions.checkArgument(
+ offsetTableEnd <= value.remaining(),
Review Comment:
This bounds `numElements` by the buffer size, but with `offsetSize=1` that's
still ~`value.remaining()` elements, and we allocate `new
VariantValue[numElements]` right after (same in `SerializedObject`, across five
arrays). Backed by a large Parquet buffer, a crafted count in the hundreds of
millions passes this check while the array allocation at 8 bytes/ref is an
order of magnitude larger — an amplification that OOMs the executor from a
physically valid buffer.
I'd add an absolute cap on `numElements` before the allocations, ideally
matching whatever bound parquet-java #3562 chose so we stay in parity. Worth a
test too — everything here uses tiny buffers, so the large-but-valid path is
currently unexercised. wdyt?
##########
api/src/main/java/org/apache/iceberg/variants/SerializedObject.java:
##########
@@ -96,11 +116,26 @@ private void initOffsetsAndLengths(int numElements) {
int dataLength =
ByteBuffers.readLittleEndianUnsigned(
value, offsetListOffset + (numElements * offsetSize), offsetSize);
+ long dataLen = value.remaining() - (long) dataOffset;
+ Preconditions.checkArgument(
+ dataLength >= 0 && dataLength <= dataLen,
+ "Invalid variant object: data length %s out of data region [0, %s]",
+ dataLength,
+ dataLen);
+ for (int index = 0; index < numElements; index += 1) {
+ Preconditions.checkArgument(
+ offsets[index] >= 0 && offsets[index] <= dataLength,
+ "Invalid variant object: offset %s out of declared data length %s",
+ offsets[index],
+ dataLength);
+ }
offsetToLength.put(dataLength, 0);
// populate lengths list by sorting offsets
List<Integer> sortedOffsets =
offsetToLength.keySet().stream().sorted().collect(Collectors.toList());
+ Preconditions.checkArgument(
+ sortedOffsets.size() == numElements + 1, "Invalid variant object:
duplicate field offsets");
Review Comment:
This one I'd block on. `offsetToLength` is a HashMap keyed by field offset,
so two fields whose data sits at the same byte offset collapse to one key,
`sortedOffsets.size()` comes back `< numElements + 1`, and this assertion
rejects the object.
The spec allows that. VariantEncoding.md says the `field_offset` values need
not be monotonically increasing, and parquet-java's `VariantUtil` (#3562)
explicitly notes children may overlap or leave gaps and deliberately does not
reject duplicate offsets. So an object where two zero-length fields share an
offset (or a compacting writer dedups) reads fine in parquet-java, Spark, and
iceberg-rust but throws here — a cross-client read regression on spec-legal
data.
I'd drop this assertion and compute lengths with a sorted pass that
tolerates equal offsets (zero-length spans). The per-field bounds check just
above already guarantees every offset lands in `[0, dataLength]`, which is what
the spec actually requires. wdyt?
##########
api/src/main/java/org/apache/iceberg/variants/SerializedMetadata.java:
##########
@@ -59,15 +59,32 @@ static SerializedMetadata from(ByteBuffer metadata) {
private SerializedMetadata(ByteBuffer metadata, int header) {
this.isSorted = (header & SORTED_STRINGS) == SORTED_STRINGS;
this.offsetSize = 1 + ((header & OFFSET_SIZE_MASK) >> OFFSET_SIZE_SHIFT);
+ Preconditions.checkArgument(
+ metadata.remaining() >= HEADER_SIZE + offsetSize,
+ "Invalid variant metadata: buffer too small for dictionary size
field");
int dictSize = ByteBuffers.readLittleEndianUnsigned(metadata, HEADER_SIZE,
offsetSize);
- this.dict = new String[dictSize];
+ Preconditions.checkArgument(
+ dictSize >= 0, "Invalid variant metadata: negative dictionary size
%s", dictSize);
this.offsetListOffset = HEADER_SIZE + offsetSize;
+ long offsetTableEnd = (long) offsetListOffset + ((long) dictSize + 1L) *
offsetSize;
+ Preconditions.checkArgument(
+ offsetTableEnd <= metadata.remaining(),
+ "Invalid variant metadata: dictionary size %s exceeds buffer",
+ dictSize);
+ this.dict = new String[dictSize];
this.dataOffset = offsetListOffset + ((1 + dictSize) * offsetSize);
- int endOffset =
- dataOffset
- + ByteBuffers.readLittleEndianUnsigned(
- metadata, offsetListOffset + (offsetSize * dictSize),
offsetSize);
- if (endOffset < metadata.limit()) {
+ int lastOffset =
+ ByteBuffers.readLittleEndianUnsigned(
+ metadata, offsetListOffset + (offsetSize * dictSize), offsetSize);
+ Preconditions.checkArgument(
+ lastOffset >= 0, "Invalid variant metadata: negative end offset %s",
lastOffset);
+ long endOffsetLong = (long) dataOffset + lastOffset;
+ Preconditions.checkArgument(
+ endOffsetLong <= metadata.remaining(),
+ "Invalid variant metadata: end offset %s exceeds buffer",
+ endOffsetLong);
+ int endOffset = (int) endOffsetLong;
+ if (endOffset < metadata.remaining()) {
Review Comment:
Switching this from `metadata.limit()` to `metadata.remaining()` is a
genuine fix, not just hardening — the old comparison of a position-relative
`endOffset` against the absolute limit was wrong for any buffer with a non-zero
position. I'd call it out in the PR description as a behavioral change rather
than leave it folded in with the bounds checks.
##########
api/src/test/java/org/apache/iceberg/variants/TestMalformedVariant.java:
##########
@@ -0,0 +1,342 @@
+/*
+ * 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.iceberg.variants;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import org.junit.jupiter.api.Test;
+
+public class TestMalformedVariant {
+
+ private static final ByteBuffer EMPTY_METADATA =
+ ByteBuffer.wrap(new byte[] {0x01, 0x00,
0x00}).order(ByteOrder.LITTLE_ENDIAN);
+
+ @Test
+ public void testOversizedMetadataDictSize() {
+ byte[] bytes = new byte[] {0x01, (byte) 0xFF};
+
+ assertThatThrownBy(() -> SerializedMetadata.from(bytes))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("dictionary size");
+ }
+
+ @Test
+ public void testUnsupportedMetadataVersionRejected() {
+ // header low 4 bits = version; spec version is 1. 0x02 = version 2, must
be rejected.
+ byte[] bytes = new byte[] {0x02, 0x00, 0x00};
+
+ assertThatThrownBy(() -> SerializedMetadata.from(bytes))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Unsupported version");
+ }
+
+ @Test
+ public void testOversizedArrayNumElements() {
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b10011, 0x00, 0x00, 0x00, 0x10,
0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testOversizedObjectNumElements() {
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b1000010, 0x00, 0x00, 0x00, 0x10,
0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testFieldIdOutOfRange() {
+ // field id check is lazy, so descend via get() to trigger it
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0x02, 0x01, 0x05, 0x00, 0x01, 0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ VariantValue top =
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value);
+
+ assertThatThrownBy(() -> top.asObject().get("anything"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("field id");
+ }
+
+ @Test
+ public void testOutOfRangeChildOffsetInArray() {
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0b0011, 0x01, (byte) 0xFF, 0x01, 0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ VariantValue top =
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value);
+
+ assertThatThrownBy(() -> top.asArray().get(0))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("out of data region");
+ }
+
+ @Test
+ public void testMalformedChildOffsetCaughtOnDescent() {
+ // outer array constructs cleanly. Inner element is rejected only when
accessed
+ ByteBuffer value =
+ ByteBuffer.wrap(
+ new byte[] {0b0011, 0x01, 0x00, 0x06, (byte) 0b10011, 0x00,
0x00, 0x00, 0x10, 0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ VariantValue top =
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value);
+
+ assertThatThrownBy(() -> top.asArray().get(0))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testNegativeMetadataEndOffset() {
+ byte[] metadata = {
+ (byte) 0b11000001, 0x00, 0x00, 0x00, 0x00, (byte) 0xFF, (byte) 0xFF,
(byte) 0xFF, (byte) 0xFF
+ };
+
+ assertThatThrownBy(() -> SerializedMetadata.from(metadata))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("negative end offset");
+ }
+
+ @Test
+ public void testNegativeDictOffsetInGet() {
+ byte[] metadata = {
+ (byte) 0b11000001,
+ 0x01,
+ 0x00,
+ 0x00,
+ 0x00,
+ (byte) 0xFF,
+ (byte) 0xFF,
+ (byte) 0xFF,
+ (byte) 0xFF,
+ 0x00,
+ 0x00,
+ 0x00,
+ 0x00
+ };
+
+ SerializedMetadata parsed = SerializedMetadata.from(metadata);
+
+ assertThatThrownBy(() -> parsed.get(0))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("dict entry");
+ }
+
+ @Test
+ public void testOversizedPrimitiveStringSize() {
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b1000000, (byte) 0xFF, (byte)
0xFF, (byte) 0xFF, 0x7F})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("payload");
+ }
+
+ @Test
+ public void testOffsetExceedsDeclaredDataLengthInObject() {
+ byte[] metadata = {0x01, 0x01, 0x00, 0x01, 'a'};
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0x02, 0x01, 0x00, 0x32, 0x00, 0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(metadata), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("declared data length");
+ }
+
+ @Test
+ public void testOffsetArithmeticOverflowInObject() {
+ // numElements * offsetSize would overflow signed int
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0x4E, 0x33, 0x33, 0x33, 0x33, (byte) 0xFF,
(byte) 0xFF, 0x3F})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testArrayDataOffsetIntOverflow() {
+ // large array, offsetSize=4, numElements=0x20000000 (~536M).
+ // (1+numElements)*offsetSize overflows signed int; long guard must fire
first.
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b11111, 0x00, 0x00, 0x00, 0x20,
0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testObjectDataOffsetIntOverflow() {
+ // large object, offsetSize=4, fieldIdSize=4, numElements=0x20000000
(~536M).
+ // int sub-expressions for offsetListOffset and dataOffset overflow; long
guard fires first.
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b1111110, 0x00, 0x00, 0x00, 0x20,
0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testNonMonotonicDictOffset() {
+ byte[] metadata = {0x01, 0x02, 0x00, 0x05, 0x03, 'A', 'B', 'C', 'D', 'E'};
+
+ SerializedMetadata parsed = SerializedMetadata.from(metadata);
+
+ assertThatThrownBy(() -> parsed.get(1))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("dict entry");
+ }
+
+ @Test
+ public void testDuplicateFieldOffsetsInObject() {
+ byte[] metadata = {0x01, 0x01, 0x00, 0x01, 'a'};
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0x02, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(metadata), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("duplicate field offsets");
+ }
+
+ @Test
+ public void testEmptyChildValueBufferInArray() {
+ // offset[0] == offset[1] produces a zero-length child slice
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0b0011, 0x01, 0x00,
0x00}).order(ByteOrder.LITTLE_ENDIAN);
+
+ VariantValue top =
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value);
+
+ assertThatThrownBy(() -> top.asArray().get(0))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("empty value buffer");
+ }
+
+ @Test
+ public void testShortStringLengthExceedsBuffer() {
+ // header declares length=7, but only the header byte is present
+ ByteBuffer value = ByteBuffer.wrap(new byte[]
{0x1D}).order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("length");
+ }
+
+ @Test
+ public void testNegativeObjectNumElements() {
+ // largeSize object header, four 0xFF bytes read as int32 -1
+ ByteBuffer value =
+ ByteBuffer.wrap(
+ new byte[] {(byte) 0b1000010, (byte) 0xFF, (byte) 0xFF, (byte)
0xFF, (byte) 0xFF})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("negative element count");
+ }
+
+ @Test
+ public void testNegativeMetadataDictSize() {
+ // offsetSize=4, four 0xFF bytes read as int32 -1
+ byte[] metadata = {(byte) 0b11000001, (byte) 0xFF, (byte) 0xFF, (byte)
0xFF, (byte) 0xFF};
+
+ assertThatThrownBy(() -> SerializedMetadata.from(metadata))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("negative dictionary size");
+ }
+
+ @Test
+ public void testOversizedMetadataEndOffset() {
+ // dictSize=0, lastOffset=0xFF claims 255 bytes of dict data past the
3-byte buffer
+ byte[] metadata = {0x01, 0x00, (byte) 0xFF};
+
+ assertThatThrownBy(() -> SerializedMetadata.from(metadata))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("end offset");
+ }
+
+ @Test
+ public void testTruncatedPrimitiveSizeField() {
+ // string header but buffer is too short to contain the 4-byte size field
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b1000000,
0x00}).order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("size field");
+ }
+
+ @Test
+ public void testOverdeepNestingRejected() {
+ int chainDepth = VariantUtil.MAX_VARIANT_DEPTH + 1;
Review Comment:
This covers the over-the-limit case through an array chain only. I'd add a
positive assertion right at the boundary — a chain of exactly
`MAX_VARIANT_DEPTH` should parse — and an object-chain variant, since
`SerializedObject.get()` routes through the same depth counter and nothing here
exercises that path.
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -30,8 +32,41 @@ class VariantUtil {
private static final int BASIC_TYPE_OBJECT = 2;
private static final int BASIC_TYPE_ARRAY = 3;
+ /**
+ * Maximum permitted nesting depth of a Variant value. The top-level value
is depth 0, so a
+ * Variant may contain up to {@code MAX_VARIANT_DEPTH + 1} nested levels.
+ */
+ static final int MAX_VARIANT_DEPTH = 1000;
Review Comment:
Two small things. The spec defines no max nesting depth, so this 500 is an
implementation cap — a Spark/Flink writer with deeper nesting produces data
readable everywhere except iceberg-java. Matching parquet-java's 500 is the
right call, but I'd note in the Javadoc that it's a safety limit, not a spec
bound.
Separately, the downstream check is `depth <= MAX_VARIANT_DEPTH`, so 500
passes and 501 fails — 501 levels. The "up to MAX_VARIANT_DEPTH + 1 levels"
wording is technically right but easy to misread; `depth < MAX_VARIANT_DEPTH`
or a clearer constant name would read better. Minor either way.
##########
api/src/main/java/org/apache/iceberg/variants/SerializedArray.java:
##########
@@ -35,30 +35,47 @@ static SerializedArray from(VariantMetadata metadata,
byte[] bytes) {
return from(metadata,
ByteBuffer.wrap(bytes).order(ByteOrder.LITTLE_ENDIAN), bytes[0]);
}
+ @VisibleForTesting
Review Comment:
The 3-arg `from` gets `@VisibleForTesting` here but the analogous
`SerializedObject.from(metadata, value, header)` doesn't, and both are real
production paths reached from `from(byte[])` (and both are called directly from
tests). I'd drop it from both rather than annotate one — it reads as ad hoc
otherwise.
##########
api/src/main/java/org/apache/iceberg/variants/SerializedMetadata.java:
##########
@@ -59,15 +59,32 @@ static SerializedMetadata from(ByteBuffer metadata) {
private SerializedMetadata(ByteBuffer metadata, int header) {
this.isSorted = (header & SORTED_STRINGS) == SORTED_STRINGS;
this.offsetSize = 1 + ((header & OFFSET_SIZE_MASK) >> OFFSET_SIZE_SHIFT);
+ Preconditions.checkArgument(
+ metadata.remaining() >= HEADER_SIZE + offsetSize,
+ "Invalid variant metadata: buffer too small for dictionary size
field");
int dictSize = ByteBuffers.readLittleEndianUnsigned(metadata, HEADER_SIZE,
offsetSize);
- this.dict = new String[dictSize];
+ Preconditions.checkArgument(
+ dictSize >= 0, "Invalid variant metadata: negative dictionary size
%s", dictSize);
this.offsetListOffset = HEADER_SIZE + offsetSize;
+ long offsetTableEnd = (long) offsetListOffset + ((long) dictSize + 1L) *
offsetSize;
+ Preconditions.checkArgument(
+ offsetTableEnd <= metadata.remaining(),
+ "Invalid variant metadata: dictionary size %s exceeds buffer",
+ dictSize);
+ this.dict = new String[dictSize];
this.dataOffset = offsetListOffset + ((1 + dictSize) * offsetSize);
Review Comment:
This stayed plain `int` arithmetic while `SerializedArray` and
`SerializedObject` moved the equivalent assignment to
`Math.toIntExact(offsetTableEnd)`. It's safe today since `offsetTableEnd <=
remaining()` was just checked, but the asymmetry reads like an oversight and a
future reorder of these checks would silently reintroduce overflow here. I'd
use `Math.toIntExact(offsetTableEnd)` for consistency.
##########
api/src/test/java/org/apache/iceberg/variants/TestMalformedVariant.java:
##########
@@ -0,0 +1,342 @@
+/*
+ * 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.iceberg.variants;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import org.junit.jupiter.api.Test;
+
+public class TestMalformedVariant {
+
+ private static final ByteBuffer EMPTY_METADATA =
+ ByteBuffer.wrap(new byte[] {0x01, 0x00,
0x00}).order(ByteOrder.LITTLE_ENDIAN);
+
+ @Test
+ public void testOversizedMetadataDictSize() {
+ byte[] bytes = new byte[] {0x01, (byte) 0xFF};
+
+ assertThatThrownBy(() -> SerializedMetadata.from(bytes))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("dictionary size");
+ }
+
+ @Test
+ public void testUnsupportedMetadataVersionRejected() {
+ // header low 4 bits = version; spec version is 1. 0x02 = version 2, must
be rejected.
+ byte[] bytes = new byte[] {0x02, 0x00, 0x00};
+
+ assertThatThrownBy(() -> SerializedMetadata.from(bytes))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Unsupported version");
+ }
+
+ @Test
+ public void testOversizedArrayNumElements() {
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b10011, 0x00, 0x00, 0x00, 0x10,
0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testOversizedObjectNumElements() {
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b1000010, 0x00, 0x00, 0x00, 0x10,
0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testFieldIdOutOfRange() {
+ // field id check is lazy, so descend via get() to trigger it
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0x02, 0x01, 0x05, 0x00, 0x01, 0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ VariantValue top =
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value);
+
+ assertThatThrownBy(() -> top.asObject().get("anything"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("field id");
+ }
+
+ @Test
+ public void testOutOfRangeChildOffsetInArray() {
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0b0011, 0x01, (byte) 0xFF, 0x01, 0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ VariantValue top =
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value);
+
+ assertThatThrownBy(() -> top.asArray().get(0))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("out of data region");
+ }
+
+ @Test
+ public void testMalformedChildOffsetCaughtOnDescent() {
+ // outer array constructs cleanly. Inner element is rejected only when
accessed
+ ByteBuffer value =
+ ByteBuffer.wrap(
+ new byte[] {0b0011, 0x01, 0x00, 0x06, (byte) 0b10011, 0x00,
0x00, 0x00, 0x10, 0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ VariantValue top =
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value);
+
+ assertThatThrownBy(() -> top.asArray().get(0))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testNegativeMetadataEndOffset() {
+ byte[] metadata = {
+ (byte) 0b11000001, 0x00, 0x00, 0x00, 0x00, (byte) 0xFF, (byte) 0xFF,
(byte) 0xFF, (byte) 0xFF
+ };
+
+ assertThatThrownBy(() -> SerializedMetadata.from(metadata))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("negative end offset");
+ }
+
+ @Test
+ public void testNegativeDictOffsetInGet() {
+ byte[] metadata = {
+ (byte) 0b11000001,
+ 0x01,
+ 0x00,
+ 0x00,
+ 0x00,
+ (byte) 0xFF,
+ (byte) 0xFF,
+ (byte) 0xFF,
+ (byte) 0xFF,
+ 0x00,
+ 0x00,
+ 0x00,
+ 0x00
+ };
+
+ SerializedMetadata parsed = SerializedMetadata.from(metadata);
+
+ assertThatThrownBy(() -> parsed.get(0))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("dict entry");
+ }
+
+ @Test
+ public void testOversizedPrimitiveStringSize() {
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b1000000, (byte) 0xFF, (byte)
0xFF, (byte) 0xFF, 0x7F})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("payload");
+ }
+
+ @Test
+ public void testOffsetExceedsDeclaredDataLengthInObject() {
+ byte[] metadata = {0x01, 0x01, 0x00, 0x01, 'a'};
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0x02, 0x01, 0x00, 0x32, 0x00, 0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(metadata), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("declared data length");
+ }
+
+ @Test
+ public void testOffsetArithmeticOverflowInObject() {
+ // numElements * offsetSize would overflow signed int
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0x4E, 0x33, 0x33, 0x33, 0x33, (byte) 0xFF,
(byte) 0xFF, 0x3F})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testArrayDataOffsetIntOverflow() {
+ // large array, offsetSize=4, numElements=0x20000000 (~536M).
+ // (1+numElements)*offsetSize overflows signed int; long guard must fire
first.
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b11111, 0x00, 0x00, 0x00, 0x20,
0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testObjectDataOffsetIntOverflow() {
+ // large object, offsetSize=4, fieldIdSize=4, numElements=0x20000000
(~536M).
+ // int sub-expressions for offsetListOffset and dataOffset overflow; long
guard fires first.
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b1111110, 0x00, 0x00, 0x00, 0x20,
0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testNonMonotonicDictOffset() {
+ byte[] metadata = {0x01, 0x02, 0x00, 0x05, 0x03, 'A', 'B', 'C', 'D', 'E'};
+
+ SerializedMetadata parsed = SerializedMetadata.from(metadata);
+
+ assertThatThrownBy(() -> parsed.get(1))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("dict entry");
+ }
+
+ @Test
+ public void testDuplicateFieldOffsetsInObject() {
Review Comment:
This test locks in the duplicate-offset rejection I flagged in
`SerializedObject` — since that input is spec-legal, it's asserting the wrong
behavior. Once the assertion over there is dropped, this should flip to
asserting the object parses and both fields resolve to their (zero-length)
values.
##########
api/src/test/java/org/apache/iceberg/variants/TestMalformedVariant.java:
##########
@@ -0,0 +1,342 @@
+/*
+ * 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.iceberg.variants;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import org.junit.jupiter.api.Test;
+
+public class TestMalformedVariant {
+
+ private static final ByteBuffer EMPTY_METADATA =
+ ByteBuffer.wrap(new byte[] {0x01, 0x00,
0x00}).order(ByteOrder.LITTLE_ENDIAN);
+
+ @Test
+ public void testOversizedMetadataDictSize() {
+ byte[] bytes = new byte[] {0x01, (byte) 0xFF};
+
+ assertThatThrownBy(() -> SerializedMetadata.from(bytes))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("dictionary size");
+ }
+
+ @Test
+ public void testUnsupportedMetadataVersionRejected() {
+ // header low 4 bits = version; spec version is 1. 0x02 = version 2, must
be rejected.
+ byte[] bytes = new byte[] {0x02, 0x00, 0x00};
+
+ assertThatThrownBy(() -> SerializedMetadata.from(bytes))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Unsupported version");
+ }
+
+ @Test
+ public void testOversizedArrayNumElements() {
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b10011, 0x00, 0x00, 0x00, 0x10,
0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testOversizedObjectNumElements() {
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b1000010, 0x00, 0x00, 0x00, 0x10,
0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testFieldIdOutOfRange() {
+ // field id check is lazy, so descend via get() to trigger it
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0x02, 0x01, 0x05, 0x00, 0x01, 0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ VariantValue top =
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value);
+
+ assertThatThrownBy(() -> top.asObject().get("anything"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("field id");
+ }
+
+ @Test
+ public void testOutOfRangeChildOffsetInArray() {
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0b0011, 0x01, (byte) 0xFF, 0x01, 0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ VariantValue top =
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value);
+
+ assertThatThrownBy(() -> top.asArray().get(0))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("out of data region");
+ }
+
+ @Test
+ public void testMalformedChildOffsetCaughtOnDescent() {
+ // outer array constructs cleanly. Inner element is rejected only when
accessed
+ ByteBuffer value =
+ ByteBuffer.wrap(
+ new byte[] {0b0011, 0x01, 0x00, 0x06, (byte) 0b10011, 0x00,
0x00, 0x00, 0x10, 0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ VariantValue top =
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value);
+
+ assertThatThrownBy(() -> top.asArray().get(0))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testNegativeMetadataEndOffset() {
+ byte[] metadata = {
+ (byte) 0b11000001, 0x00, 0x00, 0x00, 0x00, (byte) 0xFF, (byte) 0xFF,
(byte) 0xFF, (byte) 0xFF
+ };
+
+ assertThatThrownBy(() -> SerializedMetadata.from(metadata))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("negative end offset");
+ }
+
+ @Test
+ public void testNegativeDictOffsetInGet() {
+ byte[] metadata = {
+ (byte) 0b11000001,
+ 0x01,
+ 0x00,
+ 0x00,
+ 0x00,
+ (byte) 0xFF,
+ (byte) 0xFF,
+ (byte) 0xFF,
+ (byte) 0xFF,
+ 0x00,
+ 0x00,
+ 0x00,
+ 0x00
+ };
+
+ SerializedMetadata parsed = SerializedMetadata.from(metadata);
+
+ assertThatThrownBy(() -> parsed.get(0))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("dict entry");
+ }
+
+ @Test
+ public void testOversizedPrimitiveStringSize() {
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b1000000, (byte) 0xFF, (byte)
0xFF, (byte) 0xFF, 0x7F})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("payload");
+ }
+
+ @Test
+ public void testOffsetExceedsDeclaredDataLengthInObject() {
+ byte[] metadata = {0x01, 0x01, 0x00, 0x01, 'a'};
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0x02, 0x01, 0x00, 0x32, 0x00, 0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(metadata), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("declared data length");
+ }
+
+ @Test
+ public void testOffsetArithmeticOverflowInObject() {
+ // numElements * offsetSize would overflow signed int
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0x4E, 0x33, 0x33, 0x33, 0x33, (byte) 0xFF,
(byte) 0xFF, 0x3F})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testArrayDataOffsetIntOverflow() {
+ // large array, offsetSize=4, numElements=0x20000000 (~536M).
+ // (1+numElements)*offsetSize overflows signed int; long guard must fire
first.
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b11111, 0x00, 0x00, 0x00, 0x20,
0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testObjectDataOffsetIntOverflow() {
+ // large object, offsetSize=4, fieldIdSize=4, numElements=0x20000000
(~536M).
+ // int sub-expressions for offsetListOffset and dataOffset overflow; long
guard fires first.
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {(byte) 0b1111110, 0x00, 0x00, 0x00, 0x20,
0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ assertThatThrownBy(() ->
VariantValue.from(SerializedMetadata.from(EMPTY_METADATA), value))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("element count");
+ }
+
+ @Test
+ public void testNonMonotonicDictOffset() {
+ byte[] metadata = {0x01, 0x02, 0x00, 0x05, 0x03, 'A', 'B', 'C', 'D', 'E'};
Review Comment:
The name implies a monotonicity check across the offset sequence, but what
actually fires is the per-entry `next >= offset` guard inside a single `get(1)`
— there's no assertion for an out-of-sequence-but-individually-valid pair like
`offset[0]=3, offset[1]=1`. I'd rename to something like
`testDictEntryEndBeforeStart`. While we're here, these hand-built metadata
arrays are hard to read cold — a one-line byte-layout comment (dictSize,
offsets, data) on this and the other constructed payloads would help a lot.
##########
api/src/main/java/org/apache/iceberg/variants/SerializedObject.java:
##########
@@ -163,9 +198,16 @@ public String next() {
private int id(int index) {
if (null == fieldIds[index]) {
- fieldIds[index] =
+ int dictSize = metadata.dictionarySize();
Review Comment:
The field-id range check only fires on the first `id(index)` call, so it's
driven lazily by `get()`/`fieldNames()` — parquet-java #3562 validates all
field ids eagerly in `validateContainerShallow` at construction. Not a
correctness bug, but it means a successfully constructed `SerializedObject`
doesn't guarantee its ids are in range, an invariant parquet-java callers can
rely on. I'd either move the check into the init loop or add a line noting
construction doesn't validate ids. Not blocking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
