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.*'

[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.

[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.             │                                                                                                                 
  └────────────────────────────────────────┴──────────────────────────────────────────────────────────────────────────────────────────────────┘                                                                                                                 
                                                                                                                                                                                                                                                                

[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)

[laskoviymishka]

LGTM,

One thing to track rather than block on: this mirrors apache/parquet-java#3562, which is still open.

Worth keeping the two in parity as that one evolves (specifically the nesting-depth cap value, overlapping-offset tolerance, and the skip-unknown-primitive behavior) so a buffer that reads in one client doesn't fail in the other.

Might be worth a short follow-up issue to reconcile once #3562 lands.

[nssalian]

Thanks for the review @laskoviymishka . Will keep track of the parquet PR. @steveloughran PTAL here before this goes in. I'll add follow ups.

[nssalian]

@steveloughran @laskoviymishka I added a test for covering the unknown-primitive-type-id case. PTAL

[laskoviymishka]

👍

[steveloughran]

new test good, lifted one of the existing tests for the parquet pr and commented on the array size.

[steveloughran]

what about java17 switch now there's been the move and this is new, or at least moved, code?

[steveloughran]

missed this one; lifting it for the parquet test.

[steveloughran]

the parquet pr doesn't enforce any limit here, and I'm not going to worry about one; the cost of array size is less than recursing down nested structures and I'm not aware of other bits of parquet imposing limits other than "you are free to run out of memory if you want to"