From d0c5febc627b6d441052e5c4ff9e0ec3d0183bee Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Tue, 21 Jul 2026 15:01:29 +0100 Subject: [PATCH 01/12] TestVariantScalar moves to junit5 --- parquet-variant/pom.xml | 11 + .../org/apache/parquet/variant/Variant.java | 76 +- .../parquet/variant/VariantJsonParser.java | 4 +- .../apache/parquet/variant/VariantUtil.java | 158 ++++ .../parquet/variant/TestHardenedReader.java | 746 ++++++++++++++++++ .../parquet/variant/TestVariantArray.java | 23 +- .../variant/TestVariantArrayBuilder.java | 5 +- .../parquet/variant/TestVariantObject.java | 7 +- .../variant/TestVariantObjectBuilder.java | 5 +- .../parquet/variant/TestVariantScalar.java | 80 +- .../parquet/variant/VariantTestUtil.java | 3 +- 11 files changed, 1034 insertions(+), 84 deletions(-) create mode 100644 parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java diff --git a/parquet-variant/pom.xml b/parquet-variant/pom.xml index 26d4d5b537..5a1996f87a 100644 --- a/parquet-variant/pom.xml +++ b/parquet-variant/pom.xml @@ -69,6 +69,17 @@ ${slf4j.version} test + + org.apache.parquet + parquet-hadoop + ${project.version} + test + + + org.apache.hadoop + hadoop-client + test + diff --git a/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java b/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java index 3fdfc00605..2c97e824a4 100644 --- a/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java +++ b/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java @@ -61,6 +61,8 @@ public final class Variant { * Lazy cache for the parsed array header. */ private VariantUtil.ArrayInfo cachedArrayInfo; + /** Nesting depth of this Variant relative to the top-level value (0 = top-level). */ + private final int depth; /** * The threshold to switch from linear search to binary search when looking up a field by key in @@ -69,10 +71,31 @@ public final class Variant { */ static final int BINARY_SEARCH_THRESHOLD = 32; + /** + * Create a Variant from a tuple of (value, metadata) byte arrays. + * Includes validation of the version and the top-level structure. + * @param value value buffer + * @param metadata metadata buffer + * @throws UnsupportedOperationException if there is a version mismatch + * @throws IllegalArgumentException for any validation failure. + */ public Variant(byte[] value, byte[] metadata) { this(value, 0, value.length, metadata, 0, metadata.length); } + /** + * Create a Variant from a subset of the (value, metadata) buffers + * supplied. + * Includes validation of the version and the toplevel structure. + * @param value value buffer + * @param valuePos offset where the value data begins + * @param valueLength length of value data + * @param metadata metadata buffer + * @param metadataPos offset where the metadata begins. + * @param metadataLength length of the metadata. + * @throws UnsupportedOperationException if there is a version mismatch + * @throws IllegalArgumentException for any validation failure. + */ public Variant(byte[] value, int valuePos, int valueLength, byte[] metadata, int metadataPos, int metadataLength) { this(ByteBuffer.wrap(value, valuePos, valueLength), ByteBuffer.wrap(metadata, metadataPos, metadataLength)); } @@ -81,10 +104,20 @@ public Variant(byte[] value, int valuePos, int valueLength, byte[] metadata, int this(value, metadata.getEncodedBuffer()); } + /** + * Create a Variant from a tuple of (value, metadata) buffers. + * Includes validation of the version and the toplevel structure. + * @param value value buffer + * @param metadata metadata buffer + * @throws UnsupportedOperationException if there is a version mismatch + * @throws IllegalArgumentException for any validation failure. + */ public Variant(ByteBuffer value, ByteBuffer metadata) { this.value = value.asReadOnlyBuffer().order(ByteOrder.LITTLE_ENDIAN); this.metadata = metadata.asReadOnlyBuffer().order(ByteOrder.LITTLE_ENDIAN); - + this.depth = 0; + // A metadata buffer must contain at least the version byte + Preconditions.checkArgument(this.metadata.remaining() >= 1, "variant metadata is empty"); // There is currently only one allowed version. if ((metadata.get(metadata.position()) & VariantUtil.VERSION_MASK) != VariantUtil.VERSION) { throw new UnsupportedOperationException(String.format( @@ -95,29 +128,39 @@ public Variant(ByteBuffer value, ByteBuffer metadata) { // Pre-compute dictionary size for lazy metadata cache allocation. int pos = this.metadata.position(); int metaOffsetSize = ((this.metadata.get(pos) >> 6) & 0x3) + 1; - if (this.metadata.remaining() > 1) { - Preconditions.checkArgument( - this.metadata.remaining() >= 1 + metaOffsetSize, - "variant metadata truncated: offsetSize=" + metaOffsetSize); - this.dictSize = VariantUtil.readUnsignedLittleEndian(this.metadata, pos + 1, metaOffsetSize); - long dictTableEnd = 1L + metaOffsetSize + ((long) this.dictSize + 1) * metaOffsetSize; - Preconditions.checkArgument( - dictTableEnd <= this.metadata.remaining(), - "variant metadata dictionary extends past buffer: dictSize=" + this.dictSize); - } else { - this.dictSize = 0; - } + Preconditions.checkArgument( + this.metadata.remaining() >= 1 + metaOffsetSize, + "variant metadata truncated: offsetSize=" + metaOffsetSize); + this.dictSize = VariantUtil.readUnsignedLittleEndian(this.metadata, pos + 1, metaOffsetSize); + long dictTableEnd = 1L + metaOffsetSize + ((long) this.dictSize + 1) * metaOffsetSize; + Preconditions.checkArgument( + dictTableEnd <= this.metadata.remaining(), + "variant metadata dictionary extends past buffer: dictSize=" + this.dictSize); this.metadataCache = null; + VariantUtil.validateValueShallow(this.value, dictSize); } /** - * Package-private constructor that shares pre-parsed metadata state from a parent Variant. + * Package-private constructor that shares pre-parsed metadata state from a parent Variant, performing shallow validation of the structure. + * @param value value buffer + * @param metadata metadata buffer + * @param metadataCache shared metadata cache. + * @param dictSize shared dictionary size. + * @param depth depth of this variant in a recursive structure. + * @throws IllegalArgumentException if the depth of variants is too high or the structure + * invalid in some other form. */ - Variant(ByteBuffer value, ByteBuffer metadata, String[] metadataCache, int dictSize) { + Variant(ByteBuffer value, ByteBuffer metadata, String[] metadataCache, int dictSize, int depth) { + Preconditions.checkArgument( + depth <= VariantUtil.MAX_VARIANT_DEPTH, + "variant nesting depth exceeds maximum %s", + VariantUtil.MAX_VARIANT_DEPTH); this.value = value.asReadOnlyBuffer().order(ByteOrder.LITTLE_ENDIAN); this.metadata = metadata.asReadOnlyBuffer().order(ByteOrder.LITTLE_ENDIAN); this.metadataCache = metadataCache; this.dictSize = dictSize; + this.depth = depth; + VariantUtil.validateValueShallow(this.value, dictSize); } public ByteBuffer getValueBuffer() { @@ -359,9 +402,10 @@ public Variant getElementAtIndex(int index) { /** * Creates a child Variant that shares this instance's metadata cache. + * @throws IllegalArgumentException validation error during construction. */ private Variant childVariant(ByteBuffer childValue) { - return new Variant(childValue, metadata, metadataCache, dictSize); + return new Variant(childValue, metadata, metadataCache, dictSize, depth + 1); } /** diff --git a/parquet-variant/src/main/java/org/apache/parquet/variant/VariantJsonParser.java b/parquet-variant/src/main/java/org/apache/parquet/variant/VariantJsonParser.java index 05551c26d5..c5cd9a9058 100644 --- a/parquet-variant/src/main/java/org/apache/parquet/variant/VariantJsonParser.java +++ b/parquet-variant/src/main/java/org/apache/parquet/variant/VariantJsonParser.java @@ -16,6 +16,8 @@ */ package org.apache.parquet.variant; +import static org.apache.parquet.variant.VariantUtil.MAX_VARIANT_DEPTH; + import com.fasterxml.jackson.core.JsonFactory; import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.core.JsonParser; @@ -37,7 +39,7 @@ public final class VariantJsonParser { private static final JsonFactory JSON_FACTORY = JsonFactory.builder() .streamReadConstraints(StreamReadConstraints.builder() - .maxNestingDepth(500) + .maxNestingDepth(MAX_VARIANT_DEPTH) .maxStringLength(10_000_000) .maxDocumentLength(50_000_000L) .build()) diff --git a/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java b/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java index ad7165fcfe..8625cdc8e3 100644 --- a/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java +++ b/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java @@ -190,6 +190,12 @@ class VariantUtil { // The size (in bytes) of a UUID. static final int UUID_SIZE = 16; + /** + * Maximum permitted nesting depth of a Variant value. + * same limit as in VariantJsonParser. + */ + static final int MAX_VARIANT_DEPTH = 1000; + // header bytes static final byte HEADER_NULL = primitiveHeader(NULL); static final byte HEADER_LONG_STRING = primitiveHeader(LONG_STR); @@ -880,6 +886,158 @@ static HashMap getMetadataMap(ByteBuffer metadata) { return result; } + /** + * Bounds-checks a single Variant value node against its buffer slot. Performs no recursion + * into nested children: child nodes are checked on demand when callers descend into them. + * + *

Cost: O(1) for primitives and short strings, O(numElements) for objects and arrays. + * Validation of nested structures is deferred so that opening a large well-formed Variant + * is not penalized by sub-trees the caller never inspects. + * + * @param valueBuffer the variant value buffer (position/limit define the extent of this node's slot) + * @param dictSize the metadata dictionary size, used to bound object field ids + * @throws IllegalArgumentException if the value header or container table does not fit within + * the buffer slot, or if any object field id is out of range + */ + static void validateValueShallow(final ByteBuffer valueBuffer, final int dictSize) { + int pos = valueBuffer.position(); + Preconditions.checkArgument(pos >= 0 && pos < valueBuffer.limit(), "variant value is empty"); + long slot = (long) valueBuffer.limit() - pos; + int header = valueBuffer.get(pos) & 0xFF; + int basicType = header & BASIC_TYPE_MASK; + int typeInfo = (header >> BASIC_TYPE_BITS) & PRIMITIVE_TYPE_MASK; + switch (basicType) { + case SHORT_STR: + Preconditions.checkArgument(1L + typeInfo <= slot, "variant short string extends past buffer"); + return; + case OBJECT: + validateContainerShallow(valueBuffer, typeInfo, pos, slot, dictSize, true); + return; + case ARRAY: + validateContainerShallow(valueBuffer, typeInfo, pos, slot, dictSize, false); + return; + default: + validatePrimitiveShallow(valueBuffer, typeInfo, pos, slot); + } + } + + /** + * Shallow validation of a container. + * + * @param valueBuffer buffer with the variant data + * @param typeInfo type information from the metadata + * @param pos buffer read position + * @param length length of slot for this primitive. + * @param dictSize dictionary size + * @param isObject is this an object? + */ + private static void validateContainerShallow( + final ByteBuffer valueBuffer, + final int typeInfo, + final int pos, + final long length, + final int dictSize, + final boolean isObject) { + boolean largeSize; + int idSize; + if (isObject) { + largeSize = ((typeInfo >> 4) & 0x1) != 0; + idSize = ((typeInfo >> 2) & 0x3) + 1; + } else { + largeSize = ((typeInfo >> 2) & 0x1) != 0; + idSize = 0; + } + int offsetSize = (typeInfo & 0x3) + 1; + int sizeBytes = largeSize ? U32_SIZE : 1; + Preconditions.checkArgument(1L + sizeBytes <= length, "variant container header truncated"); + int numElements = readUnsignedLittleEndian(valueBuffer, pos + 1, sizeBytes); + long idStart = 1L + sizeBytes; + long idBytes = isObject ? (long) numElements * idSize : 0L; + long offsetStart = idStart + idBytes; + long offsetBytes = (long) (numElements + 1) * offsetSize; + long dataStart = offsetStart + offsetBytes; + Preconditions.checkArgument( + dataStart <= length, "variant container offset table extends past buffer: numElements=%s", numElements); + long dataLen = length - dataStart; + if (isObject) { + for (int i = 0; i < numElements; i++) { + int id = readUnsignedLittleEndian(valueBuffer, pos + (int) idStart + i * idSize, idSize); + Preconditions.checkArgument( + id < dictSize, "variant object key id %s out of range (dictSize=%s)", id, dictSize); + } + } + // Each child offset must lie within the data region. Children may overlap or leave gaps; + // the trailing terminator offset is range-checked for the same reason. + for (int i = 0; i <= numElements; i++) { + // O(elements) + int off = readUnsignedLittleEndian(valueBuffer, pos + (int) offsetStart + i * offsetSize, offsetSize); + Preconditions.checkArgument( + off <= dataLen, "variant child offset out of range: %s (data length %s)", off, dataLen); + } + } + + /** + * Validate the primitive type. The buffer must have enough capacity + * for the given type (and of course, the type must be recognized). + * + * @param valueBuffer buffer with the variant data + * @param typeInfo type information from the metadata + * @param pos buffer read position + * @param length length of slot for this primitive. + */ + private static void validatePrimitiveShallow( + final ByteBuffer valueBuffer, final int typeInfo, final int pos, final long length) { + long size; + switch (typeInfo) { + case NULL: + case TRUE: + case FALSE: + size = 1; + break; + case INT8: + size = 2; + break; + case INT16: + size = 3; + break; + case INT32: + case DATE: + case FLOAT: + size = 5; + break; + case INT64: + case DOUBLE: + case TIMESTAMP_TZ: + case TIMESTAMP_NTZ: + case TIME: + case TIMESTAMP_NANOS_TZ: + case TIMESTAMP_NANOS_NTZ: + size = 9; + break; + case DECIMAL4: + size = 6; + break; + case DECIMAL8: + size = 10; + break; + case DECIMAL16: + size = 18; + break; + case BINARY: + case LONG_STR: { + Preconditions.checkArgument(1L + U32_SIZE <= length, "variant string/binary length field truncated"); + size = 1L + U32_SIZE + readUnsigned(valueBuffer, pos + 1, U32_SIZE); + break; + } + case UUID: + size = 1L + UUID_SIZE; + break; + default: + throw new IllegalArgumentException(String.format("Unknown primitive type in variant: %d", typeInfo)); + } + Preconditions.checkArgument(size <= length, "variant value extends past buffer"); + } + /** * Computes the actual size (in bytes) of the Variant value. * @param value The Variant value binary diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java new file mode 100644 index 0000000000..813fe99e83 --- /dev/null +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java @@ -0,0 +1,746 @@ +/* + * 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.parquet.variant; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import java.io.ByteArrayOutputStream; +import java.io.EOFException; +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import org.apache.parquet.example.data.Group; +import org.apache.parquet.example.data.simple.SimpleGroup; +import org.apache.parquet.hadoop.ParquetReader; +import org.apache.parquet.hadoop.ParquetWriter; +import org.apache.parquet.hadoop.api.ReadSupport; +import org.apache.parquet.hadoop.example.ExampleParquetWriter; +import org.apache.parquet.hadoop.example.GroupReadSupport; +import org.apache.parquet.hadoop.metadata.CompressionCodecName; +import org.apache.parquet.io.InputFile; +import org.apache.parquet.io.OutputFile; +import org.apache.parquet.io.PositionOutputStream; +import org.apache.parquet.io.SeekableInputStream; +import org.apache.parquet.io.api.Binary; +import org.apache.parquet.schema.MessageType; +import org.apache.parquet.schema.MessageTypeParser; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TestName; + +/** + * Exercises the bounds checks added to {@link Variant} construction by round-tripping a row of + * {@code (metadata, value)} binaries through an in-memory uncompressed Parquet file, mutating + * specific bytes after writing, and asserting that the resulting buffer is rejected with an + * {@link IllegalArgumentException}. + */ +public class TestHardenedReader { + + private static final String SCHEMA_STRING = + "message variant_record { required binary metadata; required binary value; }"; + + private static final MessageType SCHEMA = MessageTypeParser.parseMessageType(SCHEMA_STRING); + + /** + * Minimal well-formed empty metadata: version=1, offsetSize=1, dictSize=0, single end-offset=0. + */ + private static final byte[] EMPTY_METADATA = {0x01, 0x00, 0x00}; + + /** + * Set to {@code true} to persist the generated Parquet files under + * {@code target/test-hardened-reader/$testName.parquet}. + * This allows file to be added to the bad_data section of parquet-testing repository. + */ + private static final boolean SAVE_GENERATED_FILES = false; + + /** Directory under the module's build dir where generated files are written, if enabled. */ + private static final Path OUTPUT_DIR = Paths.get("target", "test-hardened-reader"); + + @Rule + public TestName testName = new TestName(); + + /** + * Happy path. + */ + @Test + public void testWellFormedRoundTrip() throws IOException { + byte[] wellFormed = arrayOneNull(); + byte[][] roundTripped = roundTrip(EMPTY_METADATA, wellFormed); + Variant v = new Variant(ByteBuffer.wrap(roundTripped[1]), ByteBuffer.wrap(roundTripped[0])); + assertEquals(Variant.Type.ARRAY, v.getType()); + assertEquals(1, v.numArrayElements()); + assertEquals(Variant.Type.NULL, v.getElementAtIndex(0).getType()); + } + + /** + * Out-of-range child offset. + */ + @Test + public void testOutOfRangeChildOffset() throws IOException { + byte[] value = arrayOneNull(); + // Layout of arrayOneNull(): [header, numElements=1, offset[0]=0, offset[1]=1, NULL_header] + // Set offset[0] to 0xFF — far outside the 1-byte data region. + value[2] = (byte) 0xFF; + expectRoundTripRaisesIllegalArgument(EMPTY_METADATA, value, "child offset"); + } + + /** + * Reject numElements larger than the surrounding buffer. + */ + @Test + public void testOversizedNumElements() throws IOException { + // Use a largeSize array so numElements is a 4-byte field we can grow. + // Layout: [arrayHeader(large=1,offsetSize=1)=0b10011, numElements(4 bytes LE)=0, + // end-offset(1 byte)=0] — total 6 bytes, zero data. + byte[] value = new byte[] {0b10011, 0x00, 0x00, 0x00, 0x00, 0x00}; + // Baseline must parse cleanly. + new Variant(ByteBuffer.wrap(value.clone()), ByteBuffer.wrap(EMPTY_METADATA.clone())); + // Set numElements to a value whose offset table cannot fit in the buffer. + int huge = 0x10000000; + value[1] = (byte) (huge & 0xFF); + value[2] = (byte) ((huge >> 8) & 0xFF); + value[3] = (byte) ((huge >> 16) & 0xFF); + value[4] = (byte) ((huge >> 24) & 0xFF); + expectRoundTripRaisesIllegalArgument(EMPTY_METADATA, value, "offset table"); + } + + /** + * Reject Offset arithmetic that would wrap java's signed 32-bit integer. + */ + @Test + public void testOffsetArithmeticBoundsCheck() throws IOException { + // Object header that claims numElements and offsetSize values whose product overflows int + // unless validated with widened arithmetic. + // Layout: [objectHeader(large=1, idSize=1, offsetSize=4) = 0x4E, + // numElements (4 bytes, little-endian) = 0x33333333, + // three filler bytes] + byte[] value = new byte[] {0x4E, 0x33, 0x33, 0x33, 0x33, (byte) 0xFF, (byte) 0xFF, 0x3F}; + expectRoundTripRaisesIllegalArgument(EMPTY_METADATA, value, ""); + } + + /** + * Well-formed outer array containing a malformed inner array: the outer header table is + * consistent with the buffer extent and is accepted at top-level construction, but the inner + * child is rejected only when the caller descends into it. + */ + @Test + public void testMalformedChildInsideWellFormedParent() throws IOException { + // Outer layout (offsetSize=1, smallSize, 1 element): + // [0] 0b0011 arrayHeader(largeSize=false, offsetSize=1) + // [1] 0x01 numElements = 1 + // [2] 0x00 offset[0] = 0 + // [3] 0x06 offset[1] = 6 (terminator = data region length) + // [4..9] 6-byte child data region + // + // Inner layout, occupying that 6-byte region: + // [4] 0b10011 arrayHeader(largeSize=true, offsetSize=1) + // [5..8] 4-byte LE numElements + // [9] 0x00 end-offset + byte[] value = { + 0b0011, + 0x01, + 0x00, + 0x06, // outer header + offset table + 0b10011, + 0x00, + 0x00, + 0x00, + 0x00, + 0x00 // inner: well-formed baseline (numElements=0) + }; + + // Baseline: well-formed outer + inner round-trip cleanly and descend without complaint. + byte[][] baseline = roundTrip(EMPTY_METADATA, value.clone()); + Variant baseTop = new Variant(ByteBuffer.wrap(baseline[1]), ByteBuffer.wrap(baseline[0])); + assertEquals(1, baseTop.numArrayElements()); + assertEquals(Variant.Type.ARRAY, baseTop.getElementAtIndex(0).getType()); + + // Patch the inner numElements field to a value whose offset table cannot fit in the + // 6-byte inner slot. The outer's own header table is unchanged. + int huge = 0x10000000; + value[5] = (byte) (huge & 0xFF); + value[6] = (byte) ((huge >> 8) & 0xFF); + value[7] = (byte) ((huge >> 16) & 0xFF); + value[8] = (byte) ((huge >> 24) & 0xFF); + + byte[][] rt = roundTrip(EMPTY_METADATA, value); + Variant top = new Variant(ByteBuffer.wrap(rt[1]), ByteBuffer.wrap(rt[0])); + // Top-level construction succeeds — the outer is well-formed. + assertEquals(Variant.Type.ARRAY, top.getType()); + assertEquals(1, top.numArrayElements()); + + // Descending into the malformed inner is what trips the per-child shallow check. + IllegalArgumentException thrown = assertThrows(IllegalArgumentException.class, () -> top.getElementAtIndex(0)); + assertExceptionMessageContains(thrown, "offset table"); + } + + /** + * Deeply-nested chain of single-element arrays. Each container is well-formed in isolation + * (the byte layout strictly shrinks at each level, so per-level shallow validation accepts + * every node). A naive recursive consumer would descend indefinitely and exhaust the JVM + * stack; the depth counter on the child constructor must reject the descent that would + * cross {@link VariantUtil#MAX_VARIANT_DEPTH}. + */ + @Test + public void testOverdeepChainRejected() throws IOException { + int arrayLevels = VariantUtil.MAX_VARIANT_DEPTH + 10; + byte[] value = buildNestedArrayChain(arrayLevels); + byte[][] rt = roundTrip(EMPTY_METADATA, value); + Variant top = new Variant(ByteBuffer.wrap(rt[1]), ByteBuffer.wrap(rt[0])); + assertEquals(Variant.Type.ARRAY, top.getType()); + + // Descend until the depth check fires. The cursor at depth d is the variant produced by + // d successful getElementAtIndex(0) calls. The (MAX_VARIANT_DEPTH + 1)-th call attempts + // to construct a child at depth MAX_VARIANT_DEPTH + 1, which is rejected. + Variant cursor = top; + int successfulDescents = 0; + IllegalArgumentException thrown = null; + while (thrown == null) { + try { + cursor = cursor.getElementAtIndex(0); + successfulDescents++; + } catch (IllegalArgumentException e) { + thrown = e; + } + } + assertEquals(VariantUtil.MAX_VARIANT_DEPTH, successfulDescents); + String msg = thrown.getMessage(); + assertTrue( + "Expected message to mention nesting depth: " + msg, + msg != null && msg.toLowerCase().contains("nesting depth")); + } + + /** + * Build a value buffer consisting of {@code arrayLevels} single-element arrays nested one + * inside the next, with a {@code NULL} primitive at the bottom. Each array uses + * {@code offsetSize=2}, smallSize, so the per-level header + offset table is 6 bytes. + */ + private static byte[] buildNestedArrayChain(int arrayLevels) { + // arrayHeader(largeSize=false, offsetSize=2) = + // (0 << 4) | ((2-1) << 2) | ARRAY(3) = 0b0111 + final byte arrayHeader = 0b0111; + final int perLevel = 6; // 1 header + 1 numElements + 2 offsets * 2 bytes each + final int leafSize = 1; // NULL primitive header + int total = perLevel * arrayLevels + leafSize; + byte[] buf = new byte[total]; + for (int i = 0; i < arrayLevels; i++) { + int pos = i * perLevel; + int dataLen = total - pos - perLevel; // size of the inner container at this level + buf[pos] = arrayHeader; + buf[pos + 1] = 0x01; // numElements = 1 + buf[pos + 2] = 0x00; // offset[0] LE byte 0 (= 0) + buf[pos + 3] = 0x00; // offset[0] LE byte 1 + buf[pos + 4] = (byte) (dataLen & 0xFF); // offset[1] LE byte 0 + buf[pos + 5] = (byte) ((dataLen >> 8) & 0xFF); // offset[1] LE byte 1 + } + buf[total - 1] = (byte) 0x00; // NULL primitive header + return buf; + } + + /** + * Metadata whose dictionary table fits in the buffer (so top-level construction accepts it) + * but whose per-entry offsets are non-monotonic. The structural fit-check is eager; the + * per-entry offset check is deferred to {@link VariantUtil#getMetadataKey}, which fires + * when a caller actually reads a field name. + */ + @Test + public void testMalformedDictOffsetCaughtLazily() throws IOException { + // Metadata layout (offsetSize=1, dictSize=2): + // [0] 0x01 header: version=1, offsetSize=1 + // [1] 0x02 dictSize = 2 (table fits: 1 + 1 + 3*1 = 5 ≤ buffer size) + // [2] 0x00 offset[0] = 0 + // [3] 0x05 offset[1] = 5 (entry 0 spans data bytes [0..5)) + // [4] 0x03 offset[2] = 3 (BOGUS: entry 1 would have negative span) + // [5..9] 5 bytes of string data + byte[] metadata = {0x01, 0x02, 0x00, 0x05, 0x03, 'A', 'B', 'C', 'D', 'E'}; + + // Value: 1-field object referencing dict id 1. + // objectHeader(largeSize=false, idSize=1, offsetSize=1) = 0x02 + // [0] 0x02 header + // [1] 0x01 numElements = 1 + // [2] 0x01 id[0] = 1 (the bogus dict entry) + // [3] 0x00 offset[0] = 0 + // [4] 0x01 offset[1] = 1 + // [5] 0x00 data: NULL primitive + byte[] value = {0x02, 0x01, 0x01, 0x00, 0x01, 0x00}; + + byte[][] rt = roundTrip(metadata, value); + Variant top = new Variant(ByteBuffer.wrap(rt[1]), ByteBuffer.wrap(rt[0])); + // Top-level construction succeeds: metadata table fits, value is shallow-valid, id < dictSize. + assertEquals(Variant.Type.OBJECT, top.getType()); + assertEquals(1, top.numObjectElements()); + + // The per-entry dict offset check fires only when the caller resolves a field name. + // getMetadataKey throws IllegalStateException for non-monotonic offsets — both that and + // IllegalArgumentException are subtypes of RuntimeException, neither is OOM or SOE. + RuntimeException thrown = assertThrows(RuntimeException.class, () -> top.getFieldAtIndex(0)); + final String expected = "offset"; + assertExceptionMessageContains(thrown, expected); + } + + /** + * Reject Metadata dictSize larger than the surrounding buffer. + */ + @Test + public void testOversizedMetadataDictSize() throws IOException { + // Start from the empty metadata, then set dictSize to 0xFF. + // With offsetSize=1 the offset table would then occupy 256 bytes — past the 3-byte buffer. + byte[] metadata = EMPTY_METADATA.clone(); + metadata[1] = (byte) 0xFF; + expectRoundTripRaisesIllegalArgument(metadata, arrayOneNull(), "dictionary"); + } + + /** + * Reject an empty metadata buffer. + */ + @Test + public void testEmptyMetadataRejected() { + IllegalArgumentException thrown = assertThrows( + IllegalArgumentException.class, + () -> new Variant(ByteBuffer.wrap(arrayOneNull()), ByteBuffer.wrap(new byte[0]))); + assertExceptionMessageContains(thrown, "empty"); + } + + /** + * Reject a single-byte metadata buffer. + */ + @Test + public void testSingleByteMetadataRejected() { + IllegalArgumentException thrown = assertThrows( + IllegalArgumentException.class, + () -> new Variant(ByteBuffer.wrap(arrayOneNull()), ByteBuffer.wrap(new byte[] {0x01}))); + assertExceptionMessageContains(thrown, "truncated"); + } + + /** + * Reject metadata whose version bits are not the single supported version. + */ + @Test + public void testUnsupportedMetadataVersionRejected() throws IOException { + // Header 0x02: version=2 in the low bits, offsetSize=1. A well-formed empty dictionary. + final byte[] metadata = {0x02, 0x00, 0x00}; + byte[][] roundTripped = roundTrip(metadata, arrayOneNull()); + UnsupportedOperationException thrown = assertThrows( + UnsupportedOperationException.class, + () -> new Variant(ByteBuffer.wrap(roundTripped[1]), ByteBuffer.wrap(roundTripped[0]))); + assertExceptionMessageContains(thrown, "version"); + } + + /** + * Reject an object whose field id points past the metadata dictionary. The shallow validator + * bounds every object key id against {@code dictSize}; an id {@code >= dictSize} cannot name a + * dictionary entry and must be rejected at construction. + */ + @Test + public void testFieldIdOutOfRange() throws IOException { + // Metadata with a one-entry dictionary {"a"} (dictSize=1): + // [0] 0x01 header: version=1, offsetSize=1 + // [1] 0x01 dictSize = 1 + // [2] 0x00 offset[0] = 0 + // [3] 0x01 offset[1] = 1 + // [4] 'a' string data + byte[] metadata = {0x01, 0x01, 0x00, 0x01, 'a'}; + + // One-field object whose id is 5 — past the single dictionary entry. + // objectHeader(largeSize=false, idSize=1, offsetSize=1) = 0x02 + // [0] 0x02 header + // [1] 0x01 numElements = 1 + // [2] 0x05 id[0] = 5 (out of range: dictSize = 1) + // [3] 0x00 offset[0] = 0 + // [4] 0x01 offset[1] = 1 + // [5] 0x00 data: NULL primitive + byte[] value = {0x02, 0x01, 0x05, 0x00, 0x01, 0x00}; + expectRoundTripRaisesIllegalArgument(metadata, value, "out of range"); + } + + /** + * Reject a long-string/binary primitive whose declared size runs past the buffer. + */ + @Test + public void testOversizedPrimitiveStringSize() throws IOException { + byte[] value = {0x40, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, 0x7F}; + expectRoundTripRaisesIllegalArgument(EMPTY_METADATA, value, "extends past buffer"); + } + + /** + * Reject a long-string/binary primitive whose four-byte size field does not fit in the buffer. + */ + @Test + public void testTruncatedPrimitiveSizeField() throws IOException { + byte[] value = {0x40}; + expectRoundTripRaisesIllegalArgument(EMPTY_METADATA, value, "truncated"); + } + + /** + * Reject metadata whose four-byte dictionary size reads as a negative int. With offsetSize=4 the + * dictionary_size field is read via the wide path, whose {@code v >= 0} guard is the only thing + * standing between {@code 0xFFFFFFFF} and a negative size used in later arithmetic. + */ + @Test + public void testNegativeDictionarySize() throws IOException { + // Header 0xC1: version=1 (low bits), offsetSize=4 (bits 6-7). dictSize field = 0xFFFFFFFF. + byte[] metadata = {(byte) 0xC1, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF}; + expectRoundTripRaisesIllegalArgument(metadata, arrayOneNull(), "unsigned int"); + } + + /** + * Reject a short-string primitive whose inline length (carried in the header's type-info bits) + * runs past the buffer. Here the header claims a 7-byte string but no string bytes follow. + */ + @Test + public void testShortStringLengthExceedsBuffer() throws IOException { + // shortStringHeader(basicType=SHORT_STR=1, length=7) = (7 << 2) | 1 = 0x1D, with no payload. + expectRoundTripRaisesIllegalArgument(EMPTY_METADATA, new byte[] {0x1D}, "short string"); + } + + /** + * "new types may be added to the specification without incrementing the version ID. In such a + * situation, an implementation should be able to read the rest of the Variant value if desired." + */ + @Test + public void testUnknownPrimitiveType() throws IOException { + // Unknown primitive type id 63. + final byte unknownPrimitive = (byte) 0xFC; + // arrayHeader(largeSize=false, offsetSize=1) = (0 << 4) | (0 << 2) | ARRAY = 0b0011. + // numElements = 3; the four 1-byte offsets frame three back-to-back elements: + // element 0: INT8(10) -> [0x0C, 0x0A] spans data bytes [0,2) + // element 1: unknown type 63 -> [0xFC] spans data bytes [2,3) + // element 2: INT8(20) -> [0x0C, 0x14] spans data bytes [3,5) + byte[] value = { + 0b0011, + 0x03, + 0x00, + 0x02, + 0x03, + 0x05, + 0x0C, + 0x0A, + unknownPrimitive, // element 1: unknown primitive type + 0x0C, + 0x14 + }; + + byte[][] rt = roundTrip(EMPTY_METADATA, value); + Variant top = new Variant(ByteBuffer.wrap(rt[1]), ByteBuffer.wrap(rt[0])); + + // construction succeeds and reports all three elements. + assertEquals(Variant.Type.ARRAY, top.getType()); + assertEquals(3, top.numArrayElements()); + + // Get first and third elements, skip the unknown second one. + Variant first = top.getElementAtIndex(0); + assertEquals(Variant.Type.BYTE, first.getType()); + assertEquals((byte) 10, first.getByte()); + + Variant third = top.getElementAtIndex(2); + assertEquals(Variant.Type.BYTE, third.getType()); + assertEquals((byte) 20, third.getByte()); + + // Asking for the unknown element fails. + IllegalArgumentException thrown = assertThrows(IllegalArgumentException.class, () -> top.getElementAtIndex(1)); + assertExceptionMessageContains(thrown, "unknown primitive type"); + } + + /** + * The spec permits a variant field offsets to be non-monotonic, and equal offsets are a legal + * consequence of that — two fields may point at the same value bytes. + */ + @Test + public void testDuplicateFieldOffsets() throws IOException { + // Metadata with a two-entry dictionary {"a", "b"} + byte[] metadata = {0x01, 0x02, 0x00, 0x01, 0x02, 'a', 'b'}; + + // Two-field variant whose offsets are identical (both 0): fields "a" and "b" share the + // single TRUE value in the data region. + // objectHeader(largeSize=false, idSize=1, offsetSize=1) = 0x02 + // [0] 0x02 header + // [1] 0x02 numElements = 2 + // [2] 0x00 id[0] = 0 ("a") + // [3] 0x01 id[1] = 1 ("b") + // [4] 0x00 offset[0] = 0 + // [5] 0x00 offset[1] = 0 (duplicate of offset[0]: "b" overlaps "a") + // [6] 0x01 offset[2] = 1 (terminator = data region length) + // [7] 0x04 data: a single TRUE primitive, referenced by both fields + byte[] value = {0x02, 0x02, 0x00, 0x01, 0x00, 0x00, 0x01, 0x04}; + + byte[][] rt = roundTrip(metadata, value); + Variant top = new Variant(ByteBuffer.wrap(rt[1]), ByteBuffer.wrap(rt[0])); + + assertEquals(Variant.Type.OBJECT, top.getType()); + assertEquals(2, top.numObjectElements()); + + // Both keys resolve to the shared value. + Variant a = top.getFieldByKey("a"); + Variant b = top.getFieldByKey("b"); + assertEquals(Variant.Type.BOOLEAN, a.getType()); + assertEquals(Variant.Type.BOOLEAN, b.getType()); + assertTrue(a.getBoolean()); + assertTrue(b.getBoolean()); + + // Index-ordered access agrees: both fields report their key and the same value. + assertEquals("a", top.getFieldAtIndex(0).key); + assertEquals("b", top.getFieldAtIndex(1).key); + assertTrue(top.getFieldAtIndex(0).value.getBoolean()); + assertTrue(top.getFieldAtIndex(1).value.getBoolean()); + } + + // ------------------------------------------------------------------ + // Helpers + // ------------------------------------------------------------------ + + /** Single-element array containing a NULL primitive. 5 bytes. */ + private static byte[] arrayOneNull() { + // header = arrayHeader(largeSize=false, offsetSize=1) = (0 << 4) | (0 << 2) | ARRAY = 0b0011 + // numElements = 1; offset[0] = 0, offset[1] = 1; data: NULL primitive header = 0x00. + return new byte[] {0b0011, 0x01, 0x00, 0x01, 0x00}; + } + + /** + * Run patched bytes through a real Parquet write/read cycle and expect rejection. + * @param metadata metadata + * @param value payload + * @param messageSubstring substring of expected error message. Pass "" to skip check. + * */ + private void expectRoundTripRaisesIllegalArgument(byte[] metadata, byte[] value, String messageSubstring) + throws IOException { + byte[][] roundTripped = roundTrip(metadata, value); + Assert.assertArrayEquals("metadata changed during round-trip", metadata, roundTripped[0]); + Assert.assertArrayEquals("value changed during round-trip", value, roundTripped[1]); + IllegalArgumentException thrown = assertThrows( + IllegalArgumentException.class, + () -> new Variant(ByteBuffer.wrap(roundTripped[1]), ByteBuffer.wrap(roundTripped[0]))); + assertExceptionMessageContains(thrown, messageSubstring); + } + + /** + * Assert that an exception contains a specific message. If it doesn't, an assertion is raised that + * contains the thrown exception too. + * @param thrown exception to be analyzed. + * @param expected expected string, case-insensitive. + * @throws AssertionError is the text isn't found in the exception message. + */ + private static void assertExceptionMessageContains(RuntimeException thrown, String expected) { + + String msg = thrown.getMessage(); + if (msg == null || !msg.toLowerCase().contains(expected)) { + throw new AssertionError("Did not find \"" + expected + "\" in: " + msg, thrown); + } + } + + /** + * Write one row of {@code (metadata, value)} to an in-memory Parquet file, read it back + * as [metadata, value] + * @param metadata variant metadata + * @param value data + * @return the data read back as the tuple of metadata and value. + */ + private byte[][] roundTrip(byte[] metadata, byte[] value) throws IOException { + ByteArrayOutputFile out = new ByteArrayOutputFile(); + + // write it out + try (ParquetWriter writer = ExampleParquetWriter.builder(out) + .withType(SCHEMA) + .withCompressionCodec(CompressionCodecName.UNCOMPRESSED) + .withDictionaryEncoding(false) + .build()) { + SimpleGroup row = new SimpleGroup(SCHEMA); + row.add("metadata", Binary.fromConstantByteArray(metadata)); + row.add("value", Binary.fromConstantByteArray(value)); + writer.write(row); + } + + byte[] parquetBytes = out.toByteArray(); + + // optional save to fs for later testing. + if (SAVE_GENERATED_FILES) { + Files.createDirectories(OUTPUT_DIR); + Path target = OUTPUT_DIR.resolve(testName.getMethodName() + ".parquet"); + Files.write(target, parquetBytes); + } + // read it back + ByteArrayInputFile in = new ByteArrayInputFile(parquetBytes); + try (ParquetReader reader = new GroupParquetReaderBuilder(in).build()) { + Group g = reader.read(); + Assert.assertNotNull("expected at least one row", g); + byte[] readMetadata = g.getBinary("metadata", 0).getBytes(); + byte[] readValue = g.getBinary("value", 0).getBytes(); + Assert.assertNull("expected exactly one row", reader.read()); + return new byte[][] {readMetadata, readValue}; + } + } + + /** + * ParquetReader.Builder over an InputFile, using GroupReadSupport. + */ + private static final class GroupParquetReaderBuilder extends ParquetReader.Builder { + GroupParquetReaderBuilder(InputFile file) { + super(file); + } + + @Override + protected ReadSupport getReadSupport() { + return new GroupReadSupport(); + } + } + + /** {@link OutputFile} backed by a {@link ByteArrayOutputStream}. */ + private static final class ByteArrayOutputFile implements OutputFile { + private final ByteArrayOutputStream baos = new ByteArrayOutputStream(); + + byte[] toByteArray() { + return baos.toByteArray(); + } + + @Override + public PositionOutputStream create(long blockSizeHint) { + return newStream(); + } + + @Override + public PositionOutputStream createOrOverwrite(long blockSizeHint) { + return newStream(); + } + + private PositionOutputStream newStream() { + return new PositionOutputStream() { + private long pos = 0; + + @Override + public void write(int b) { + baos.write(b); + pos++; + } + + @Override + public void write(byte[] b, int off, int len) { + baos.write(b, off, len); + pos += len; + } + + @Override + public long getPos() { + return pos; + } + }; + } + + @Override + public boolean supportsBlockSize() { + return false; + } + + @Override + public long defaultBlockSize() { + return 0; + } + } + + /** {@link InputFile} backed by a {@code byte[]}. */ + private static final class ByteArrayInputFile implements InputFile { + private final byte[] data; + + ByteArrayInputFile(byte[] data) { + this.data = data; + } + + @Override + public long getLength() { + return data.length; + } + + @Override + public SeekableInputStream newStream() { + return new SeekableInputStream() { + private int pos = 0; + + @Override + public int read() { + return pos < data.length ? (data[pos++] & 0xFF) : -1; + } + + @Override + public int read(byte[] b, int off, int len) { + int remaining = data.length - pos; + if (remaining <= 0) { + return -1; + } + int n = Math.min(len, remaining); + System.arraycopy(data, pos, b, off, n); + pos += n; + return n; + } + + @Override + public long getPos() { + return pos; + } + + @Override + public void seek(long newPos) { + pos = (int) newPos; + } + + @Override + public void readFully(byte[] bytes) throws IOException { + readFully(bytes, 0, bytes.length); + } + + @Override + public void readFully(byte[] bytes, int start, int len) throws IOException { + if (pos + len > data.length) { + throw new EOFException("Unexpected end of data"); + } + System.arraycopy(data, pos, bytes, start, len); + pos += len; + } + + @Override + public int read(ByteBuffer buf) { + int len = buf.remaining(); + int remaining = data.length - pos; + if (remaining <= 0) { + return -1; + } + int n = Math.min(len, remaining); + buf.put(data, pos, n); + pos += n; + return n; + } + + @Override + public void readFully(ByteBuffer buf) throws IOException { + int len = buf.remaining(); + if (pos + len > data.length) { + throw new IOException("Unexpected end of data"); + } + buf.put(data, pos, len); + pos += len; + } + + @Override + public void close() {} + }; + } + } +} diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantArray.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantArray.java index 78673dfba3..1ba4b5ec8d 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantArray.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantArray.java @@ -76,7 +76,7 @@ private static byte[] constructArray(byte[]... elements) { @Test public void testEmptyArray() { - Variant value = new Variant(ByteBuffer.wrap(new byte[] {0b0011, 0x00}), VariantTestUtil.EMPTY_METADATA); + Variant value = new Variant(ByteBuffer.wrap(new byte[] {0b0011, 0x00, 0x00}), VariantTestUtil.EMPTY_METADATA); VariantTestUtil.testVariant(value, v -> { VariantTestUtil.checkType(v, VariantUtil.ARRAY, Variant.Type.ARRAY); assertThat(v.numArrayElements()).isEqualTo(0); @@ -86,7 +86,7 @@ public void testEmptyArray() { @Test public void testEmptyLargeArray() { Variant value = new Variant( - ByteBuffer.wrap(new byte[] {0b10011, 0x00, 0x00, 0x00, 0x00}), VariantTestUtil.EMPTY_METADATA); + ByteBuffer.wrap(new byte[] {0b10011, 0x00, 0x00, 0x00, 0x00, 0x00}), VariantTestUtil.EMPTY_METADATA); VariantTestUtil.testVariant(value, v -> { VariantTestUtil.checkType(v, VariantUtil.ARRAY, Variant.Type.ARRAY); assertThat(v.numArrayElements()).isEqualTo(0); @@ -95,9 +95,18 @@ public void testEmptyLargeArray() { @Test public void testLargeArraySize() { - Variant value = new Variant( - ByteBuffer.wrap(new byte[] {0b10011, (byte) 0xFF, (byte) 0x01, 0x00, 0x00}), - VariantTestUtil.EMPTY_METADATA); + int n = 511; + // largeSize array, offsetSize=2, 511 NULL elements + ByteBuffer buf = ByteBuffer.allocate(1 + 4 + (n + 1) * 2 + n).order(ByteOrder.LITTLE_ENDIAN); + buf.put((byte) 0b10111); + buf.putInt(n); + for (int i = 0; i <= n; i++) { + buf.putShort((short) i); + } + for (int i = 0; i < n; i++) { + buf.put(VariantTestUtil.primitiveHeader(0)); // NULL + } + Variant value = new Variant(ByteBuffer.wrap(buf.array()), VariantTestUtil.EMPTY_METADATA); VariantTestUtil.testVariant(value, v -> { VariantTestUtil.checkType(v, VariantUtil.ARRAY, Variant.Type.ARRAY); assertThat(v.numArrayElements()).isEqualTo(511); @@ -171,8 +180,8 @@ public void testArrayFourByteOffset() { @Test public void testInvalidArray() { - // An object header - Variant value = new Variant(ByteBuffer.wrap(new byte[] {0b1000010}), VariantTestUtil.EMPTY_METADATA); + // A minimal well-formed object value (header, numElements=0, terminator=0). + Variant value = new Variant(ByteBuffer.wrap(new byte[] {0b10, 0x00, 0x00}), VariantTestUtil.EMPTY_METADATA); assertThatThrownBy(value::numArrayElements) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Cannot read OBJECT value as ARRAY"); diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantArrayBuilder.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantArrayBuilder.java index 772777f474..a14b1501e7 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantArrayBuilder.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantArrayBuilder.java @@ -151,12 +151,13 @@ private void buildNested(int i, VariantArrayBuilder obj) { @Test public void testNestedBuilder() { VariantBuilder b = new VariantBuilder(); - buildNested(1000, b.startArray()); + int depth = VariantUtil.MAX_VARIANT_DEPTH - 1; + buildNested(depth, b.startArray()); b.endArray(); VariantTestUtil.testVariant(b.build(), v -> { Variant curr = v; - for (int i = 1000; i >= 0; i--) { + for (int i = depth; i >= 0; i--) { VariantTestUtil.checkType(curr, VariantUtil.ARRAY, Variant.Type.ARRAY); if (i == 0) { assertThat(curr.numArrayElements()).isEqualTo(0); diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantObject.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantObject.java index c26a7c3ec4..32a6f69c55 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantObject.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantObject.java @@ -138,7 +138,7 @@ private static ByteBuffer constructMetadata(Boolean isSorted, List field @Test public void testEmptyObject() { - Variant value = new Variant(ByteBuffer.wrap(new byte[] {0b10, 0x00}), VariantTestUtil.EMPTY_METADATA); + Variant value = new Variant(ByteBuffer.wrap(new byte[] {0b10, 0x00, 0x00}), VariantTestUtil.EMPTY_METADATA); VariantTestUtil.testVariant(value, v -> { VariantTestUtil.checkType(v, VariantUtil.OBJECT, Variant.Type.OBJECT); assertThat(v.numObjectElements()).isEqualTo(0); @@ -148,7 +148,7 @@ public void testEmptyObject() { @Test public void testEmptyLargeObject() { Variant value = new Variant( - ByteBuffer.wrap(new byte[] {0b1000010, 0x00, 0x00, 0x00, 0x00}), VariantTestUtil.EMPTY_METADATA); + ByteBuffer.wrap(new byte[] {0b1000010, 0x00, 0x00, 0x00, 0x00, 0x00}), VariantTestUtil.EMPTY_METADATA); VariantTestUtil.testVariant(value, v -> { VariantTestUtil.checkType(v, VariantUtil.OBJECT, Variant.Type.OBJECT); assertThat(v.numObjectElements()).isEqualTo(0); @@ -332,7 +332,8 @@ public void testLargeObject() { @Test public void testInvalidObject() { - Variant value = new Variant(ByteBuffer.wrap(new byte[] {0b10011}), VariantTestUtil.EMPTY_METADATA); + // A minimal well-formed array value (header, numElements=0, terminator=0). + Variant value = new Variant(ByteBuffer.wrap(new byte[] {0b0011, 0x00, 0x00}), VariantTestUtil.EMPTY_METADATA); assertThatThrownBy(value::numObjectElements) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Cannot read ARRAY value as OBJECT"); diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantObjectBuilder.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantObjectBuilder.java index d739fdba15..90b84a42b2 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantObjectBuilder.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantObjectBuilder.java @@ -185,12 +185,13 @@ private void buildNested(int i, VariantObjectBuilder obj) { @Test public void testNestedBuilder() { VariantBuilder b = new VariantBuilder(); - buildNested(1000, b.startObject()); + int depth = VariantUtil.MAX_VARIANT_DEPTH - 1; + buildNested(depth, b.startObject()); b.endObject(); VariantTestUtil.testVariant(b.build(), v -> { Variant curr = v; - for (int i = 1000; i >= 0; i--) { + for (int i = depth; i >= 0; i--) { VariantTestUtil.checkType(curr, VariantUtil.OBJECT, Variant.Type.OBJECT); if (i == 0) { assertThat(curr.numObjectElements()).isEqualTo(0); diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantScalar.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantScalar.java index 73a686e60b..bee50c93c6 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantScalar.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestVariantScalar.java @@ -28,6 +28,7 @@ import java.time.LocalTime; import java.time.temporal.ChronoUnit; import java.util.UUID; +import java.util.function.Consumer; import org.junit.jupiter.api.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -645,25 +646,21 @@ public void testUUID() { @Test public void testInvalidType() { - Variant value = new Variant(ByteBuffer.wrap(new byte[] {(byte) 0xFC}), VariantTestUtil.EMPTY_METADATA); - assertThatThrownBy(value::getBoolean) + assertThatThrownBy(() -> new Variant(ByteBuffer.wrap(new byte[] {(byte) 0xFC}), VariantTestUtil.EMPTY_METADATA)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot read unknownType(basicType: 0, valueHeader: 63) value as BOOLEAN"); + .hasMessage("Unknown primitive type in variant: 63"); } @Test public void testInvalidBoolean() { - Variant value = new Variant( - ByteBuffer.wrap(new byte[] {VariantTestUtil.primitiveHeader(6)}), VariantTestUtil.EMPTY_METADATA); - assertThatThrownBy(value::getBoolean) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot read LONG value as BOOLEAN"); + assertVariantLongReadInvalid("Cannot read LONG value as BOOLEAN", Variant::getBoolean); } @Test public void testInvalidLong() { Variant value = new Variant( - ByteBuffer.wrap(new byte[] {VariantTestUtil.primitiveHeader(16)}), VariantTestUtil.EMPTY_METADATA); + ByteBuffer.wrap(new byte[] {VariantTestUtil.primitiveHeader(16), 0, 0, 0, 0}), + VariantTestUtil.EMPTY_METADATA); assertThatThrownBy(value::getLong) .isInstanceOf(IllegalArgumentException.class) .hasMessage( @@ -672,82 +669,61 @@ public void testInvalidLong() { @Test public void testInvalidInt() { - Variant value = new Variant( - ByteBuffer.wrap(new byte[] {VariantTestUtil.primitiveHeader(6)}), VariantTestUtil.EMPTY_METADATA); - assertThatThrownBy(value::getInt) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot read LONG value as one of [BYTE, SHORT, INT, DATE]"); + assertVariantLongReadInvalid("Cannot read LONG value as one of [BYTE, SHORT, INT, DATE]", Variant::getInt); } @Test public void testInvalidShort() { - Variant value = new Variant( - ByteBuffer.wrap(new byte[] {VariantTestUtil.primitiveHeader(6)}), VariantTestUtil.EMPTY_METADATA); - assertThatThrownBy(value::getShort) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot read LONG value as one of [BYTE, SHORT]"); + assertVariantLongReadInvalid("Cannot read LONG value as one of [BYTE, SHORT]", Variant::getShort); } @Test public void testInvalidByte() { - Variant value = new Variant( - ByteBuffer.wrap(new byte[] {VariantTestUtil.primitiveHeader(6)}), VariantTestUtil.EMPTY_METADATA); - assertThatThrownBy(value::getByte) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot read LONG value as BYTE"); + assertVariantLongReadInvalid("Cannot read LONG value as BYTE", Variant::getByte); } @Test public void testInvalidFloat() { - Variant value = new Variant( - ByteBuffer.wrap(new byte[] {VariantTestUtil.primitiveHeader(6)}), VariantTestUtil.EMPTY_METADATA); - assertThatThrownBy(value::getFloat) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot read LONG value as FLOAT"); + assertVariantLongReadInvalid("Cannot read LONG value as FLOAT", Variant::getFloat); } @Test public void testInvalidDouble() { - Variant value = new Variant( - ByteBuffer.wrap(new byte[] {VariantTestUtil.primitiveHeader(6)}), VariantTestUtil.EMPTY_METADATA); - assertThatThrownBy(value::getDouble) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot read LONG value as DOUBLE"); + assertVariantLongReadInvalid("Cannot read LONG value as DOUBLE", Variant::getDouble); } @Test public void testInvalidDecimal() { - Variant value = new Variant( - ByteBuffer.wrap(new byte[] {VariantTestUtil.primitiveHeader(6), 0}), VariantTestUtil.EMPTY_METADATA); - assertThatThrownBy(value::getDecimal) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot read LONG value as one of [DECIMAL4, DECIMAL8, DECIMAL16]"); + assertVariantLongReadInvalid( + "Cannot read LONG value as one of [DECIMAL4, DECIMAL8, DECIMAL16]", Variant::getDecimal); } @Test public void testInvalidUUID() { - Variant value = new Variant( - ByteBuffer.wrap(new byte[] {VariantTestUtil.primitiveHeader(6)}), VariantTestUtil.EMPTY_METADATA); - assertThatThrownBy(value::getUUID) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot read LONG value as UUID"); + assertVariantLongReadInvalid("Cannot read LONG value as UUID", Variant::getUUID); } @Test public void testInvalidString() { - Variant value = new Variant( - ByteBuffer.wrap(new byte[] {VariantTestUtil.primitiveHeader(6)}), VariantTestUtil.EMPTY_METADATA); - assertThatThrownBy(value::getString) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot read LONG value as STRING"); + assertVariantLongReadInvalid("Cannot read LONG value as STRING", Variant::getString); } @Test public void testInvalidBinary() { + assertVariantLongReadInvalid("Cannot read LONG value as BINARY", Variant::getBinary); + } + + /** + * Assert that reading a long value variant raises an IllegalArgumentException. + * @param expectedErrorText expected text. + * @param consumer variant consumer. + */ + private static void assertVariantLongReadInvalid(String expectedErrorText, Consumer consumer) { Variant value = new Variant( - ByteBuffer.wrap(new byte[] {VariantTestUtil.primitiveHeader(6)}), VariantTestUtil.EMPTY_METADATA); - assertThatThrownBy(value::getBinary) + ByteBuffer.wrap(new byte[] {VariantTestUtil.primitiveHeader(6), 0, 0, 0, 0, 0, 0, 0, 0}), + VariantTestUtil.EMPTY_METADATA); + assertThatThrownBy(() -> consumer.accept(value)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot read LONG value as BINARY"); + .hasMessage(expectedErrorText); } } diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/VariantTestUtil.java b/parquet-variant/src/test/java/org/apache/parquet/variant/VariantTestUtil.java index 00d0bb82ff..9507738daa 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/VariantTestUtil.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/VariantTestUtil.java @@ -36,7 +36,8 @@ public class VariantTestUtil { /** Random number generator for generating random strings */ private static SecureRandom random = new SecureRandom(new byte[] {1, 2, 3, 4, 5}); - static final ByteBuffer EMPTY_METADATA = ByteBuffer.wrap(new byte[] {0b1}); + // version=1, offsetSize=1, dictSize=0, single end-offset=0 — minimum well-formed empty metadata + static final ByteBuffer EMPTY_METADATA = ByteBuffer.wrap(new byte[] {0b1, 0x00, 0x00}); static void checkType(Variant v, int expectedBasicType, Variant.Type expectedType) { assertThat(v.value.get(v.value.position()) & VariantUtil.BASIC_TYPE_MASK) From 195a698d58929594dcbaae5f0fb42e76c4db0426 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Fri, 10 Jul 2026 19:14:36 +0100 Subject: [PATCH 02/12] Review feedback plus move to assertj --- .../apache/parquet/variant/VariantUtil.java | 49 ++++-- .../parquet/variant/TestHardenedReader.java | 155 +++++++++--------- 2 files changed, 110 insertions(+), 94 deletions(-) diff --git a/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java b/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java index 8625cdc8e3..262bc67497 100644 --- a/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java +++ b/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java @@ -192,7 +192,12 @@ class VariantUtil { /** * Maximum permitted nesting depth of a Variant value. - * same limit as in VariantJsonParser. + * same limit as in VariantJsonParser: {@value}. + *

This depth is not part of the specification and was chosen after discussion on the + * Parquet developer mailing list to be high enough to reduce the impact of parsing malicious files + * on a multi-tenant, multi-threaded system, while still supporting complex structurres. + * + * @see how deep is a realistic variant depth? */ static final int MAX_VARIANT_DEPTH = 1000; @@ -748,15 +753,23 @@ static ObjectInfo getObjectInfo(ByteBuffer value) { // b4 to determine whether the object uses a 1/4-byte size. boolean largeSize = ((typeInfo >> 4) & 0x1) != 0; int sizeBytes = (largeSize ? U32_SIZE : 1); - int numElements = readUnsigned(value, value.position() + 1, sizeBytes); + // store as a long to force all arithmetic to be on long numbers and avoid + // wrapping; it will be cast to an int later. + long numElements = readUnsigned(value, value.position() + 1, sizeBytes); // Extracts b3b2 to determine the integer size of the field id list. int idSize = ((typeInfo >> 2) & 0x3) + 1; // Extracts b1b0 to determine the integer size of the offset list. int offsetSize = (typeInfo & 0x3) + 1; int idStartOffset = 1 + sizeBytes; - int offsetStartOffset = idStartOffset + numElements * idSize; - int dataStartOffset = offsetStartOffset + (numElements + 1) * offsetSize; - return new ObjectInfo(numElements, idSize, offsetSize, idStartOffset, offsetStartOffset, dataStartOffset); + // Widen before multiplying/adding: the id list and offset table can each span more than + // Integer.MAX_VALUE bytes for a large numElements, which would wrap a plain int computation. + long offsetStart = idStartOffset + numElements * idSize; + long dataStart = offsetStart + (numElements + 1) * offsetSize; + Preconditions.checkArgument( + dataStart <= (long) value.limit() - value.position(), + "variant object offset table extends past buffer: numElements=%s", + numElements); + return new ObjectInfo((int) numElements, idSize, offsetSize, idStartOffset, (int) offsetStart, (int) dataStart); } /** @@ -795,12 +808,19 @@ static ArrayInfo getArrayInfo(ByteBuffer value) { // b2 to determine whether the object uses a 1/4-byte size. boolean largeSize = ((typeInfo >> 2) & 0x1) != 0; int sizeBytes = (largeSize ? U32_SIZE : 1); - int numElements = readUnsigned(value, value.position() + 1, sizeBytes); + // keep as long until after validation + long numElements = readUnsigned(value, value.position() + 1, sizeBytes); // Extracts b1b0 to determine the integer size of the offset list. int offsetSize = (typeInfo & 0x3) + 1; int offsetStartOffset = 1 + sizeBytes; - int dataStartOffset = offsetStartOffset + (numElements + 1) * offsetSize; - return new ArrayInfo(numElements, offsetSize, offsetStartOffset, dataStartOffset); + // Widen before multiplying/adding: the offset table can span more than Integer.MAX_VALUE bytes + // for a large numElements, which would wrap a plain int computation. + long dataStart = offsetStartOffset + (numElements + 1) * offsetSize; + Preconditions.checkArgument( + dataStart <= (long) value.limit() - value.position(), + "variant array offset table extends past buffer: numElements=%s", + numElements); + return new ArrayInfo((int) numElements, offsetSize, offsetStartOffset, (int) dataStart); } /** @@ -894,6 +914,9 @@ static HashMap getMetadataMap(ByteBuffer metadata) { * Validation of nested structures is deferred so that opening a large well-formed Variant * is not penalized by sub-trees the caller never inspects. * + *

Note that as validation is against the total bytebuffer capacity, a variant can expand into + * the buffer space nominally allocated its siblings. This is not something checked + * for; it is memory safe. * @param valueBuffer the variant value buffer (position/limit define the extent of this node's slot) * @param dictSize the metadata dictionary size, used to bound object field ids * @throws IllegalArgumentException if the value header or container table does not fit within @@ -901,8 +924,8 @@ static HashMap getMetadataMap(ByteBuffer metadata) { */ static void validateValueShallow(final ByteBuffer valueBuffer, final int dictSize) { int pos = valueBuffer.position(); - Preconditions.checkArgument(pos >= 0 && pos < valueBuffer.limit(), "variant value is empty"); long slot = (long) valueBuffer.limit() - pos; + Preconditions.checkArgument(slot > 0, "variant value is empty"); int header = valueBuffer.get(pos) & 0xFF; int basicType = header & BASIC_TYPE_MASK; int typeInfo = (header >> BASIC_TYPE_BITS) & PRIMITIVE_TYPE_MASK; @@ -927,7 +950,7 @@ static void validateValueShallow(final ByteBuffer valueBuffer, final int dictSiz * @param valueBuffer buffer with the variant data * @param typeInfo type information from the metadata * @param pos buffer read position - * @param length length of slot for this primitive. + * @param length length of slot for this container. * @param dictSize dictionary size * @param isObject is this an object? */ @@ -950,11 +973,11 @@ private static void validateContainerShallow( int offsetSize = (typeInfo & 0x3) + 1; int sizeBytes = largeSize ? U32_SIZE : 1; Preconditions.checkArgument(1L + sizeBytes <= length, "variant container header truncated"); - int numElements = readUnsignedLittleEndian(valueBuffer, pos + 1, sizeBytes); + long numElements = readUnsignedLittleEndian(valueBuffer, pos + 1, sizeBytes); long idStart = 1L + sizeBytes; - long idBytes = isObject ? (long) numElements * idSize : 0L; + long idBytes = isObject ? numElements * idSize : 0L; long offsetStart = idStart + idBytes; - long offsetBytes = (long) (numElements + 1) * offsetSize; + long offsetBytes = (numElements + 1) * offsetSize; long dataStart = offsetStart + offsetBytes; Preconditions.checkArgument( dataStart <= length, "variant container offset table extends past buffer: numElements=%s", numElements); diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java index 813fe99e83..002461125c 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java @@ -18,9 +18,8 @@ */ package org.apache.parquet.variant; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertThrows; -import static org.junit.Assert.assertTrue; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.io.ByteArrayOutputStream; import java.io.EOFException; @@ -44,7 +43,6 @@ import org.apache.parquet.io.api.Binary; import org.apache.parquet.schema.MessageType; import org.apache.parquet.schema.MessageTypeParser; -import org.junit.Assert; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TestName; @@ -88,9 +86,9 @@ public void testWellFormedRoundTrip() throws IOException { byte[] wellFormed = arrayOneNull(); byte[][] roundTripped = roundTrip(EMPTY_METADATA, wellFormed); Variant v = new Variant(ByteBuffer.wrap(roundTripped[1]), ByteBuffer.wrap(roundTripped[0])); - assertEquals(Variant.Type.ARRAY, v.getType()); - assertEquals(1, v.numArrayElements()); - assertEquals(Variant.Type.NULL, v.getElementAtIndex(0).getType()); + assertThat(v.getType()).isEqualTo(Variant.Type.ARRAY); + assertThat(v.numArrayElements()).isEqualTo(1); + assertThat(v.getElementAtIndex(0).getType()).isEqualTo(Variant.Type.NULL); } /** @@ -132,11 +130,21 @@ public void testOversizedNumElements() throws IOException { public void testOffsetArithmeticBoundsCheck() throws IOException { // Object header that claims numElements and offsetSize values whose product overflows int // unless validated with widened arithmetic. - // Layout: [objectHeader(large=1, idSize=1, offsetSize=4) = 0x4E, - // numElements (4 bytes, little-endian) = 0x33333333, - // three filler bytes] - byte[] value = new byte[] {0x4E, 0x33, 0x33, 0x33, 0x33, (byte) 0xFF, (byte) 0xFF, 0x3F}; - expectRoundTripRaisesIllegalArgument(EMPTY_METADATA, value, ""); + byte[] value = new byte[] {0x13, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, 0x7F}; + expectRoundTripRaisesIllegalArgument(EMPTY_METADATA, value, "offset table"); + } + + /** + * {@link VariantUtil#valueSize} decodes a raw value buffer directly, without going through the + * validating {@link Variant} constructor. + */ + @Test + public void testValueSizeRejectsOffsetArithmeticOverflow() { + // Large array claiming numElements = 0x7FFFFFFF + byte[] value = new byte[] {0x13, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, 0x7F}; + assertThatThrownBy(() -> VariantUtil.valueSize(ByteBuffer.wrap(value))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("offset table"); } /** @@ -173,8 +181,8 @@ public void testMalformedChildInsideWellFormedParent() throws IOException { // Baseline: well-formed outer + inner round-trip cleanly and descend without complaint. byte[][] baseline = roundTrip(EMPTY_METADATA, value.clone()); Variant baseTop = new Variant(ByteBuffer.wrap(baseline[1]), ByteBuffer.wrap(baseline[0])); - assertEquals(1, baseTop.numArrayElements()); - assertEquals(Variant.Type.ARRAY, baseTop.getElementAtIndex(0).getType()); + assertThat(baseTop.numArrayElements()).isEqualTo(1); + assertThat(baseTop.getElementAtIndex(0).getType()).isEqualTo(Variant.Type.ARRAY); // Patch the inner numElements field to a value whose offset table cannot fit in the // 6-byte inner slot. The outer's own header table is unchanged. @@ -187,12 +195,13 @@ public void testMalformedChildInsideWellFormedParent() throws IOException { byte[][] rt = roundTrip(EMPTY_METADATA, value); Variant top = new Variant(ByteBuffer.wrap(rt[1]), ByteBuffer.wrap(rt[0])); // Top-level construction succeeds — the outer is well-formed. - assertEquals(Variant.Type.ARRAY, top.getType()); - assertEquals(1, top.numArrayElements()); + assertThat(top.getType()).isEqualTo(Variant.Type.ARRAY); + assertThat(top.numArrayElements()).isEqualTo(1); // Descending into the malformed inner is what trips the per-child shallow check. - IllegalArgumentException thrown = assertThrows(IllegalArgumentException.class, () -> top.getElementAtIndex(0)); - assertExceptionMessageContains(thrown, "offset table"); + assertThatThrownBy(() -> top.getElementAtIndex(0)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("offset table"); } /** @@ -208,7 +217,7 @@ public void testOverdeepChainRejected() throws IOException { byte[] value = buildNestedArrayChain(arrayLevels); byte[][] rt = roundTrip(EMPTY_METADATA, value); Variant top = new Variant(ByteBuffer.wrap(rt[1]), ByteBuffer.wrap(rt[0])); - assertEquals(Variant.Type.ARRAY, top.getType()); + assertThat(top.getType()).isEqualTo(Variant.Type.ARRAY); // Descend until the depth check fires. The cursor at depth d is the variant produced by // d successful getElementAtIndex(0) calls. The (MAX_VARIANT_DEPTH + 1)-th call attempts @@ -224,11 +233,8 @@ public void testOverdeepChainRejected() throws IOException { thrown = e; } } - assertEquals(VariantUtil.MAX_VARIANT_DEPTH, successfulDescents); - String msg = thrown.getMessage(); - assertTrue( - "Expected message to mention nesting depth: " + msg, - msg != null && msg.toLowerCase().contains("nesting depth")); + assertThat(successfulDescents).isEqualTo(VariantUtil.MAX_VARIANT_DEPTH); + assertThat(thrown).hasMessageContaining("nesting depth"); } /** @@ -288,15 +294,15 @@ public void testMalformedDictOffsetCaughtLazily() throws IOException { byte[][] rt = roundTrip(metadata, value); Variant top = new Variant(ByteBuffer.wrap(rt[1]), ByteBuffer.wrap(rt[0])); // Top-level construction succeeds: metadata table fits, value is shallow-valid, id < dictSize. - assertEquals(Variant.Type.OBJECT, top.getType()); - assertEquals(1, top.numObjectElements()); + assertThat(top.getType()).isEqualTo(Variant.Type.OBJECT); + assertThat(top.numObjectElements()).isEqualTo(1); // The per-entry dict offset check fires only when the caller resolves a field name. // getMetadataKey throws IllegalStateException for non-monotonic offsets — both that and // IllegalArgumentException are subtypes of RuntimeException, neither is OOM or SOE. - RuntimeException thrown = assertThrows(RuntimeException.class, () -> top.getFieldAtIndex(0)); - final String expected = "offset"; - assertExceptionMessageContains(thrown, expected); + assertThatThrownBy(() -> top.getFieldAtIndex(0)) + .isInstanceOf(RuntimeException.class) + .hasMessageContaining("offset"); } /** @@ -316,10 +322,9 @@ public void testOversizedMetadataDictSize() throws IOException { */ @Test public void testEmptyMetadataRejected() { - IllegalArgumentException thrown = assertThrows( - IllegalArgumentException.class, - () -> new Variant(ByteBuffer.wrap(arrayOneNull()), ByteBuffer.wrap(new byte[0]))); - assertExceptionMessageContains(thrown, "empty"); + assertThatThrownBy(() -> new Variant(ByteBuffer.wrap(arrayOneNull()), ByteBuffer.wrap(new byte[0]))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("empty"); } /** @@ -327,10 +332,9 @@ public void testEmptyMetadataRejected() { */ @Test public void testSingleByteMetadataRejected() { - IllegalArgumentException thrown = assertThrows( - IllegalArgumentException.class, - () -> new Variant(ByteBuffer.wrap(arrayOneNull()), ByteBuffer.wrap(new byte[] {0x01}))); - assertExceptionMessageContains(thrown, "truncated"); + assertThatThrownBy(() -> new Variant(ByteBuffer.wrap(arrayOneNull()), ByteBuffer.wrap(new byte[] {0x01}))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("truncated"); } /** @@ -341,10 +345,9 @@ public void testUnsupportedMetadataVersionRejected() throws IOException { // Header 0x02: version=2 in the low bits, offsetSize=1. A well-formed empty dictionary. final byte[] metadata = {0x02, 0x00, 0x00}; byte[][] roundTripped = roundTrip(metadata, arrayOneNull()); - UnsupportedOperationException thrown = assertThrows( - UnsupportedOperationException.class, - () -> new Variant(ByteBuffer.wrap(roundTripped[1]), ByteBuffer.wrap(roundTripped[0]))); - assertExceptionMessageContains(thrown, "version"); + assertThatThrownBy(() -> new Variant(ByteBuffer.wrap(roundTripped[1]), ByteBuffer.wrap(roundTripped[0]))) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessageContaining("version"); } /** @@ -445,21 +448,22 @@ public void testUnknownPrimitiveType() throws IOException { Variant top = new Variant(ByteBuffer.wrap(rt[1]), ByteBuffer.wrap(rt[0])); // construction succeeds and reports all three elements. - assertEquals(Variant.Type.ARRAY, top.getType()); - assertEquals(3, top.numArrayElements()); + assertThat(top.getType()).isEqualTo(Variant.Type.ARRAY); + assertThat(top.numArrayElements()).isEqualTo(3); // Get first and third elements, skip the unknown second one. Variant first = top.getElementAtIndex(0); - assertEquals(Variant.Type.BYTE, first.getType()); - assertEquals((byte) 10, first.getByte()); + assertThat(first.getType()).isEqualTo(Variant.Type.BYTE); + assertThat(first.getByte()).isEqualTo((byte) 10); Variant third = top.getElementAtIndex(2); - assertEquals(Variant.Type.BYTE, third.getType()); - assertEquals((byte) 20, third.getByte()); + assertThat(third.getType()).isEqualTo(Variant.Type.BYTE); + assertThat(third.getByte()).isEqualTo((byte) 20); // Asking for the unknown element fails. - IllegalArgumentException thrown = assertThrows(IllegalArgumentException.class, () -> top.getElementAtIndex(1)); - assertExceptionMessageContains(thrown, "unknown primitive type"); + assertThatThrownBy(() -> top.getElementAtIndex(1)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Unknown primitive type"); } /** @@ -487,22 +491,22 @@ public void testDuplicateFieldOffsets() throws IOException { byte[][] rt = roundTrip(metadata, value); Variant top = new Variant(ByteBuffer.wrap(rt[1]), ByteBuffer.wrap(rt[0])); - assertEquals(Variant.Type.OBJECT, top.getType()); - assertEquals(2, top.numObjectElements()); + assertThat(top.getType()).isEqualTo(Variant.Type.OBJECT); + assertThat(top.numObjectElements()).isEqualTo(2); // Both keys resolve to the shared value. Variant a = top.getFieldByKey("a"); Variant b = top.getFieldByKey("b"); - assertEquals(Variant.Type.BOOLEAN, a.getType()); - assertEquals(Variant.Type.BOOLEAN, b.getType()); - assertTrue(a.getBoolean()); - assertTrue(b.getBoolean()); + assertThat(a.getType()).isEqualTo(Variant.Type.BOOLEAN); + assertThat(b.getType()).isEqualTo(Variant.Type.BOOLEAN); + assertThat(a.getBoolean()).isTrue(); + assertThat(b.getBoolean()).isTrue(); // Index-ordered access agrees: both fields report their key and the same value. - assertEquals("a", top.getFieldAtIndex(0).key); - assertEquals("b", top.getFieldAtIndex(1).key); - assertTrue(top.getFieldAtIndex(0).value.getBoolean()); - assertTrue(top.getFieldAtIndex(1).value.getBoolean()); + assertThat(top.getFieldAtIndex(0).key).isEqualTo("a"); + assertThat(top.getFieldAtIndex(1).key).isEqualTo("b"); + assertThat(top.getFieldAtIndex(0).value.getBoolean()).isTrue(); + assertThat(top.getFieldAtIndex(1).value.getBoolean()).isTrue(); } // ------------------------------------------------------------------ @@ -525,27 +529,16 @@ private static byte[] arrayOneNull() { private void expectRoundTripRaisesIllegalArgument(byte[] metadata, byte[] value, String messageSubstring) throws IOException { byte[][] roundTripped = roundTrip(metadata, value); - Assert.assertArrayEquals("metadata changed during round-trip", metadata, roundTripped[0]); - Assert.assertArrayEquals("value changed during round-trip", value, roundTripped[1]); - IllegalArgumentException thrown = assertThrows( - IllegalArgumentException.class, - () -> new Variant(ByteBuffer.wrap(roundTripped[1]), ByteBuffer.wrap(roundTripped[0]))); - assertExceptionMessageContains(thrown, messageSubstring); - } - /** - * Assert that an exception contains a specific message. If it doesn't, an assertion is raised that - * contains the thrown exception too. - * @param thrown exception to be analyzed. - * @param expected expected string, case-insensitive. - * @throws AssertionError is the text isn't found in the exception message. - */ - private static void assertExceptionMessageContains(RuntimeException thrown, String expected) { - - String msg = thrown.getMessage(); - if (msg == null || !msg.toLowerCase().contains(expected)) { - throw new AssertionError("Did not find \"" + expected + "\" in: " + msg, thrown); - } + assertThat(roundTripped[0]) + .describedAs("metadata changed during round-trip") + .isEqualTo(metadata); + assertThat(roundTripped[1]) + .describedAs("value changed during round-trip") + .isEqualTo(value); + assertThatThrownBy(() -> new Variant(ByteBuffer.wrap(roundTripped[1]), ByteBuffer.wrap(roundTripped[0]))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining(messageSubstring); } /** @@ -582,10 +575,10 @@ private byte[][] roundTrip(byte[] metadata, byte[] value) throws IOException { ByteArrayInputFile in = new ByteArrayInputFile(parquetBytes); try (ParquetReader reader = new GroupParquetReaderBuilder(in).build()) { Group g = reader.read(); - Assert.assertNotNull("expected at least one row", g); + assertThat(g).describedAs("expected at least one row").isNotNull(); byte[] readMetadata = g.getBinary("metadata", 0).getBytes(); byte[] readValue = g.getBinary("value", 0).getBytes(); - Assert.assertNull("expected exactly one row", reader.read()); + assertThat(reader.read()).describedAs("expected exactly one row").isNull(); return new byte[][] {readMetadata, readValue}; } } From e6f0b61f83e1abbb095171e833e380f196751388 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Mon, 13 Jul 2026 17:09:57 +0100 Subject: [PATCH 03/12] use Math.toIntExact when casting to int to guarantee overflows raise exceptions I actually think that the predecessor range check will suffice; this is just due diligence --- .../main/java/org/apache/parquet/variant/VariantUtil.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java b/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java index 262bc67497..915f567670 100644 --- a/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java +++ b/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java @@ -195,7 +195,7 @@ class VariantUtil { * same limit as in VariantJsonParser: {@value}. *

This depth is not part of the specification and was chosen after discussion on the * Parquet developer mailing list to be high enough to reduce the impact of parsing malicious files - * on a multi-tenant, multi-threaded system, while still supporting complex structurres. + * on a multi-tenant, multi-threaded system, while still supporting complex structures. * * @see how deep is a realistic variant depth? */ @@ -769,7 +769,8 @@ static ObjectInfo getObjectInfo(ByteBuffer value) { dataStart <= (long) value.limit() - value.position(), "variant object offset table extends past buffer: numElements=%s", numElements); - return new ObjectInfo((int) numElements, idSize, offsetSize, idStartOffset, (int) offsetStart, (int) dataStart); + return new ObjectInfo( + Math.toIntExact(numElements), idSize, offsetSize, idStartOffset, (int) offsetStart, (int) dataStart); } /** From ff528f4267cfb000b6435667ff7e9924621a74e7 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Tue, 14 Jul 2026 14:52:22 +0100 Subject: [PATCH 04/12] Hardening ImmutableMetadata loading, with test This was effectively robust, just pulling the check up earlier --- .../apache/parquet/variant/VariantUtil.java | 3 +++ .../parquet/variant/TestHardenedReader.java | 23 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java b/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java index 915f567670..0d5b045447 100644 --- a/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java +++ b/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java @@ -877,7 +877,10 @@ static HashMap getMetadataMap(ByteBuffer metadata) { // Extracts the highest 2 bits in the metadata header to determine the integer size of the // offset list. int offsetSize = ((metadata.get(pos) >> 6) & 0x3) + 1; + // dictionary size int dictSize = readUnsigned(metadata, pos + 1, offsetSize); + // range check the dictionary, including from integer overflow + checkIndex(Math.toIntExact((long) dictSize * offsetSize), metadata.limit()); HashMap result = new HashMap<>(); int offset = readUnsigned(metadata, pos + 1 + offsetSize, offsetSize); for (int id = 0; id < dictSize; id++) { diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java index 002461125c..2ba63bc33e 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java @@ -509,6 +509,29 @@ public void testDuplicateFieldOffsets() throws IOException { assertThat(top.getFieldAtIndex(1).value.getBoolean()).isTrue(); } + @Test + public void testInvalidImmutableMetadata() { + // test immutable metadata resilience + VariantBuilder vb = new VariantBuilder(); + VariantObjectBuilder obj = vb.startObject(); + obj.appendKey("one"); + obj.appendInt(1); + obj.appendKey("two"); + obj.appendInt(2); + vb.endObject(); + Variant variant = vb.build(); + + ByteBuffer metaBuf = variant.getMetadataBuffer(); + ByteBuffer writable = ByteBuffer.allocate(metaBuf.remaining()); + writable.put(metaBuf.duplicate()); + writable.put(1, (byte) -1); + writable.flip(); + // the offset rejected is 255. Without the hardening, later checks + // will fail, but a different offset value will be reported. + assertThatThrownBy(() -> new ImmutableMetadata(writable)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Invalid byte-array offset (255). length: 11"); + } // ------------------------------------------------------------------ // Helpers // ------------------------------------------------------------------ From 4ac460c93df0e24155a7da00f71c016b44f53bf6 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Wed, 15 Jul 2026 20:35:54 +0100 Subject: [PATCH 05/12] Unknown versions are *not rejected* Policy update after monthly call and slack chat, restore old behaviour until a design for what to do is sketched out. + expand test for unknown (unshredded) type. --- .../org/apache/parquet/variant/Variant.java | 8 +--- .../parquet/variant/TestHardenedReader.java | 46 ++++++++++--------- 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java b/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java index 2c97e824a4..c5dd8542c0 100644 --- a/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java +++ b/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java @@ -118,12 +118,8 @@ public Variant(ByteBuffer value, ByteBuffer metadata) { this.depth = 0; // A metadata buffer must contain at least the version byte Preconditions.checkArgument(this.metadata.remaining() >= 1, "variant metadata is empty"); - // There is currently only one allowed version. - if ((metadata.get(metadata.position()) & VariantUtil.VERSION_MASK) != VariantUtil.VERSION) { - throw new UnsupportedOperationException(String.format( - "Unsupported variant metadata version: %d", - metadata.get(metadata.position()) & VariantUtil.VERSION_MASK)); - } + // There is no checking of version until a policy of how to handle mismatched versions + // is defined. // Pre-compute dictionary size for lazy metadata cache allocation. int pos = this.metadata.position(); diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java index 2ba63bc33e..928df612b7 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java @@ -48,10 +48,16 @@ import org.junit.rules.TestName; /** - * Exercises the bounds checks added to {@link Variant} construction by round-tripping a row of - * {@code (metadata, value)} binaries through an in-memory uncompressed Parquet file, mutating - * specific bytes after writing, and asserting that the resulting buffer is rejected with an - * {@link IllegalArgumentException}. + * Test that the hardened variant reader behaves as expected. + * Many of the tests save to the local filesystem and then clean up the files + * in teardown. This is overkill, but it does permit these test files to + * be preserved and so contributed to the parquet-format repository for + * interoperability testing. + * + *

Set {@link #SAVE_GENERATED_FILES} to true to enable file preservation under + * {@code parquet-variant/target/test-hardened-reader}; return it to false to + * restore cleanup. Each file will be saved with the name of the test. + * . */ public class TestHardenedReader { @@ -338,16 +344,14 @@ public void testSingleByteMetadataRejected() { } /** - * Reject metadata whose version bits are not the single supported version. + * Handling of unknown metadata versions. */ @Test public void testUnsupportedMetadataVersionRejected() throws IOException { // Header 0x02: version=2 in the low bits, offsetSize=1. A well-formed empty dictionary. final byte[] metadata = {0x02, 0x00, 0x00}; byte[][] roundTripped = roundTrip(metadata, arrayOneNull()); - assertThatThrownBy(() -> new Variant(ByteBuffer.wrap(roundTripped[1]), ByteBuffer.wrap(roundTripped[0]))) - .isInstanceOf(UnsupportedOperationException.class) - .hasMessageContaining("version"); + new Variant(ByteBuffer.wrap(roundTripped[1]), ByteBuffer.wrap(roundTripped[0])); } /** @@ -418,30 +422,28 @@ public void testShortStringLengthExceedsBuffer() throws IOException { } /** - * "new types may be added to the specification without incrementing the version ID. In such a - * situation, an implementation should be able to read the rest of the Variant value if desired." + * Handling of unknown primitives. */ @Test public void testUnknownPrimitiveType() throws IOException { - // Unknown primitive type id 63. + // Unknown primitive type final byte unknownPrimitive = (byte) 0xFC; - // arrayHeader(largeSize=false, offsetSize=1) = (0 << 4) | (0 << 2) | ARRAY = 0b0011. - // numElements = 3; the four 1-byte offsets frame three back-to-back elements: - // element 0: INT8(10) -> [0x0C, 0x0A] spans data bytes [0,2) - // element 1: unknown type 63 -> [0xFC] spans data bytes [2,3) - // element 2: INT8(20) -> [0x0C, 0x14] spans data bytes [3,5) + final int elt3 = 0x14; byte[] value = { 0b0011, - 0x03, + 0x03, // three elements 0x00, 0x02, - 0x03, - 0x05, + 0x06, + 0x08, 0x0C, 0x0A, - unknownPrimitive, // element 1: unknown primitive type + unknownPrimitive, // element 1: unknown primitive type, 4 bytes wide + 0x01, + 0x02, + 0x03, 0x0C, - 0x14 + elt3 }; byte[][] rt = roundTrip(EMPTY_METADATA, value); @@ -458,7 +460,7 @@ public void testUnknownPrimitiveType() throws IOException { Variant third = top.getElementAtIndex(2); assertThat(third.getType()).isEqualTo(Variant.Type.BYTE); - assertThat(third.getByte()).isEqualTo((byte) 20); + assertThat(third.getByte()).isEqualTo((byte) elt3); // Asking for the unknown element fails. assertThatThrownBy(() -> top.getElementAtIndex(1)) From 2efa3eeff2e6b4910950a2cfb5d6fd9693748738 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Wed, 15 Jul 2026 20:52:37 +0100 Subject: [PATCH 06/12] Unknown shredded type is rejected. Contains content written by claude code. --- .../parquet/variant/TestHardenedReader.java | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java index 928df612b7..b95d3db79e 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java @@ -41,6 +41,7 @@ import org.apache.parquet.io.PositionOutputStream; import org.apache.parquet.io.SeekableInputStream; import org.apache.parquet.io.api.Binary; +import org.apache.parquet.io.api.RecordConsumer; import org.apache.parquet.schema.MessageType; import org.apache.parquet.schema.MessageTypeParser; import org.junit.Rule; @@ -468,6 +469,48 @@ public void testUnknownPrimitiveType() throws IOException { .hasMessageContaining("Unknown primitive type"); } + /** + * Shredding a container that holds an unknown primitive type raises an exception. The shredding + * writer walks children through {@link Variant#getElementAtIndex}/{@link Variant#getFieldAtIndex}, + * each of which materializes (and therefore validates) a child Variant. An unknown primitive type + * cannot be materialized, so the known elements shred into their typed_value columns and the + * unknown element aborts the write rather than being silently dropped or corrupted. + * + *

The current specification is: "new types may be added to the specification without + * incrementing the version ID. In such a situation, an implementation should be able to read the + * rest of the Variant value if desired." Reading the unknown value is optional; the shredding + * writer here declines and raises when it cannot reconstruct the value. A future policy may always + * reject on reconstruction. + */ + @Test + public void testUnknownPrimitiveTypeShredding() { + // Shredded schema: metadata + residual value + a typed_value LIST whose element shreds INT8. + // Known BYTE elements land in element.typed_value; an unknown element cannot be reconstructed. + MessageType shreddedSchema = MessageTypeParser.parseMessageType("message variant {" + + " required binary metadata;" + + " optional binary value;" + + " optional group typed_value (LIST) {" + + " repeated group list {" + + " required group element {" + + " optional binary value;" + + " optional int32 typed_value (INTEGER(8, true));" + + " }" + + " }" + + " }" + + "}"); + + // Array [BYTE(10), unknown-type-63, BYTE(20)]: the top-level array is well-formed, so it + // constructs; the unknown element is only rejected when the writer descends into it. + final byte unknownPrimitive = (byte) 0xFC; + byte[] value = {0b0011, 0x03, 0x00, 0x02, 0x03, 0x05, 0x0C, 0x0A, unknownPrimitive, 0x0C, 0x14}; + Variant variant = new Variant(ByteBuffer.wrap(value), ByteBuffer.wrap(EMPTY_METADATA)); + assertThat(variant.getType()).isEqualTo(Variant.Type.ARRAY); + + assertThatThrownBy(() -> VariantValueWriter.write(new NoOpRecordConsumer(), shreddedSchema, variant)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Unknown primitive type"); + } + /** * The spec permits a variant field offsets to be non-monotonic, and equal offsets are a legal * consequence of that — two fields may point at the same value bytes. @@ -608,6 +651,45 @@ private byte[][] roundTrip(byte[] metadata, byte[] value) throws IOException { } } + /** A {@link RecordConsumer} that discards everything: used to drive a shredded write to the point of failure. */ + private static final class NoOpRecordConsumer extends RecordConsumer { + @Override + public void startMessage() {} + + @Override + public void endMessage() {} + + @Override + public void startField(String field, int index) {} + + @Override + public void endField(String field, int index) {} + + @Override + public void startGroup() {} + + @Override + public void endGroup() {} + + @Override + public void addInteger(int value) {} + + @Override + public void addLong(long value) {} + + @Override + public void addBoolean(boolean value) {} + + @Override + public void addBinary(Binary value) {} + + @Override + public void addFloat(float value) {} + + @Override + public void addDouble(double value) {} + } + /** * ParquetReader.Builder over an InputFile, using GroupReadSupport. */ From d6499344744186d375667cc173a5a6744853396d Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Thu, 16 Jul 2026 13:52:59 +0100 Subject: [PATCH 07/12] shredded types -read path. claude had done the write path; get it to do what I wanted this time round --- .../parquet/variant/TestHardenedReader.java | 87 +++---------------- 1 file changed, 11 insertions(+), 76 deletions(-) diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java index b95d3db79e..45234ce683 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java @@ -41,7 +41,6 @@ import org.apache.parquet.io.PositionOutputStream; import org.apache.parquet.io.SeekableInputStream; import org.apache.parquet.io.api.Binary; -import org.apache.parquet.io.api.RecordConsumer; import org.apache.parquet.schema.MessageType; import org.apache.parquet.schema.MessageTypeParser; import org.junit.Rule; @@ -470,45 +469,20 @@ public void testUnknownPrimitiveType() throws IOException { } /** - * Shredding a container that holds an unknown primitive type raises an exception. The shredding - * writer walks children through {@link Variant#getElementAtIndex}/{@link Variant#getFieldAtIndex}, - * each of which materializes (and therefore validates) a child Variant. An unknown primitive type - * cannot be materialized, so the known elements shred into their typed_value columns and the - * unknown element aborts the write rather than being silently dropped or corrupted. - * - *

The current specification is: "new types may be added to the specification without - * incrementing the version ID. In such a situation, an implementation should be able to read the - * rest of the Variant value if desired." Reading the unknown value is optional; the shredding - * writer here declines and raises when it cannot reconstruct the value. A future policy may always - * reject on reconstruction. + * Reconstructing a residual {@code value} column that carries an unknown primitive type fails to + * parse. On the shredded read path the residual {@code value} column is the only channel that can + * carry an unknown variant type (a {@code typed_value} column's type is fixed by the Parquet + * schema), and it is replayed through {@link VariantBuilder#appendEncodedValue} — exactly what + * {@code VariantConverters.VariantValueConverter} does on read. */ @Test - public void testUnknownPrimitiveTypeShredding() { - // Shredded schema: metadata + residual value + a typed_value LIST whose element shreds INT8. - // Known BYTE elements land in element.typed_value; an unknown element cannot be reconstructed. - MessageType shreddedSchema = MessageTypeParser.parseMessageType("message variant {" - + " required binary metadata;" - + " optional binary value;" - + " optional group typed_value (LIST) {" - + " repeated group list {" - + " required group element {" - + " optional binary value;" - + " optional int32 typed_value (INTEGER(8, true));" - + " }" - + " }" - + " }" - + "}"); - - // Array [BYTE(10), unknown-type-63, BYTE(20)]: the top-level array is well-formed, so it - // constructs; the unknown element is only rejected when the writer descends into it. - final byte unknownPrimitive = (byte) 0xFC; - byte[] value = {0b0011, 0x03, 0x00, 0x02, 0x03, 0x05, 0x0C, 0x0A, unknownPrimitive, 0x0C, 0x14}; - Variant variant = new Variant(ByteBuffer.wrap(value), ByteBuffer.wrap(EMPTY_METADATA)); - assertThat(variant.getType()).isEqualTo(Variant.Type.ARRAY); + public void testUnknownShreddededPrimitiveTypeReconstruction() { + final byte unknownPrimitive = (byte) 0xFC; // primitive typeInfo 63 - assertThatThrownBy(() -> VariantValueWriter.write(new NoOpRecordConsumer(), shreddedSchema, variant)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Unknown primitive type"); + assertThatThrownBy( + () -> new VariantBuilder().appendEncodedValue(ByteBuffer.wrap(new byte[] {unknownPrimitive}))) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessageContaining("Unknown type in Variant"); } /** @@ -651,45 +625,6 @@ private byte[][] roundTrip(byte[] metadata, byte[] value) throws IOException { } } - /** A {@link RecordConsumer} that discards everything: used to drive a shredded write to the point of failure. */ - private static final class NoOpRecordConsumer extends RecordConsumer { - @Override - public void startMessage() {} - - @Override - public void endMessage() {} - - @Override - public void startField(String field, int index) {} - - @Override - public void endField(String field, int index) {} - - @Override - public void startGroup() {} - - @Override - public void endGroup() {} - - @Override - public void addInteger(int value) {} - - @Override - public void addLong(long value) {} - - @Override - public void addBoolean(boolean value) {} - - @Override - public void addBinary(Binary value) {} - - @Override - public void addFloat(float value) {} - - @Override - public void addDouble(double value) {} - } - /** * ParquetReader.Builder over an InputFile, using GroupReadSupport. */ From 0359bea459d0037f787c9217431887596faa8b3d Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Fri, 17 Jul 2026 13:54:36 +0100 Subject: [PATCH 08/12] add a test for an empty data region, based on the iceberg pr --- .../parquet/variant/TestHardenedReader.java | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java index 45234ce683..c618d812f2 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java @@ -57,7 +57,6 @@ *

Set {@link #SAVE_GENERATED_FILES} to true to enable file preservation under * {@code parquet-variant/target/test-hardened-reader}; return it to false to * restore cleanup. Each file will be saved with the name of the test. - * . */ public class TestHardenedReader { @@ -109,6 +108,33 @@ public void testOutOfRangeChildOffset() throws IOException { expectRoundTripRaisesIllegalArgument(EMPTY_METADATA, value, "child offset"); } + /** + * A zero-length child element is not a valid encoded Variant value. The spec requires every + * array/object element to carry a mandatory 1-byte value header (value_data may be empty, but the + * header may not). An offset that leaves no bytes for the child has no header to read, so the + * top-level array constructs, but materializing the empty element fails to parse. + */ + @Test + public void testEmptyChildElement() { + // Array, numElements=1, offsets [0, 0], empty data region. + // [0] 0b0011 arrayHeader(largeSize=false, offsetSize=1) + // [1] 0x01 numElements = 1 + // [2] 0x00 offset[0] = 0 + // [3] 0x00 offset[1] = 0 (terminator: data region length = 0) + // The single element points at offset 0 of a zero-length data region: no header byte exists. + byte[] value = {0b0011, 0x01, 0x00, 0x00}; + Variant top = new Variant(ByteBuffer.wrap(value), ByteBuffer.wrap(EMPTY_METADATA)); + + // The container itself is well-formed and constructs. + assertThat(top.getType()).isEqualTo(Variant.Type.ARRAY); + assertThat(top.numArrayElements()).isEqualTo(1); + + // Materializing the empty element fails: there is no value header to read. + assertThatThrownBy(() -> top.getElementAtIndex(0)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("variant value is empty"); + } + /** * Reject numElements larger than the surrounding buffer. */ From 01ad858317a5ebd16cadcf575dbd1991a40a23f5 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Sat, 24 Oct 2026 15:56:19 +0100 Subject: [PATCH 09/12] review feedback on comment text --- .../src/main/java/org/apache/parquet/variant/Variant.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java b/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java index c5dd8542c0..b3b2081d2b 100644 --- a/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java +++ b/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java @@ -61,7 +61,11 @@ public final class Variant { * Lazy cache for the parsed array header. */ private VariantUtil.ArrayInfo cachedArrayInfo; - /** Nesting depth of this Variant relative to the top-level value (0 = top-level). */ + + /** + * Child depth of this Variant relative to the top-level value (0 = top-level). + * Arrays and short strings are children here, along with nested variants. + */ private final int depth; /** @@ -109,7 +113,6 @@ public Variant(byte[] value, int valuePos, int valueLength, byte[] metadata, int * Includes validation of the version and the toplevel structure. * @param value value buffer * @param metadata metadata buffer - * @throws UnsupportedOperationException if there is a version mismatch * @throws IllegalArgumentException for any validation failure. */ public Variant(ByteBuffer value, ByteBuffer metadata) { From 6632567fb95aa3f226120a24169206b45efd4a0a Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Sat, 24 Oct 2026 16:34:25 +0100 Subject: [PATCH 10/12] Variant Hardening: relax checks on array ranges during construction ...they will happen during resolution. --- .../apache/parquet/variant/VariantUtil.java | 34 ++++++++++++++----- .../parquet/variant/TestHardenedReader.java | 21 ++++++++---- 2 files changed, 40 insertions(+), 15 deletions(-) diff --git a/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java b/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java index 0d5b045447..8cfaa60dd9 100644 --- a/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java +++ b/parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java @@ -914,7 +914,7 @@ static HashMap getMetadataMap(ByteBuffer metadata) { * Bounds-checks a single Variant value node against its buffer slot. Performs no recursion * into nested children: child nodes are checked on demand when callers descend into them. * - *

Cost: O(1) for primitives and short strings, O(numElements) for objects and arrays. + *

Cost: O(1) for primitives, short strings, and arrays; O(numElements) for objects. * Validation of nested structures is deferred so that opening a large well-formed Variant * is not penalized by sub-trees the caller never inspects. * @@ -924,7 +924,8 @@ static HashMap getMetadataMap(ByteBuffer metadata) { * @param valueBuffer the variant value buffer (position/limit define the extent of this node's slot) * @param dictSize the metadata dictionary size, used to bound object field ids * @throws IllegalArgumentException if the value header or container table does not fit within - * the buffer slot, or if any object field id is out of range + * the buffer slot, if an array's end offset extends past the buffer slot, or if any object + * field id or child offset is out of range */ static void validateValueShallow(final ByteBuffer valueBuffer, final int dictSize) { int pos = valueBuffer.position(); @@ -950,6 +951,8 @@ static void validateValueShallow(final ByteBuffer valueBuffer, final int dictSiz /** * Shallow validation of a container. + *

O(elements) for an object; O(1) for an array, + * the latter with a lot less validation. * * @param valueBuffer buffer with the variant data * @param typeInfo type information from the metadata @@ -987,19 +990,32 @@ private static void validateContainerShallow( dataStart <= length, "variant container offset table extends past buffer: numElements=%s", numElements); long dataLen = length - dataStart; if (isObject) { + // validate the dictionary elements for (int i = 0; i < numElements; i++) { int id = readUnsignedLittleEndian(valueBuffer, pos + (int) idStart + i * idSize, idSize); Preconditions.checkArgument( id < dictSize, "variant object key id %s out of range (dictSize=%s)", id, dictSize); } - } - // Each child offset must lie within the data region. Children may overlap or leave gaps; - // the trailing terminator offset is range-checked for the same reason. - for (int i = 0; i <= numElements; i++) { - // O(elements) - int off = readUnsignedLittleEndian(valueBuffer, pos + (int) offsetStart + i * offsetSize, offsetSize); + // validate the child offsets are in range. + // This is O(elements), but so is the + // Children may overlap or leave gaps, but must remain in the object's data region. + for (int i = 0; i <= numElements; i++) { + int off = readUnsignedLittleEndian(valueBuffer, pos + (int) offsetStart + i * offsetSize, offsetSize); + Preconditions.checkArgument( + off <= dataLen, "variant child offset out of range: %s (data length %s)", off, dataLen); + } + } else { + // array validation: make sure the end offset isn't outside the data region. + // It ensures the container’s declared data region itself does not run past its slot. + // Without it, even an empty array—or an array whose accessed element offsets happen to be valid—can claim + // an + // end offset beyond the available bytes. + int endOffset = readUnsignedLittleEndian(valueBuffer, pos + (int) dataStart - offsetSize, offsetSize); Preconditions.checkArgument( - off <= dataLen, "variant child offset out of range: %s (data length %s)", off, dataLen); + endOffset <= dataLen, + "variant array end offset out of range: %s (data length %s)", + endOffset, + dataLen); } } diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java index c618d812f2..b05fc3f490 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java @@ -96,16 +96,25 @@ public void testWellFormedRoundTrip() throws IOException { assertThat(v.getElementAtIndex(0).getType()).isEqualTo(Variant.Type.NULL); } - /** - * Out-of-range child offset. - */ @Test - public void testOutOfRangeChildOffset() throws IOException { + public void testArrayChildOffsetIsCheckedOnAccess() throws IOException { byte[] value = arrayOneNull(); // Layout of arrayOneNull(): [header, numElements=1, offset[0]=0, offset[1]=1, NULL_header] - // Set offset[0] to 0xFF — far outside the 1-byte data region. + // Set offset[0] to 0xFF, beyond the 1-byte data region. value[2] = (byte) 0xFF; - expectRoundTripRaisesIllegalArgument(EMPTY_METADATA, value, "child offset"); + byte[][] roundTripped = roundTrip(EMPTY_METADATA, value); + Variant top = new Variant(ByteBuffer.wrap(roundTripped[1]), ByteBuffer.wrap(roundTripped[0])); + + assertThat(top.getType()).isEqualTo(Variant.Type.ARRAY); + assertThatThrownBy(() -> top.getElementAtIndex(0)).isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void testArrayEndOffsetMustFitInDataRegion() throws IOException { + byte[] value = arrayOneNull(); + // Set offset[1], the end offset, beyond the 1-byte data region. + value[3] = (byte) 0xFF; + expectRoundTripRaisesIllegalArgument(EMPTY_METADATA, value, "array end offset"); } /** From 66002fa840fe65285c6154d62973bdec08d8fc2d Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Sat, 24 Oct 2026 17:04:15 +0100 Subject: [PATCH 11/12] Late checking requires late checks ..read in data size and validate offset before reading. end of data offset is cached. note that validation has prefetched these values but it isn't passed back. Pity, but we'd have to return some object/array value tuple with the info extracted from each. Not impossible in future though... --- .../org/apache/parquet/variant/Variant.java | 21 +++++++++++++++++++ .../parquet/variant/TestHardenedReader.java | 4 +++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java b/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java index b3b2081d2b..e963436f88 100644 --- a/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java +++ b/parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java @@ -62,6 +62,11 @@ public final class Variant { */ private VariantUtil.ArrayInfo cachedArrayInfo; + /** + * Lazy cache for the parsed array offset; saved when {@link #cachedArrayInfo} is saved. + */ + private int cachedArrayEndOffset; + /** * Child depth of this Variant relative to the top-level value (0 = top-level). * Arrays and short strings are children here, along with nested variants. @@ -396,6 +401,9 @@ public Variant getElementAtIndex(int index) { int dataStart = value.position() + info.dataStartOffset; int offset = VariantUtil.readUnsignedLittleEndian(value, offsetStart + info.offsetSize * index, info.offsetSize); + int endOffset = arrayEndOffset(); + Preconditions.checkArgument( + offset <= endOffset, "variant array child offset out of range: %s (end offset %s)", offset, endOffset); return childVariant(VariantUtil.slice(value, dataStart + offset)); } @@ -448,7 +456,20 @@ private VariantUtil.ArrayInfo arrayInfo() { if (info == null) { info = VariantUtil.getArrayInfo(value); cachedArrayInfo = info; + cachedArrayEndOffset = VariantUtil.readUnsignedLittleEndian( + value, + value.position() + info.offsetStartOffset + info.offsetSize * info.numElements, + info.offsetSize); } return info; } + + /** + * Return the array end offset, parsing it on deman. + * @return array end offset. + */ + private int arrayEndOffset() { + arrayInfo(); + return cachedArrayEndOffset; + } } diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java index b05fc3f490..fb02ed7cd4 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java @@ -106,7 +106,9 @@ public void testArrayChildOffsetIsCheckedOnAccess() throws IOException { Variant top = new Variant(ByteBuffer.wrap(roundTripped[1]), ByteBuffer.wrap(roundTripped[0])); assertThat(top.getType()).isEqualTo(Variant.Type.ARRAY); - assertThatThrownBy(() -> top.getElementAtIndex(0)).isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> top.getElementAtIndex(0)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("array child offset"); } @Test From ad6d5998d34777cb7225a1bfca0a7b0d8f558c35 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Wed, 28 Oct 2026 19:00:16 +0000 Subject: [PATCH 12/12] Track down that junit4/5 issue. Not the assertions, it's @Test and the rule to get the test method --- .../parquet/variant/TestHardenedReader.java | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java index fb02ed7cd4..7c589c41f9 100644 --- a/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java +++ b/parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java @@ -43,9 +43,9 @@ import org.apache.parquet.io.api.Binary; import org.apache.parquet.schema.MessageType; import org.apache.parquet.schema.MessageTypeParser; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.TestName; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; /** * Test that the hardened variant reader behaves as expected. @@ -80,8 +80,12 @@ public class TestHardenedReader { /** Directory under the module's build dir where generated files are written, if enabled. */ private static final Path OUTPUT_DIR = Paths.get("target", "test-hardened-reader"); - @Rule - public TestName testName = new TestName(); + private String testName; + + @BeforeEach + void setTestName(TestInfo testInfo) { + testName = testInfo.getTestMethod().orElseThrow().getName(); + } /** * Happy path. @@ -647,7 +651,7 @@ private byte[][] roundTrip(byte[] metadata, byte[] value) throws IOException { // optional save to fs for later testing. if (SAVE_GENERATED_FILES) { Files.createDirectories(OUTPUT_DIR); - Path target = OUTPUT_DIR.resolve(testName.getMethodName() + ".parquet"); + Path target = OUTPUT_DIR.resolve(testName + ".parquet"); Files.write(target, parquetBytes); } // read it back