Skip to content

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

Description

@cetra3

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

use parquet_variant::Variant;

/// `depth` single-element lists nested around a null primitive.
fn make_nested_lists(depth: usize) -> Vec<u8> {
    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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions