GH-3561 Harden variants against malformed metadata. by steveloughran · Pull Request #3562 · apache/parquet-java

steveloughran · 2026-05-14T11:53:50Z

Rationale for this change

Malformed parquet files could be distruptive enough to not only affect the execution of a single worker thread (which will ultimately reject it), but other threads on the same process. This can be disruptive.

What changes are included in this PR?

reject oversized metadata/value declarations
reject oversize dictSize in objects
range checking

Only low cost checks are made, equivalent to arrow variant try_new_with_metadata_and_shallow_validation()

There's no equivalent with_full_validation() logic is omitted. The caching logic of #3481 may be able to do this when it builds a dictionary, as range checking the increasing dictionary offsets is the key work there.

There's also a depth check consistent with the json parser; it's arguable as to whether that is needed. It will defend against StackOverflowExceptions by anything trying to treewalk, but shouldn't that code be the place to do the checks?

Are these changes tested?

The new test suite TestHardenedReader can be configured to actually emit the malformed files, to see how applications deal with them.

Existing tests needed changes because they'd been getting away with malformed byte arrays; now this stuff is being checked the test cases need valid variants.

Are there any user-facing changes?

No

Closes #3561

steveloughran · 2026-05-21T13:10:24Z

@rdblue why not review this PR before worrying about the iceberg one, as its where some things could be clarified and then repeated.

replicate arrow's "full verification"? they have a verified flag and do that on demand. Primarily one of verifying that the dictionary keys are all good. More expensive. If you are just scanning through records doing filtering and projection, I don't see it's needed.
what is a good depth limit for children?

steveloughran · 2026-05-26T13:52:22Z

@Fokko could you look at this while I work on that thrift update?

Fokko

This look good to me, thanks @steveloughran for working on this. I left a few comments. Should we also test the performance implications for list/dicts? Maybe a small JMH microbenchmark.

Fokko · 2026-05-27T19:55:57Z

+  // 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});


I assume that we don't accept the single byte anymore? Should we still allow that, or make it explicit to the users?

good point. my reading of the spec says header includes 'offset size -1`
set offset-size -1 to 0 and you have

dictionary size = 1

metadata size = 1

This is something maybe to call out/clarify in the docs, but I don't see it being possible to declare a zero byte dictionary

update: no, it's impossible to generate a valid zero byte variant

fail fast with an explicit message (allows a branch to be cut later)

test

Fokko · 2026-05-27T19:59:01Z

      this.dictSize = 0;
    }
    this.metadataCache = null;
+    VariantUtil.validateValueShallow(this.value, dictSize);


Since we now validate in the constructor, should we add some Javadoc to this public function? Including the @throws IllegalArgumentException

steveloughran · 2026-06-01T09:44:20Z

Latest status. Ran benchmarks unattended; still lot of variation where even some tests which only write data are being statisticially significantly faster to serialize. That's a codepath which isn't being updated, and implies that system/other work is interfering.

Results: https://github.com/steveloughran/benchmarking-variants/tree/main/json/hardening
final hardened json and baseline master

Comparing these with JMH tabulate (important: use my fork as it strictly enforces a safe version of the charting.js lib from npm):

This looks like a slowdown but filter on statistical significance and most vanish and of the three which are significant, two are writing data not reading it

the two write variants are just serializing the prebuilt variant, so not stressing the code. the reader does on a deeply nested structure, but there I'm not sure anything is showing

Summary: even though minor some statistically significant slowdown is being reported, the fact that speedups in unmodified codepaths are also observed tells me the results aren't reliable.

Whatever changes are being made here, they aren't actually measurable in the new dataset and general os/execution/jvm noise is more of a factor

updated all the constructor javadocs

steveloughran · 2026-06-25T16:52:48Z

latest status

rebase to master
explicit zero byte metadata rejection and test
new test for invalid version
javadocs review
switch to readUnsignedLittleEndian() to read the metadata during validation

steveloughran · 2026-07-07T12:15:12Z

@Fokko any chance of a final review + merge? there's nothing left to do that I can see here...any else would be feature creep.

steveloughran · 2026-07-07T13:37:44Z

feature creep: handle unrecognised primitive type as per spec SHOULD "an implementation should be able to read the rest of the Variant value"

steveloughran · 2026-07-07T17:14:27Z

@laskoviymishka can you review this alongside the iceberg PR apache/iceberg#16568 ? we are trying to harden them together

steveloughran · 2026-07-09T14:58:32Z

squash and rebase to deal with the assertj move. happy with the move though!

- reject oversized metadata/value declarations - range checking - depth check of 1000 elements - if metadata length == 0, fail with specific error. With tests - reject version != 1 - skip unknown primitive types.

laskoviymishka

I reviewed the original Iceberg PR this was ported from, apache/iceberg#16568, so I mostly looked at this through a parity lens rather than reopening the design. The shallow validation on descent, depth threading through childVariant, and primitive size table all match what landed in Iceberg.

I’m not a committer here, so please weigh these against parquet-java conventions. Some differences may be intentional.

The one thing I’d want another look at before merge is in validateContainerShallow:

(long) (numElements + 1) * offsetSize

The cast happens after numElements + 1, so that addition is still int arithmetic. With numElements == Integer.MAX_VALUE, it wraps negative, dataStart goes negative, the bounds check passes, and the offset loop can read out of bounds. This is reachable for arrays.

The Iceberg version handled this class with a MAX_ELEMENTS cap, which doesn’t seem to have made it into the port. The line above already widens correctly with (long) numElements * idSize, so this may just need:

((long) numElements + 1) * offsetSize

