Modified my parquet-variant PR to set a depth limit of 1000 I've also added test to the module, and a test file to https://github.com/apache/parquet-testing/pull/113 to verify that unknown primitive types are skipped, which the spec says libraries should let the rest of the variant be read "new types may be added to the specification without incrementing the version ID. In such a situation, an implementation should be able to read the rest of the Variant value if desired."
On Sat, 27 Jun 2026 at 15:50, Gunnar Morling <[email protected]> wrote: > Agreed; a parser should be able to reject malformed payloads by itself. > > Not all Parquet usage is via Iceberg, so having these guardrails in > the parser itself ensures they're applied consistently, no matter > what's the next upper layer in the application stack. > > --Gunnar > > On Sat, 27 Jun 2026 at 12:45, Steve Loughran <[email protected]> wrote: > > > > On Fri, 26 Jun 2026 at 21:22, Kurtis Wright <[email protected]> > wrote: > > > > > Hi Steven, > > > > > > Thank you for putting this together. I have a naive question, is > Parquet > > > the correct layer to build Reader performance guardrails that Writers > > > choose to ignore or should that be at an application level such as > Iceberg? > > > > > > > > We're trying to harden all ASF projects right now to be resilient to cves > > and emails to security@ about aI reports of CVEs. There's PRs up for > > parquet java and iceberg java for consistent rejection of the really > > disruptive variants, where integer multiplication wraparound permits a > > short payload to trigger a multi gb array allocation, which would be > > disruptive on any multi-user service. > > > > hardening the core reader lib ensures that layers above don't need to > know > > about limits but are resilient to malicious content. > > > > the issue with the nesting depth is very much about that you want to > limit > > deep documents having adverse consequences on other threads in a process, > > and for the thread processing a file, say on a server, it's good to have > it > > fail robustly and consistently > > > > maybe the format doc should say something like > > > > *Implementations SHOULD be resilient to deeply nested variants. If there > is > > tracking and rejection of deep nesting, then the threshold for rejection > > MUST be at least XXXX levels* > > > > and we can discuss levels here. The issue then is not "what is the > limit?", > > but "what is a consistent minimum threshold?" > > > > > > > On Fri, Jun 26, 2026 at 12:05 PM Steve Loughran <stevel@ .com > > > <[email protected]>> wrote: > > > > > > > I've got a PR up to harden variant parsing in parquet-jave, trying to > > > some > > > > shallow validation of inputs > > > > > > > > https://github.com/apache/parquet-java/pull/3562 > > > > ...with output saved and submitted as bad data to the test archive > > > > https://github.com/apache/parquet-testing/pull/113 > > > > > > > > Neelesh Salian has a matching PR for iceberg java > > > > https://github.com/apache/iceberg/pull/16568 > > > > > > > > Where there's an interesting question: what is a sensible depth > limit for > > > > variants? Too deep a variant creates stack problems, too shallow and > you > > > > can't represent data. > > > > > > > > my parquet-variant PR just took the 500 element depth from the json > > > parsing > > > > in org.apache.parquet.variant.VariantJsonParser, but really, it > would be > > > > good to have a consensus on what is a sensible depth for nested > variants > > > > which every library can expect other readers to cope with. > > > > > > > > Looking at jackson, there's a issue to link to all their issues, > > > including > > > > what's considered a CVE because it was a bit too brittle > > > > https://github.com/FasterXML/jackson-core/issues/637 > > > > > > > > And it seems like the current depth is 1000, though we should check > with > > > PJ > > > > Fanning there https://github.com/FasterXML/jackson-core/pull/943 > > > > > > > > > > > > 1. What depth do people think is good? > > > > 2. What limits do the other variant parsers apply? > > > > > > > > I don't care what the actual number is, what is important is that > > > consensus > > > > and everyone confident it's a good number. > > > > > > > > -Steve > > > > > > > >
