API: Harden variant binary parsing against malformed input

[nssalian]

Closes #16334
Addresses #16455

Summary

Validates malformed Variant binary input so adversarial buffers fail fast with IllegalArgumentException instead of OOM, StackOverflowError, or raw JVM exceptions. Adds bounds checks across header, count, offset table,
primitive payload, and short-string length, plus a 1000-level nesting depth cap threaded through object/array recursive descent.

The approach mirrors Steve Loughran's parquet-java hardening work in apache/parquet-java#3562. Pulled in the changes from @steveloughran's #16335

Changes

• SerializedMetadata, SerializedArray, SerializedObject: header / count / offset-table bounds, lazy per-element offset and field-id checks, long arithmetic to prevent int overflow.
• SerializedPrimitive: payload bounds, BIN/STR size-field bounds, non-negative size.
• SerializedShortString: length bounds.
• VariantUtil: MAX_VARIANT_DEPTH = 1000 (depth <= cap, matching parquet-java 3562) and MAX_ELEMENTS = 16_777_216 absolute element-count cap (iceberg-specific; parquet-java 3562 bounds by buffer size only), threaded through fromBuffer recursive descent.
• TestMalformedVariant: attack-payload tests covering each new check

Behavioral change (beyond just hardening)

SerializedMetadata end-offset validation now compares against metadata.remaining() instead of metadata.limit(). The old comparison of a position-relative endOffset against the absolute limit() was incorrect for any buffer with a non-zero position. Flagging it separately from the bounds-check hardening since it changes read behavior.

Test plan

• ./gradlew :iceberg-api:test --tests 'org.apache.iceberg.variants.*'

[steveloughran]

I've just realised that this and the parquet-java hardening don't worry about leftover data. "don't do that" is implicit the policy there, being as it is useless.

I wonder what the rust parquet reader does.

[nssalian]

Will be matching parquet-java's implicit "don't do that" stance. Happy to revisit if the dev list lands on rejecting trailing bytes.

[laskoviymishka]

Nice hardening here. TestMalformedVariant covers a good spread of crafted-buffer cases, and moving the offset-table checks to long is the right direction.

I’d hold on one bug and one design question.

The bug: in both SerializedArray and SerializedObject, we compute the offset-table bound in long, check it, then immediately assign dataOffset using plain int arithmetic. With 4-byte offsets this can still overflow around ~536M elements. The long guard passes, dataOffset wraps negative, and we get a ByteBuffer underflow instead of the IllegalArgumentException this PR is trying to guarantee.

The design question: MAX_VARIANT_DEPTH = 500 is a non-spec limit. Parquet Variant doesn’t cap nesting, and Iceberg defers to that. So a >500-level variant written by Spark or another client could read elsewhere but fail here at scan time. What did parquet-java#3562 settle on? I’d either match that or document why 500 is the right cap.

Once the overflow is fixed and the depth-limit decision is clear, happy to take another pass.

[laskoviymishka]

Same int-overflow as the array side: offsetListOffset and dataOffset below are assigned in plain int while dataStart here is long. numElements * fieldIdSize and (1 + numElements) * offsetSize can overflow before the long guard's bound is ever applied to the int fields, wrapping the data offset negative for a large-but-valid count.

I'd derive both offsets from the long intermediates and Math.toIntExact them so the guard actually protects the value we use.

[nssalian]

Fixed - both offsets are now derived from a long dataStart intermediate and Math.toIntExact-ed, so the guard protects the values we use downstream.

[laskoviymishka]

The Javadoc on MAX_VARIANT_DEPTH says a variant "may contain up to MAX_VARIANT_DEPTH nested levels," but depth <= MAX_VARIANT_DEPTH with the top-level at depth 0 actually admits 501 levels (depths 0 through 500). It's internally consistent, just off-by-one against the comment.

I'd either flip this to depth < MAX_VARIANT_DEPTH or update the Javadoc to state the counting model exactly. A depth >= 0 lower bound here would also close the direct-call path where depth + 1 could overflow past the cap.

[nssalian]

Fixed the doc to match: "top-level value is depth 0, so a Variant may contain up to MAX_VARIANT_DEPTH + 1 nested levels."

[laskoviymishka]

This is a non-spec limit — the Parquet VariantEncoding spec doesn't cap nesting depth and Iceberg defers to it. A valid variant with >500 levels written by Spark or another client would read fine elsewhere but throw here at scan time, which reads as silent data loss rather than malformed-input rejection.

What did parquet-java#3562 settle on? If it picked a different value (or no cap), we'd diverge on file interop. I'd at minimum cross-reference that decision in a comment here and confirm 500 is high enough to never reject real data (it's below Jackson's default of 2000).

[steveloughran]

