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.
PartialEqon unvalidated deeply-nested variants overflows the stack (SIGABRT)Describe the bug
VariantList::eq(list.rs:313) andVariantObject::eqare hand-written and recurse intochild values with no depth bound. Comparing two deeply-nested variants that were constructed
with the shallow
Variant::newconstructor overflows the stack and aborts the process(
SIGABRT). Unlike a panic, a stack-overflow abort cannot be contained bycatch_unwind.This is not closed by the validation depth limit added in #10352: that limit
(
MAX_NESTING_DEPTH) is enforced insidewith_full_validation, andVariant::newnever callsit. So
eqon aVariant::newvalue has no gate.To Reproduce
Output:
(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::newperforms only constant-time shallow validation and never calls it, so nothing gates the
recursion in the hand-written
eqimplementations for lists and objects.Expected behavior
Comparing two variants should never abort the process, even for unvalidated inputs. Recursion
should be bounded inside
eqitself — e.g. a depth guard, or an iterative comparison — so that==on aVariant::newvalue fails gracefully (or simply returns) rather than overflowing thestack.
Additional context
Reachability is narrower than a typical parsing bug: it requires opting out of validation via
Variant::newand then comparing. Validated input (viatry_new) is capped atMAX_NESTING_DEPTHby #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_unwindon the strength of that doc would still lose the whole process.eqis the concrete instance found; more broadly, any hand-written recursive traversalreachable from a
Variant::newvalue is exposed to the same unbounded-recursion shape. (NoteDebugis#[derive]d over the byte buffers and does not recurse into decoded children, so itis not affected.)
Found while adding a
proptestfuzzing harness toparquet-variantin #10352; this is afollow-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.