cetra3 opened a new issue, #10360:
URL: https://github.com/apache/arrow-rs/issues/10360
# `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<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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
