GH-3561 Harden variants against malformed metadata. #3562
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(MAX_VARIANT_DEPTH) | ||
|
There was a problem hiding this 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?
Contributor
Author
There was a problem hiding this comment. That original 500 was completely chosen without any data as a general resilience number, the goal being "a malicious file can damage the stack of the thread processing it, but not the rest of the process". I went with the same number for the variant for consistency, and after a discussion in the mailing list went to 1k. we haven't shipped the json parser yet, so no need for a release note. I will look at #3415 notes to see if we should update it though |
||
| .maxStringLength(10_000_000) | ||
| .maxDocumentLength(50_000_000L) | ||
| .build()) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
There was a problem hiding this 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.
Contributor
Author
There was a problem hiding this comment. will add to the javadocs, with a link to the discussion https://lists.apache.org/thread/q6wbom1q9pndv2nj6wcynxjcjxxkc1hm |
||
|
|
||
| // header bytes | ||
| static final byte HEADER_NULL = primitiveHeader(NULL); | ||
| static final byte HEADER_LONG_STRING = primitiveHeader(LONG_STR); | ||
|
|
@@ -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"); | ||
|
There was a problem hiding this comment. Small thing: pos is
Contributor
Author
There was a problem hiding this comment. cut it; all that matters is limit > pos, and as there's a limit - pos calculation on the line below, do a check on that value to make clear what is really being checked is "is there data" |
||
| long slot = (long) valueBuffer.limit() - pos; | ||
|
There was a problem hiding this comment. One subtlety worth a doc note (not a change, I think): |
||
| 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. | ||
|
There was a problem hiding this comment. Looks like a copy-paste from validatePrimitiveShallow: the @param says "length of slot for this primitive" but this is a container. |
||
| * @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; | ||
|
There was a problem hiding this 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.
The line two above already widens the right way ( 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 |
||
| 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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to create the test files for apache/parquet-testing#113 ; so while I agree it's overkill, it's a test only dependency and an easy way to generate test data for interop testing.