plus the cap from the original. I may be missing a parquet-java-specific reason this is unreachable, though.

testOffsetArithmeticBoundsCheck also doesn’t seem to hit the wrap. 0x33333333 fails because the offset table is too large, not because numElements + 1 overflows, and the test doesn’t assert the error message. I’d retarget it at Integer.MAX_VALUE for both array and object cases.

Smaller parity/doc notes:

MAX_VARIANT_DEPTH is an implementation safety limit, not a spec limit. Worth saying that in Javadoc, since deeper variants from other engines would be rejected here.
The same change raises JSON parser depth from 500 to 1000, which may be worth a release note.
A one-byte metadata buffer that used to construct now throws. Probably correct, but still a behavior change.
The round-trip test pulls in parquet-hadoop; the Iceberg tests used direct ByteBuffer construction. Not blocking, just checking whether the extra harness is needed here.

I’ll defer the per-descent validation cost to Fokko’s JMH thread. Other than the overflow line and test coverage, this looks close.

laskoviymishka · 2026-07-10T08:48:24Z

    </dependency>
+    <dependency>
+      <groupId>org.apache.parquet</groupId>
+      <artifactId>parquet-hadoop</artifactId>


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.

laskoviymishka · 2026-07-10T08:48:24Z

   */
  private VariantUtil.ArrayInfo cachedArrayInfo;
+  /** Nesting depth of this Variant relative to the top-level value (0 = top-level). */
+  private final int depth;


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.

laskoviymishka · 2026-07-10T08:48:24Z

-      this.dictSize = 0;
-    }
+    Preconditions.checkArgument(
+        this.metadata.remaining() >= 1 + metaOffsetSize,


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.

laskoviymishka · 2026-07-10T08:48:24Z

  private static final JsonFactory JSON_FACTORY = JsonFactory.builder()
      .streamReadConstraints(StreamReadConstraints.builder()
-          .maxNestingDepth(500)
+          .maxNestingDepth(MAX_VARIANT_DEPTH)


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?

laskoviymishka · 2026-07-10T08:48:24Z

+   * Maximum permitted nesting depth of a Variant value.
+   * same limit as in VariantJsonParser.
+   */
+  static final int MAX_VARIANT_DEPTH = 1000;


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.

laskoviymishka · 2026-07-10T08:48:24Z

+   */
+  static void validateValueShallow(final ByteBuffer valueBuffer, final int dictSize) {
+    int pos = valueBuffer.position();
+    Preconditions.checkArgument(pos >= 0 && pos < valueBuffer.limit(), "variant value is empty");


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.

laskoviymishka · 2026-07-10T08:48:24Z

+  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;


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.

laskoviymishka · 2026-07-10T08:48:24Z

+   * @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.


Looks like a copy-paste from validatePrimitiveShallow: the @param says "length of slot for this primitive" but this is a container.

laskoviymishka · 2026-07-10T08:48:24Z

+    long idStart = 1L + sizeBytes;
+    long idBytes = isObject ? (long) numElements * idSize : 0L;
+    long offsetStart = idStart + idBytes;
+    long offsetBytes = (long) (numElements + 1) * offsetSize;


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:

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?

laskoviymishka · 2026-07-10T08:48:25Z

+    // 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};


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.

steveloughran changed the title ~~Harden variants against malformed metadata.~~ GH-3561 Harden variants against malformed metadata. May 14, 2026

steveloughran force-pushed the pr/variant-hardening branch from 636e75d to 9905728 Compare May 14, 2026 11:55

steveloughran mentioned this pull request May 14, 2026

Optimizing Variant read path with lazy caching #3481

Merged

steveloughran force-pushed the pr/variant-hardening branch from 9905728 to db1cfe2 Compare May 14, 2026 13:45

steveloughran mentioned this pull request May 14, 2026

Api: Harden Variant Reading apache/iceberg#16335

Closed

steveloughran force-pushed the pr/variant-hardening branch from db1cfe2 to 27d92e6 Compare May 19, 2026 12:22

nssalian mentioned this pull request May 20, 2026

Variant parsing is vulnerable to memory-DoS and malformed-input crashes apache/iceberg#16455

Open

nssalian mentioned this pull request May 26, 2026

API: Harden variant binary parsing against malformed input apache/iceberg#16568

Open

Fokko reviewed May 27, 2026

View reviewed changes

steveloughran force-pushed the pr/variant-hardening branch from efdfb6a to 240bb56 Compare June 25, 2026 16:47

steveloughran mentioned this pull request Jun 26, 2026

GH-112 Harden variant decoding: example data apache/parquet-testing#113

Open

apacheGH-3561 Harden variant decoding

ce40d68

- reject oversized metadata/value declarations - range checking - depth check of 1000 elements - if metadata length == 0, fail with specific error. With tests - reject version != 1 - skip unknown primitive types.

steveloughran force-pushed the pr/variant-hardening branch from 176420f to ce40d68 Compare July 9, 2026 15:38

laskoviymishka reviewed Jul 10, 2026

View reviewed changes

		// 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});

Uh oh!

Conversation

steveloughran commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

steveloughran commented May 21, 2026

Uh oh!

steveloughran commented May 26, 2026

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveloughran commented Jun 1, 2026

Uh oh!

steveloughran commented Jun 25, 2026

Uh oh!

steveloughran commented Jul 7, 2026

Uh oh!

steveloughran commented Jul 7, 2026

Uh oh!

steveloughran commented Jul 7, 2026

Uh oh!

steveloughran commented Jul 9, 2026

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

steveloughran commented May 14, 2026 •

edited

Loading