You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nssalian (via GitHub)" <gi...@apache.org> on 2026/05/26 15:27:28 UTC
[PR] API: Harden variant binary parsing against malformed input [iceberg]
nssalian opened a new pull request, #16568:
URL: https://github.com/apache/iceberg/pull/16568
Closes [#16334](https://github.com/apache/iceberg/issues/16334)
Addresses [#16455](https://github.com/apache/iceberg/issues/16455)
## Summary
Validates malformed Variant binary input so adversarial buffers fail fast with `IllegalArgumentException` instead of OOM, `StackOverflowError`, or raw JVM exceptions. Adds bounds checks across header, count, offset table,
primitive payload, and short-string length, plus a 500-level nesting depth cap threaded through object/array recursive descent.
The approach mirrors Steve Loughran's parquet-java hardening work in [apache/parquet-java#3562](https://github.com/apache/parquet-java/pull/3562). Pulled in the changes from @steveloughran's [#16335](https://github.com/apache/iceberg/pull/16335)
## Changes
- `SerializedMetadata`, `SerializedArray`, `SerializedObject`: header / count / offset-table bounds, lazy per-element offset and field-id checks, long arithmetic to prevent int overflow.
- `SerializedPrimitive`: payload bounds, BIN/STR size-field bounds, non-negative size.
- `SerializedShortString`: length bounds.
- `VariantUtil`: `MAX_VARIANT_DEPTH = 500` and `fromBuffer` dispatch threading depth through recursive descent.
- `TestMalformedVariant`: 20 attack-payload tests covering each new check.
## Test plan
- [x] `./gradlew :iceberg-api:test --tests 'org.apache.iceberg.variants.*'`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #16568:
URL: https://github.com/apache/iceberg/pull/16568
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#issuecomment-4906752155
@laskoviymishka great comments here, would love the reviews on the parquet one as you know details that we may have missed. I don't want these parsing tests to be stricter than the spec and so reject valid files created by other tools.
@nssalian just added fixed depth to 1000 + two new tests
- you should skip an unknown primitive type. (not a MUST, just a SHOULD).
- it's ok for two dictionary keys to point to same offset (from the comments here)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#issuecomment-4963036512
Thanks for the review @laskoviymishka . Will keep track of the parquet PR. @steveloughran PTAL here before this goes in. I'll add follow ups.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#issuecomment-4812499350
claude looking at your tests after taking the ones from the suite that weren't in the parquet lib one and replicating
```
┌────────────────────────────────────────┬──────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Local test │ Why it's unique │
├────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────┤
│ testWellFormedRoundTrip │ Happy-path baseline. Iceberg's file is malformed-only, so it has no positive control. │
├────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────┤
│ testEmptyMetadataRejected │ Zero-byte metadata → IllegalArgumentException "empty". No empty-metadata case in the Iceberg 20. │
├────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────┤
│ testSingleByteMetadataRejected │ Lone version byte (below the 3-byte spec minimum) → "truncated". No analog. │
├────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────┤
│ testUnsupportedMetadataVersionRejected │ Version bits ≠ 1 → UnsupportedOperationException. Iceberg has no version-check 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3555510136
##########
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:
Fixed - dropped `@VisibleForTesting` from the 3-arg `from`; both `SerializedArray` and `SerializedObject` now leave it off.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3555512198
##########
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:
Flipped - renamed to `testSharedFieldOffsetsInObjectSucceeds`; it now asserts the object parses and both fields resolve to their zero-length NULL 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'};
+
+ 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:
Added `testMaxNestingAcceptedArray` and `testMaxNestingAcceptedObject` for the exact-boundary parse, plus `testOverdeepNestingRejectedObject` so the object descent path through the shared depth counter is exercised.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3483199088
##########
api/src/test/java/org/apache/iceberg/variants/TestMalformedVariant.java:
##########
@@ -0,0 +1,303 @@
+/*
+ * 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 {
Review Comment:
add a test to see how a different version is handled. it MUST be rejected
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -30,8 +32,38 @@ 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} nested levels.
+ */
+ static final int MAX_VARIANT_DEPTH = 500;
+
Review Comment:
I've started a discussion on the parquet dev list, please get involved. Looking at jackson, seems like 1000 is their limit right now, so copying that would be consistent. Let's see what the mailing list discussion outcome is though and we can all go with that, and add to the test dataset.
https://github.com/FasterXML/jackson-core/pull/943
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3531109931
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -31,8 +31,37 @@ 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} nested levels.
+ */
+ static final int MAX_VARIANT_DEPTH = 500;
+
private VariantUtil() {}
+ static VariantValue fromBuffer(VariantMetadata metadata, ByteBuffer value, int depth) {
Review Comment:
Done
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3555529419
##########
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:
Agreed, this is a genuine behavioral fix rather than just hardening - the old `endOffset`-vs-`limit` comparison was wrong for any buffer with a non-zero position. Called it out in the PR description as a separate change.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3531103474
##########
api/src/main/java/org/apache/iceberg/variants/SerializedArray.java:
##########
@@ -35,28 +35,44 @@ static SerializedArray from(VariantMetadata metadata, byte[] bytes) {
}
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 = VariantUtil.readLittleEndianUnsigned(value, HEADER_SIZE, numElementsSize);
+ Preconditions.checkArgument(
+ numElements >= 0, "Invalid variant array: negative element count %s", numElements);
this.offsetListOffset = HEADER_SIZE + numElementsSize;
+ long offsetTableEnd = (long) offsetListOffset + ((long) numElements + 1L) * offsetSize;
+ Preconditions.checkArgument(
+ offsetTableEnd <= value.remaining(),
+ "Invalid variant array: element count %s exceeds buffer",
+ numElements);
Review Comment:
Will be matching parquet-java's implicit "don't do that" stance. Happy to revisit if the dev list lands on rejecting trailing bytes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3531111689
##########
api/src/main/java/org/apache/iceberg/variants/SerializedArray.java:
##########
@@ -35,29 +35,46 @@ 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;
+ long offsetTableEnd = (long) offsetListOffset + ((long) numElements + 1L) * offsetSize;
+ Preconditions.checkArgument(
+ offsetTableEnd <= value.remaining(),
+ "Invalid variant array: element count %s exceeds buffer",
+ numElements);
this.dataOffset = offsetListOffset + ((1 + numElements) * offsetSize);
Review Comment:
Fixed - dataOffset = Math.toIntExact(offsetTableEnd), reusing the long value the guard already checks.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3410365438
##########
api/src/main/java/org/apache/iceberg/variants/SerializedArray.java:
##########
@@ -35,28 +35,44 @@ static SerializedArray from(VariantMetadata metadata, byte[] bytes) {
}
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) {
Review Comment:
Added a VisibleForTesting instead. I don't think it's good to go below package private here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#issuecomment-4996203325
@steveloughran @laskoviymishka I added a test for covering the unknown-primitive-type-id case. PTAL
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3555508917
##########
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:
Aligned `MAX_VARIANT_DEPTH` to 1000 following Steve's parquet dev-list thread, and flipped the guard to `depth <= MAX_VARIANT_DEPTH` so the accepted boundary matches parquet-java 3562 exactly (both allow depths 0..1000). Javadoc now marks it a safety limit, not a spec bound, with a cross-reference to 3562.
##########
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:
Fixed this too. I added an absolute `MAX_ELEMENTS` cap (16,777,216) checked before the array/object/metadata allocations. parquet-java 3562 has no equivalent (it bounds by buffer size only), so this one is iceberg-specific and I've noted that in the javadoc. Added `testExcessiveArrayNumElementsRejected` plus object and metadata variants.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3531113380
##########
api/src/main/java/org/apache/iceberg/variants/SerializedObject.java:
##########
@@ -60,18 +64,33 @@ static SerializedObject from(VariantMetadata metadata, ByteBuffer value, int hea
private final int[] offsets;
private final int[] lengths;
private final int dataOffset;
+ private final int depth;
private final VariantValue[] values;
- private SerializedObject(VariantMetadata metadata, ByteBuffer value, int header) {
+ private SerializedObject(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);
this.fieldIdSize = 1 + ((header & FIELD_ID_SIZE_MASK) >> FIELD_ID_SIZE_SHIFT);
int numElementsSize = ((header & IS_LARGE) == IS_LARGE) ? 4 : 1;
+ Preconditions.checkArgument(
+ value.remaining() >= HEADER_SIZE + numElementsSize,
+ "Invalid variant object: buffer too small for element count field");
int numElements = ByteBuffers.readLittleEndianUnsigned(value, HEADER_SIZE, numElementsSize);
+ Preconditions.checkArgument(
+ numElements >= 0, "Invalid variant object: negative element count %s", numElements);
this.fieldIdListOffset = HEADER_SIZE + numElementsSize;
- this.fieldIds = new Integer[numElements];
+ long dataStart =
+ (long) fieldIdListOffset
+ + (long) numElements * fieldIdSize
+ + ((long) numElements + 1L) * offsetSize;
+ Preconditions.checkArgument(
+ dataStart <= value.remaining(),
Review Comment:
Fixed - both offsets are now derived from a long dataStart intermediate and Math.toIntExact-ed, so the guard protects the values we use downstream.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3531115028
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -30,8 +32,38 @@ 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} nested levels.
+ */
+ static final int MAX_VARIANT_DEPTH = 500;
+
private VariantUtil() {}
+ /** Parses a variant value from {@code value} using {@code metadata} for field-name resolution. */
+ static VariantValue fromBuffer(VariantMetadata metadata, ByteBuffer value, int depth) {
+ Preconditions.checkArgument(
+ depth <= MAX_VARIANT_DEPTH,
+ "Invalid variant: nesting depth %s exceeds maximum %s",
Review Comment:
Fixed the doc to match: "top-level value is depth 0, so a Variant may contain up to MAX_VARIANT_DEPTH + 1 nested levels."
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3421232920
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -30,8 +32,38 @@ 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} nested levels.
+ */
+ static final int MAX_VARIANT_DEPTH = 500;
+
private VariantUtil() {}
+ /** Parses a variant value from {@code value} using {@code metadata} for field-name resolution. */
+ static VariantValue fromBuffer(VariantMetadata metadata, ByteBuffer value, int depth) {
+ Preconditions.checkArgument(
+ depth <= MAX_VARIANT_DEPTH,
+ "Invalid variant: nesting depth %s exceeds maximum %s",
Review Comment:
The Javadoc on `MAX_VARIANT_DEPTH` says a variant "may contain up to MAX_VARIANT_DEPTH nested levels," but `depth <= MAX_VARIANT_DEPTH` with the top-level at depth 0 actually admits 501 levels (depths 0 through 500). It's internally consistent, just off-by-one against the comment.
I'd either flip this to `depth < MAX_VARIANT_DEPTH` or update the Javadoc to state the counting model exactly. A `depth >= 0` lower bound here would also close the direct-call path where `depth + 1` could overflow past the cap.
##########
api/src/main/java/org/apache/iceberg/variants/SerializedArray.java:
##########
@@ -35,29 +35,46 @@ 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;
+ long offsetTableEnd = (long) offsetListOffset + ((long) numElements + 1L) * offsetSize;
+ Preconditions.checkArgument(
+ offsetTableEnd <= value.remaining(),
+ "Invalid variant array: element count %s exceeds buffer",
+ numElements);
this.dataOffset = offsetListOffset + ((1 + numElements) * offsetSize);
Review Comment:
`dataOffset` here is assigned with plain-`int` arithmetic, but we just computed the same bound in `long` as `offsetTableEnd` and guarded against it.
For `offsetSize = 4`, `(1 + numElements) * offsetSize` overflows `int` around numElements ~536M — a valid 4-byte count. The `long` guard above still passes, so `dataOffset` wraps negative and we hit a `ByteBuffer` underflow at access time instead of the `IllegalArgumentException` this check is meant to produce.
I'd just reuse the value we already have: `this.dataOffset = Math.toIntExact(offsetTableEnd)`. wdyt?
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -30,8 +32,38 @@ 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} nested levels.
+ */
+ static final int MAX_VARIANT_DEPTH = 500;
+
Review Comment:
This is a non-spec limit — the Parquet VariantEncoding spec doesn't cap nesting depth and Iceberg defers to it. A valid variant with >500 levels written by Spark or another client would read fine elsewhere but throw here at scan time, which reads as silent data loss rather than malformed-input rejection.
What did parquet-java#3562 settle on? If it picked a different value (or no cap), we'd diverge on file interop. I'd at minimum cross-reference that decision in a comment here and confirm 500 is high enough to never reject real data (it's below Jackson's default of 2000).
##########
api/src/main/java/org/apache/iceberg/variants/SerializedObject.java:
##########
@@ -60,18 +64,33 @@ static SerializedObject from(VariantMetadata metadata, ByteBuffer value, int hea
private final int[] offsets;
private final int[] lengths;
private final int dataOffset;
+ private final int depth;
private final VariantValue[] values;
- private SerializedObject(VariantMetadata metadata, ByteBuffer value, int header) {
+ private SerializedObject(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);
this.fieldIdSize = 1 + ((header & FIELD_ID_SIZE_MASK) >> FIELD_ID_SIZE_SHIFT);
int numElementsSize = ((header & IS_LARGE) == IS_LARGE) ? 4 : 1;
+ Preconditions.checkArgument(
+ value.remaining() >= HEADER_SIZE + numElementsSize,
+ "Invalid variant object: buffer too small for element count field");
int numElements = ByteBuffers.readLittleEndianUnsigned(value, HEADER_SIZE, numElementsSize);
+ Preconditions.checkArgument(
+ numElements >= 0, "Invalid variant object: negative element count %s", numElements);
this.fieldIdListOffset = HEADER_SIZE + numElementsSize;
- this.fieldIds = new Integer[numElements];
+ long dataStart =
+ (long) fieldIdListOffset
+ + (long) numElements * fieldIdSize
+ + ((long) numElements + 1L) * offsetSize;
+ Preconditions.checkArgument(
+ dataStart <= value.remaining(),
Review Comment:
Same int-overflow as the array side: `offsetListOffset` and `dataOffset` below are assigned in plain `int` while `dataStart` here is `long`. `numElements * fieldIdSize` and `(1 + numElements) * offsetSize` can overflow before the `long` guard's bound is ever applied to the int fields, wrapping the data offset negative for a large-but-valid count.
I'd derive both offsets from the `long` intermediates and `Math.toIntExact` them so the guard actually protects the value we use.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3483190399
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -30,8 +32,38 @@ 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} nested levels.
+ */
+ static final int MAX_VARIANT_DEPTH = 500;
+
Review Comment:
no real reason, except it was what was in org.apache.parquet.variant.VariantJsonParser
how about we discuss on parquet dev list?
I don't *really* care, just that there's some limit and that it's broadly consistent.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3538410511
##########
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:
IMO rust does it best as it does it on demand, and only when needed, but that's the kind of change you need to design in.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3555513795
##########
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:
Renamed to `testDictEntryEndBeforeStart` to match what actually fires (the per-entry `next >= offset` guard). Added one-line byte-layout comments (dictSize, offsets, data) on this and the other hand-built payloads.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3555507375
##########
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:
Added a comment noting construction doesn't validate field ids (they're checked lazily on first access). Agree the on-demand approach rust takes reads best, but that's a larger redesign.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3555504226
##########
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:
Fixed this too. I added an absolute `MAX_ELEMENTS` cap (16,777,216) checked before the array/object/metadata allocations. parquet-java #3562 has no equivalent (it bounds by buffer size only), so this one is iceberg-specific and I've noted that in the javadoc. Added `testExcessiveArrayNumElementsRejected` plus object and metadata variants.
##########
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:
It now uses `Math.toIntExact(offsetTableEnd)`, consistent with `SerializedArray` and `SerializedObject`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3555502542
##########
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:
Great catch. I dropped the uniqueness assertion and switched to a sorted-offset pass that tolerates equal offsets (zero-length spans). The per-field bounds check just above still guarantees every offset lands in `[0, dataLength]`, which is what the spec requires.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3531228499
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -30,8 +32,38 @@ 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} nested levels.
+ */
+ static final int MAX_VARIANT_DEPTH = 500;
+
Review Comment:
Following the parquet dev list thread Steve started: [DISCUSS] Variant Hardening: how deep is a realistic variant depth? (https://lists.apache.org/thread/q6wbom1q9pndv2nj6wcynxjcjxxkc1hm). I have aligned MAX_VARIANT_DEPTH to 1000 (jackson's 1000 seems the likely outcome).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3538346128
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -30,8 +32,38 @@ 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} nested levels.
+ */
+ static final int MAX_VARIANT_DEPTH = 500;
+
Review Comment:
I've set the parquet to 1000 too, so we are all now consistent
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Harden variant binary parsing against malformed input [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #16568:
URL: https://github.com/apache/iceberg/pull/16568#discussion_r3310358921
##########
api/src/main/java/org/apache/iceberg/variants/SerializedArray.java:
##########
@@ -35,28 +35,44 @@ static SerializedArray from(VariantMetadata metadata, byte[] bytes) {
}
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) {
Review Comment:
could this be private?
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -31,8 +31,37 @@ 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} nested levels.
+ */
+ static final int MAX_VARIANT_DEPTH = 500;
+
private VariantUtil() {}
+ static VariantValue fromBuffer(VariantMetadata metadata, ByteBuffer value, int depth) {
Review Comment:
add a javadoc, mention that the function includes validation, including depth limits
##########
api/src/main/java/org/apache/iceberg/variants/SerializedArray.java:
##########
@@ -35,28 +35,44 @@ static SerializedArray from(VariantMetadata metadata, byte[] bytes) {
}
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 = VariantUtil.readLittleEndianUnsigned(value, HEADER_SIZE, numElementsSize);
+ Preconditions.checkArgument(
+ numElements >= 0, "Invalid variant array: negative element count %s", numElements);
this.offsetListOffset = HEADER_SIZE + numElementsSize;
+ long offsetTableEnd = (long) offsetListOffset + ((long) numElements + 1L) * offsetSize;
+ Preconditions.checkArgument(
+ offsetTableEnd <= value.remaining(),
+ "Invalid variant array: element count %s exceeds buffer",
+ numElements);
Review Comment:
I've just realised that this and the parquet-java hardening don't worry about leftover data. "don't do that" is implicit the policy there, being as it is useless.
I wonder what the rust parquet reader does.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org