apache / apache/arrow-rs

parquet-variant: PartialEq on unvalidated deeply-nested variants overflows the stack (SIGABRT)

Open
#10,360 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
3.6k
Forks
1.3k
Avg merge
2d 14h
Merged PRs (30d)
167

Description

# `PartialEq` on unvalidated deeply-nested variants overflows the stack (SIGABRT)

## Describe the bug

`VariantList::eq` (`list.rs:313`) and `VariantObject::eq` are hand-written and recurse into
child values with no depth bound. Comparing two deeply-nested variants that were constructed
with the shallow `Variant::new` constructor overflows the stack and **aborts the process**
(`SIGABRT`). Unlike a panic, a stack-overflow abort cannot be contained by `catch_unwind`.

This is *not* closed by the validation depth limit added in #10352: that limit
(`MAX_NESTING_DEPTH`) is enforced inside `with_full_validation`, and `Variant::new` never calls
it. So `eq` on a `Variant::new` value has no gate.

## To Reproduce

```rust
use parquet_variant::Variant;

/// `depth` single-element lists nested around a null primitive.
fn make_nested_lists(depth: usize) -> Vec {
let mut value = vec![0u8]; // null primitive
for _ in 0..depth {
let mut outer = vec![0x0F, 1]; // array header, 4-byte offsets, num_elements=1
outer.extend_from_slice(&0u32.to_le_bytes());
outer.extend_from_slice(&(value.len() as u32).to_le_bytes());
outer.append(&mut value);
value = outer;
}
value
}

let metadata = [0x01u8, 0, 0]; // empty dictionary
let value = make_nested_lists(50_000);

// Variant::new does only shallow validation -- it never calls with_full_validation,
// so the depth limit that guards try_new is never on this path.
let a = Variant::new(&metadata, &value);
let b = Variant::new(&metadata, &value);

let _ = a == b; // recurses once per level with no bound -> stack overflow
```

Output:

```
thread '...' has overflowed its stack
fatal runtime error: stack overflow, aborting
(signal: 6, SIGABRT: process abort signal)
```

(Depth 50,000 is well past the edge; on a 2 MiB stack it aborts in the low thousands of levels.)

## Root cause

The depth counter that bounds recursion lives in `with_full_validation`. `Variant::new`
performs only constant-time shallow validation and never calls it, so nothing gates the
recursion in the hand-written `eq` implementations for lists and objects.

## Expected behavior

Comparing two variants should never abort the process, even for unvalidated inputs. Recursion
should be bounded inside `eq` itself — e.g. a depth guard, or an iterative comparison — so that
`==` on a `Variant::new` value fails gracefully (or simply returns) rather than overflowing the
stack.

## Additional context

Reachability is narrower than a typical parsing bug: it requires opting out of validation via
`Variant::new` and then comparing. Validated input (via `try_new`) is capped at
`MAX_NESTING_DEPTH` by #10352, so this cannot be reached through the validating path.

One thing worth calling out regardless of how the fix is prioritised: the type documentation
warns that infallible access on an *unvalidated* instance "may panic". That guarantee is weaker
than it reads here — a stack-overflow abort is **not** a panic, so a caller who wrapped their
comparison in `catch_unwind` on the strength of that doc would still lose the whole process.

`eq` is the concrete instance found; more broadly, any hand-written recursive traversal
reachable from a `Variant::new` value is exposed to the same unbounded-recursion shape. (Note
`Debug` is `#[derive]`d over the byte buffers and does not recurse into decoded children, so it
is not affected.)

Found while adding a `proptest` fuzzing harness to `parquet-variant` in #10352; this is a
follow-up to that work and, as noted above, is deliberately *not* addressed by that PR's depth
limit. Investigation was assisted by an agent-driven proptest harness.

Contributor guide

Open the contributing guide

Research direction

Start in parquet-variant's list.rs at VariantList::eq (around line 313) and inspect VariantObject::eq, then reproduce the issue with deeply nested values built through Variant::new. Check that comparisons of unvalidated nested variants no longer overflow the stack or abort the process, while ordinary equality behavior remains correct.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
data-engineering
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.