steveloughran commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3602738105
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -30,8 +32,45 @@ class VariantUtil {
private static final int BASIC_TYPE_OBJECT = 2;
private static final int BASIC_TYPE_ARRAY = 3;
+ /**
+ * Maximum nesting depth in a Variant (permitted depths
0..MAX_VARIANT_DEPTH). Safety limit, not a
+ * spec bound. Matches parquet-java (apache/parquet-java#3562).
+ */
+ static final int MAX_VARIANT_DEPTH = 1000;
+
+ /**
+ * Maximum element count for Variant containers and metadata dictionaries.
Safety limit against
+ * buffer-to-heap allocation amplification.
+ */
+ static final int MAX_ELEMENTS = 16_777_216;
+
private VariantUtil() {}
+ /** Parses a variant value; validates input and enforces {@link
#MAX_VARIANT_DEPTH}. */
+ static VariantValue fromBuffer(VariantMetadata metadata, ByteBuffer value,
int depth) {
+ Preconditions.checkArgument(depth >= 0, "Invalid variant: negative depth
%s", depth);
+ Preconditions.checkArgument(
+ depth <= MAX_VARIANT_DEPTH,
+ "Invalid variant: nesting depth %s exceeds maximum %s",
+ depth,
+ MAX_VARIANT_DEPTH);
+ Preconditions.checkArgument(value.remaining() >= 1, "Invalid variant:
empty value buffer");
+ int header = ByteBuffers.readByte(value, 0);
+ BasicType basicType = basicType(header);
+ switch (basicType) {
+ case PRIMITIVE:
Review Comment:
what about java17 switch now there's been the move and this is new, or at
least moved, code?
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -30,8 +32,45 @@ class VariantUtil {
private static final int BASIC_TYPE_OBJECT = 2;
private static final int BASIC_TYPE_ARRAY = 3;
+ /**
+ * Maximum nesting depth in a Variant (permitted depths
0..MAX_VARIANT_DEPTH). Safety limit, not a
+ * spec bound. Matches parquet-java (apache/parquet-java#3562).
+ */
+ static final int MAX_VARIANT_DEPTH = 1000;
+
+ /**
+ * Maximum element count for Variant containers and metadata dictionaries.
Safety limit against
+ * buffer-to-heap allocation amplification.
+ */
+ static final int MAX_ELEMENTS = 16_777_216;
Review Comment:
the parquet pr doesn't enforce any limit here, and I'm not going to worry
about one; the cost of array size is less than recursing down nested structures
and I'm not aware of other bits of parquet imposing limits other than "you are
free to run out of memory if you want to"
##########
api/src/test/java/org/apache/iceberg/variants/TestMalformedVariant.java:
##########
@@ -0,0 +1,519 @@
+/*
+ * 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.assertThat;
+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() {
+ // metadata: [0x01 header - v1, offsetSize=1] [0xFF dictSize=255] - buffer
stops before offsets
+ 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() {
+ // array: [0b10011 header - large, offsetSize=1] [4-byte
numElements=0x00100000 (1M)]
+ // buffer stops before the offset table can be read
+ 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() {
+ // [0b1000010 hdr - large object] [4-byte numElements=0x00100000] - buffer
stops before tables
+ 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() {
+ // [0x02 hdr] [0x01 numEl] [0x05 fieldId - out of range in empty dict]
[0x00,0x01 offsets]
+ // [0x00 data]; range check is lazy so descend via get() to trigger
+ 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() {
+ // [0b0011 hdr - small array, offsetSize=1] [0x01 numEl] [0xFF,0x01
offsets - end<start]
+ 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 [0b0011 hdr] [0x01 numEl] [0x00,0x06 offsets]; inner large-array
at byte 4
+ // has numElements=0x00100000 - rejected only on descent, not at outer
parse
+ 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() {
+ // metadata: [0b11000001 header - v1, offsetSize=4] [4-byte dictSize=0]
+ // [0xFFFFFFFF end offset as int32 = -1]
+ 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() {
+ // metadata: [0b11000001 header - v1, offsetSize=4] [4-byte dictSize=1]
+ // [4-byte offset[0]=0xFFFFFFFF (= -1)] [4-byte offset[1]=0]
+ 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() {
+ // primitive: [0b1000000 header - STRING primitive]
+ // [0x7FFFFFFF as 4-byte size = 2GB claimed payload]
+ 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() {
+ // metadata: [0x01 hdr] [0x01 dictSize] [0x00,0x01 offsets] ['a']
+ byte[] metadata = {0x01, 0x01, 0x00, 0x01, 'a'};
+ // object: [0x02][0x01][0x00 fieldId][0x32 offset[0]=50 >
dataLength=0][0x00][0x00]
+ 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() {
+ // object: [0x4E header - large, fieldIdSize=1, offsetSize=4] [4-byte
numElements=0x33333333]
+ // numElements * offsetSize overflows signed int; long guard fires first
+ 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() {
+ // array: [0b11111 header - large, offsetSize=4] [4-byte
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() {
+ // [0b1111110 hdr - large object, fieldIdSize=4, offsetSize=4] [4-byte
numElements=0x20000000]
+ // offsetListOffset and dataOffset int expressions 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 testMetadataDataOffsetIntOverflow() {
+ // metadata: [0b11000001 header - v1, offsetSize=4] [4-byte
dictSize=0x20000000 (~536M)]
+ // (1+dictSize)*offsetSize overflows signed int; long guard fires first.
+ byte[] metadata = new byte[] {(byte) 0b11000001, 0x00, 0x00, 0x00, 0x20};
+
+ assertThatThrownBy(() -> SerializedMetadata.from(metadata))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("dictionary size");
+ }
+
+ @Test
+ public void testDictEntryEndBeforeStart() {
+ // metadata: [0x01 header] [0x02 dictSize] [0x00,0x05,0x03 offsets - entry
1 ends BEFORE
+ // it starts] ['A','B','C','D','E' data]
+ 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 testSharedFieldOffsetsInObjectSucceeds() {
+ // Shared offset is spec-legal (compacting writer dedups).
+ // metadata: [0x01 hdr] [0x02 dictSize] [0x00,0x01,0x02 offsets] ['a','b']
+ byte[] metadata = {0x01, 0x02, 0x00, 0x01, 0x02, 'a', 'b'};
+ // object: [0x02 hdr] [0x02 numEl] [0x00,0x01 fieldIds] [0x00,0x00,0x01
offsets] [0x00 NULL]
+ ByteBuffer value =
+ ByteBuffer.wrap(new byte[] {0x02, 0x02, 0x00, 0x01, 0x00, 0x00, 0x01,
0x00})
+ .order(ByteOrder.LITTLE_ENDIAN);
+
+ VariantValue top = VariantValue.from(SerializedMetadata.from(metadata),
value);
+ VariantObject obj = top.asObject();
+
+ assertThat(obj.numFields()).isEqualTo(2);
+ assertThat(obj.get("a").type()).isEqualTo(PhysicalType.NULL);
+ assertThat(obj.get("b").type()).isEqualTo(PhysicalType.NULL);
+ }
+
+ @Test
+ public void testEmptyChildValueBufferInArray() {
Review Comment:
missed this one; lifting it for the parquet test.
--
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]