no real reason, except it was what was in org.apache.parquet.variant.VariantJsonParser

how about we discuss on parquet dev list?

I don't really care, just that there's some limit and that it's broadly consistent.

[steveloughran]

I've started a discussion on the parquet dev list, please get involved. Looking at jackson, seems like 1000 is their limit right now, so copying that would be consistent. Let's see what the mailing list discussion outcome is though and we can all go with that, and add to the test dataset.
FasterXML/jackson-core#943

[nssalian]

Following the parquet dev list thread Steve started: [DISCUSS] Variant Hardening: how deep is a realistic variant depth? (https://lists.apache.org/thread/q6wbom1q9pndv2nj6wcynxjcjxxkc1hm). I have aligned MAX_VARIANT_DEPTH to 1000 (jackson's 1000 seems the likely outcome).

[steveloughran]

I've set the parquet to 1000 too, so we are all now consistent

[steveloughran]

claude looking at your tests after taking the ones from the suite that weren't in the parquet lib one and replicating

                                                                                                                                          
                                                                                                                                                                                                                                                                
  ┌────────────────────────────────────────┬──────────────────────────────────────────────────────────────────────────────────────────────────┐                                                                                                                 
  │               Local test               │                                         Why it's unique                                          │                                                                                                                 
  ├────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────┤                                                                                                                 
  │ testWellFormedRoundTrip                │ Happy-path baseline. Iceberg's file is malformed-only, so it has no positive control.            │                                                                                                                 
  ├────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────┤                                                                                                                 
  │ testEmptyMetadataRejected              │ Zero-byte metadata → IllegalArgumentException "empty". No empty-metadata case in the Iceberg 20. │                                                                                                                 
  ├────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────┤                                                                                                                 
  │ testSingleByteMetadataRejected         │ Lone version byte (below the 3-byte spec minimum) → "truncated". No analog.                      │                                                                                                                 
  ├────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────┤                                                                                                                 
  │ testUnsupportedMetadataVersionRejected │ Version bits ≠ 1 → UnsupportedOperationException. Iceberg has no version-check test.             │                                                                                                                 
  └────────────────────────────────────────┴──────────────────────────────────────────────────────────────────────────────────────────────────┘                                                                                                                 
                                                                                                                                                                                                                                                                

[steveloughran]

I've started a discussion on the parquet dev list, please get involved. Looking at jackson, seems like 1000 is their limit right now, so copying that would be consistent. Let's see what the mailing list discussion outcome is though and we can all go with that, and add to the test dataset.
FasterXML/jackson-core#943

[laskoviymishka]

Thanks for this round. Most of the earlier comments are addressed now, and the overall shape is much better. The centralized VariantUtil.fromBuffer dispatch is clean, the depth threading is consistent, and the long-arithmetic guards in the array/object constructors look solid.

A couple more things before approving.

The blocker is duplicate field offsets in SerializedObject.initOffsetsAndLengths. offsetToLength is keyed by field offset, so two fields sharing the same byte offset collapse into one entry. Then sortedOffsets.size() < numElements + 1 and the parser rejects the object.

That is too strict. The spec allows non-monotonic field offsets, and parquet-java #3562 explicitly tolerates overlapping or duplicate offsets. So this would reject data that parquet-java, Spark, and iceberg-rust can read. I’d drop the duplicate-offset assertion and compute lengths with a sorted pass that handles equal offsets. testDuplicateFieldOffsetsInObject should become a positive test.

The other issue is allocation size. offsetTableEnd <= value.remaining() bounds numElements by the input buffer, but with offsetSize=1 that can still mean hundreds of millions of elements. We then allocate new VariantValue[numElements], plus several more arrays on the object path. A crafted but physically valid buffer can therefore trigger a much larger heap allocation and OOM the executor.

I’d add an absolute numElements cap before allocation, ideally matching parquet-java #3562, with a test for the large-but-valid-buffer case.

One smaller consistency fix: SerializedMetadata.dataOffset should use Math.toIntExact(offsetTableEnd), like the array/object constructors.

One question, not a blocker: can anything take sliceValue(...) output and parse it again through VariantValue.from? That would reset the depth counter. The in-tree recursive paths all pass depth + 1, so they look fine.

The rest can follow up: depth-cap docs, lazy field-ID validation, @VisibleForTesting consistency, and test naming.

Once duplicate offsets and the allocation cap are handled, happy to take another pass.

[steveloughran]

@laskoviymishka great comments here, would love the reviews on the parquet one as you know details that we may have missed. I don't want these parsing tests to be stricter than the spec and so reject valid files created by other tools.

@nssalian just added fixed depth to 1000 + two new tests

• you should skip an unknown primitive type. (not a MUST, just a SHOULD).
• it's ok for two dictionary keys to point to same offset (from the comments here)