laskoviymishka commented on code in PR #3562:
URL: https://github.com/apache/parquet-java/pull/3562#discussion_r3557578691
##########
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;
Review Comment:
Minor: depth here counts navigation depth through Variant objects, so it
also ticks up for leaf primitives and short strings, not just container
nesting. Harmless — a one-line note on the field might just save the next
reader a double-take.
##########
parquet-variant/pom.xml:
##########
@@ -69,6 +69,17 @@
<version>${slf4j.version}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.parquet</groupId>
+ <artifactId>parquet-hadoop</artifactId>
Review Comment:
The Iceberg tests for this stayed at direct ByteBuffer construction, so I
was a little surprised to see the round-trip harness pull parquet-hadoop and
hadoop-client into parquet-variant's test scope. Since Parquet's binary column
encoding is just a length-prefixed byte copy, I don't think most of these cases
exercise a different construction path than wrapping the bytes directly. Might
be worth keeping the round-trip for the one or two cases where encode/decode
genuinely matters — but you'll know the parquet-java test conventions better
than I do.
##########
parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java:
##########
@@ -880,6 +886,158 @@ static HashMap<String, Integer> 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.
+ *
+ * <p>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");
Review Comment:
Small thing: pos is `valueBuffer.position()`, which is never negative by
ByteBuffer's contract, so the `pos >= 0` half can't fire. Might read more
clearly dropped, or with a comment on why it's kept.
##########
parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java:
##########
@@ -880,6 +886,158 @@ static HashMap<String, Integer> 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.
+ *
+ * <p>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;
Review Comment:
One subtlety worth a doc note (not a change, I think): `slot` is bounded by
the top-level buffer's limit, not this node's parent-declared extent, since
slice() doesn't set a limit. So a child can — truthfully per its own size —
extend into bytes that logically belong to a sibling or the parent's offset
table. It's memory-safe since it's the same backing array, and this matches the
shallow-not-full split on the Iceberg side, so I'd just note the limitation
rather than tighten it.
##########
parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java:
##########
@@ -880,6 +886,158 @@ static HashMap<String, Integer> 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.
+ *
+ * <p>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.
Review Comment:
Looks like a copy-paste from validatePrimitiveShallow: the @param says
"length of slot for this primitive" but this is a container.
##########
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;
Review Comment:
I checked and the spec doesn't define a max nesting depth — the Iceberg side
treats this as a safety limit too, so I think it's a reasonable cap. The one
thing I'd flag is interop: a spec-valid variant nested deeper than 1000,
written by Spark/PyIceberg/Arrow, would be rejected here. Might be worth a
Javadoc note that it's an implementation limit rather than a spec rule, so that
expectation is clear.
##########
parquet-variant/src/main/java/org/apache/parquet/variant/VariantJsonParser.java:
##########
@@ -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)
Review Comment:
This bumps the JSON parse-depth boundary from 500 to 1000 as a side effect
of sharing the constant, so documents that used to overflow at 501 now parse.
Since buildJson/buildJsonArray/buildJsonObject recurse on the real JVM stack, I
wasn't sure whether 1000 had been validated as safe on this path specifically
(as opposed to the descent path). Might be worth a release note, and maybe a
test that parses ~1000-level JSON on a realistic stack. wdyt?
##########
parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java:
##########
@@ -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,
Review Comment:
This looks like a genuine fix, but it's also a public-API behavior change
that might be worth a release note. A 1-byte metadata buffer (just the version
byte) used to construct successfully with dictSize=0 via the old `else` branch,
and now throws — so any downstream caller relying on that leniency would start
seeing IllegalArgumentException.
##########
parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java:
##########
@@ -880,6 +886,158 @@ static HashMap<String, Integer> 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.
+ *
+ * <p>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;
Review Comment:
This is the one I'd most want a second look at, since it's where the port
seems to have drifted from the Iceberg original.
`(numElements + 1)` here is computed as int arithmetic before the `(long)`
widening applies, so a largeSize container with `numElements ==
Integer.MAX_VALUE` wraps `offsetBytes` negative — `dataStart` goes negative,
the `dataStart <= length` guard passes, and the loop below then reads ~2^31
entries out of bounds (reachable for an ARRAY, where idBytes is 0). It'd
surface as IndexOutOfBoundsException rather than the IllegalArgumentException
the Javadoc promises.
The line two above already widens the right way (`(long) numElements *
idSize`), as does Variant.java (`((long) this.dictSize + 1) * metaOffsetSize`),
so it may just be:
```java
long offsetBytes = ((long) numElements + 1) * offsetSize;
```
The Iceberg original also capped numElements with a MAX_ELEMENTS bound to
make this class unreachable, which doesn't seem to have come across in the
port. I could be missing a parquet-java-side reason it can't be hit — a
regression test at `Integer.MAX_VALUE` for ARRAY and OBJECT would confirm
either way. wdyt?
##########
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};
Review Comment:
Comparing against the Iceberg tests, I don't think this one quite reaches
the overflow it's named for. numElements here (0x33333333, ~858M) doesn't wrap
on `+1` — so it passes via the offset-table-too-big path, which was already
correct, rather than the widened arithmetic. Passing "" for the message
substring also means the failure text isn't asserted. If the overflow in the
line above is real, this test would stay green.
If it turns out to be a real gap, retargeting at `numElements ==
Integer.MAX_VALUE` with a largeSize ARRAY header (and one for OBJECT),
asserting the specific exception type, would pin it down.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
