Skip to content

persist: harden state diffs, part stats and spine fuel against malformed input - #37971

Open
def- wants to merge 3 commits into
mainfrom
def/fuzz-01-persist
Open

persist: harden state diffs, part stats and spine fuel against malformed input#37971
def- wants to merge 3 commits into
mainfrom
def/fuzz-01-persist

Conversation

@def-

@def- def- commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Three unchecked-arithmetic and unchecked-decode fixes in mz-persist-client.
Each one is on a path that reads durable state, where the bytes are whatever
consensus or blob happened to hold, so none of the inputs are trusted.

  • ProtoStateFieldDiffs::validate summed data_lens with unchecked
    arithmetic.
    Overflow checks are off in the release and optimized profiles,
    so data_lens = [u64::MAX, 1] with empty data_bytes wrapped the sum to 0,
    matched the real byte length, passed validation, and left the iterator
    slicing 0..u64::MAX out of an empty slice. Sums with checked_add now and
    rejects on overflow, so a malformed diff becomes the same decode error as
    every other inconsistency validate catches.

  • LazyPartStats's Debug panicked on a malformed encoding. The proto is
    held undecoded and stored without validation, and Debug called a
    decode() that expects it to be well formed. That is reachable on the
    state-load rejection path: Spine::validate formats the spine into the
    message it rejects with, so a rollup whose trace fails validation and
    carries malformed part stats panicked while building the error meant to
    reject it. The shard then failed to load by panic rather than by error, on
    every attempt. Splits out try_decode and renders an <undecodable: ...>
    placeholder instead.

  • The spine's fuel computation could wrap. (8 << batch_index) * effort
    was unchecked in both steps, with batch_index derived from the inserted
    batch's len. Saturates now.

One NOTE: is left in the code deliberately: the read-path callers of
LazyPartStats::decode() (fetch.rs, filter pushdown in
operators/shard_source.rs) run on the same untrusted bytes and still panic on
a malformed encoding. Each needs its own decision about what a missing-stats
fallback should do, so they are out of scope here.

Tests

Adds proto_state_diff_data_lens_overflow_is_error and
lazy_part_stats_debug_does_not_panic_on_garbage, both of which panic without
the corresponding fix, plus coverage for the saturating fuel computation. The
LazyPartStats panic was found by the rollup_proto_roundtrip cargo-fuzz
target in release-qualification 1330 (v26.35.0-rc.1). That target is sharpened
in #37979, which should land after this.

@def- def- changed the title def/fuzz 01 persist persist: harden state diffs, part stats and spine fuel against malformed input Jul 31, 2026
@def-
def- marked this pull request as ready for review July 31, 2026 09:58
@def-
def- requested a review from a team as a code owner July 31, 2026 09:58
def- and others added 3 commits July 31, 2026 10:06
`ProtoStateFieldDiffs::validate` summed the `data_lens` entries with
unchecked arithmetic. Overflow checks are off in the release and
optimized profiles, so a diff declaring `data_lens = [u64::MAX, 1]` with
an empty `data_bytes` wrapped the sum to 0, matched the real byte length,
and passed validation. `ProtoStateFieldDiffsIter::next` then sliced
`0..u64::MAX` out of an empty slice. State diffs are read from consensus
on every state update, so this input is untrusted and reachable from a
corrupted or crafted blob. Under overflow checks (the `ci` profile,
`cargo test`, cargo-fuzz builds) the panic is the addition itself.

Sum with `checked_add` instead and reject on overflow, so a malformed
diff becomes the same decode error as every other inconsistency
`validate` catches, rather than a panic in the iterator.

Adds `proto_state_diff_data_lens_overflow_is_error`, which panics inside
`validate` without this change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`LazyPartStats` holds its `ProtoStructStats` undecoded, and its
`RustType<Bytes>` impl stores the bytes without validating them, so a
value read off blob carries whatever encoding was there. `Debug` called
`decode()`, which `expect`s that encoding to be well-formed.

That is reachable on the persist state-load path, and specifically on its
rejection path: `Spine::validate` formats the whole spine into the
message it rejects with, and `Trace::unflatten` turns that into the
decode error a `Rollup::from_proto` caller sees. A rollup whose trace
fails validation *and* carries malformed part stats therefore panicked
while building the error meant to reject it, so the shard failed to load
by panic rather than by error, on every attempt.

Split the fallible half out as `try_decode` and have `Debug` render an
`<undecodable: ...>` placeholder instead of panicking. `decode()` keeps
its panicking contract for callers holding stats they encoded
themselves, now documented as such.

NOTE: the read-path callers of `decode()` (`fetch.rs`, filter pushdown in
`operators/shard_source.rs`) run on the same untrusted bytes and still
panic on a malformed encoding. Left alone here because each needs a
decision about what a missing-stats fallback should do.

Adds `lazy_part_stats_debug_does_not_panic_on_garbage`, which panics in
`Debug` without this change. Found by the `rollup_proto_roundtrip`
cargo-fuzz target in release-qualification 1330 (v26.35.0-rc.1).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Spine::introduce_batch` sized each maintenance step as
`(8 << batch_index) * effort`, where `batch_index` comes from the
inserted batch's `len`. Both steps were unchecked, and the result was
`as`-cast to `isize`. A `len` near `2^60` makes the shift land on the
`isize` sign bit, so the budget handed to `apply_fuel` was negative.

`FuelingMerge::work` then read that back with `*fuel as usize`, which
wraps a negative budget to a huge one, spent `remaining_work` against it,
and subtracted that from the still-negative `fuel` -- an underflow. This
is reachable from a corrupted or crafted blob: `Trace::unflatten` replays
decoded legacy batches through the real spine, and its `MAX_TOTAL_LEN`
guard only caps the total at `usize::MAX >> 3`, which still admits such a
`len`. Under overflow checks (the `ci` profile, `cargo test`, cargo-fuzz
builds) the panic is the subtraction. In the release and optimized
profiles it wraps silently and leaves the merge's fuel accounting
meaningless instead.

Compute the fuel with checked arithmetic and saturate at `isize::MAX`.
Fuel is a budget, so an over-large one only completes merges sooner,
whereas a wrapped one can go negative and starve them. In `work`, read a
negative budget as zero available rather than as a huge one, which also
retires the two `as` casts the `TODO` there asked about.

Adds `spine_fuel_isize_overflow`, which panics in `work` without this
change. Found by the `rollup_proto_roundtrip` cargo-fuzz target in
release-qualification 1332 (v26.35.0-rc.2).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant