Skip to content

fix(chain): cap initial Vec allocation in zcash_deserialize_external_count#10591

Closed
zmanian wants to merge 2 commits into
ZcashFoundation:mainfrom
zmanian:fix/varint-preallocate-cap
Closed

fix(chain): cap initial Vec allocation in zcash_deserialize_external_count#10591
zmanian wants to merge 2 commits into
ZcashFoundation:mainfrom
zmanian:fix/varint-preallocate-cap

Conversation

@zmanian

@zmanian zmanian commented May 14, 2026

Copy link
Copy Markdown
Contributor

Motivation

Closes #10545.

zcash_deserialize_external_count in zebra-chain/src/serialization/zcash_deserialize.rs does a single upfront allocation:

let mut vec = Vec::with_capacity(external_count);
for _ in 0..external_count {
    vec.push(T::zcash_deserialize(&mut reader)?);
}

external_count comes from a peer-supplied CompactSize. A crafted message with an inflated count forces the full allocation before any element bytes are read. For block::Hash (32 bytes × 65,535 = ~2 MiB), this is reachable via read_getblocks and read_getheaders from any peer that has completed the version/verack handshake.

This is the same defect class as the AddrV1/V2 fix shipped in #10494 (GHSA-xr93-pcq3-pxf8), but those fixes capped the per-type max_allocation() rather than addressing the root cause in the deserializer. The recent read_headers 160-entry cap (#10528) is another point fix in the same family.

Solution

Cap the initial Vec::with_capacity at 1024 and let the Vec grow via push() as deserialization succeeds — matching the chunked-resize pattern zcashd uses in serialize.h:761–777. Memory now tracks bytes actually received from the peer. TrustedPreallocate::max_allocation() is unchanged and remains the upper bound (checked at the top of the function) as defense in depth.

let mut vec = Vec::with_capacity(external_count.min(MAX_INITIAL_ALLOCATION));

MAX_INITIAL_ALLOCATION = 1024 is large enough that honest messages amortize their growth to a handful of reallocations — negligible relative to per-element deserialization cost.

Test evidence

Added two regression tests in zebra-chain/src/serialization/tests/preallocate.rs, using block::Hash (the type called out in the issue):

  • external_count_above_initial_cap_still_deserializes — a count of 4096 with a full body of 32-byte hashes decodes correctly, confirming the cap does not break legitimate large vectors.
  • external_count_with_truncated_body_errors — an inflated count near Hash::max_allocation() with a 64-byte body fails with SerializationError::Io(_), confirming truncated bodies surface as read errors rather than successful giant pre-allocations.
cargo test -p zebra-chain --lib                       # 249 passed; 0 failed
cargo test -p zebra-network --lib                     # 180 passed; 0 failed
cargo clippy -p zebra-chain --all-targets -- -D warnings   # clean
cargo fmt --all -- --check                                 # clean

AI disclosure

Used Claude Code (Opus 4.7) for the patch, regression tests, and PR description. The issue description (#10545) and the zcashd-pattern fix shape were provided by @dingledropper.

…count

`zcash_deserialize_external_count` did a single
`Vec::with_capacity(external_count)` allocation based on the peer-supplied
CompactSize before reading any element bytes. A crafted message with an
inflated count forced the full allocation up-front regardless of how much
body followed. The previous mitigation pattern (capping `max_allocation()`
on individual `TrustedPreallocate` impls) addressed each type case-by-case
rather than the root cause; the recent `read_headers` cap (ZcashFoundation#10528) and the
AddrV1/V2 fixes from ZcashFoundation#10494 / GHSA-xr93-pcq3-pxf8 are examples.

Cap the initial allocation at 1024 elements and let the `Vec` grow via
`push()` as deserialization succeeds, matching the chunked-resize pattern
zcashd uses in `serialize.h`. `TrustedPreallocate::max_allocation()` is
unchanged and continues to bound total size as defense in depth.

Add regression tests in `zebra-chain/src/serialization/tests/preallocate.rs`:

- `external_count_above_initial_cap_still_deserializes` confirms a count
  above the initial cap with a full body still decodes correctly.
- `external_count_with_truncated_body_errors` confirms an inflated count
  with a short body fails with an I/O error rather than producing a
  large pre-allocated vector.

Closes ZcashFoundation#10545

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

…re-fix

Address codex review feedback on ZcashFoundation#10591: the previous two regression
tests both passed on `origin/main` because the bug they targeted is an
allocation-amplification / DoS behavior that is not directly observable
from the function's return value.

Replace `external_count_with_truncated_body_errors` with
`external_count_above_isize_max_returns_io_error_instead_of_panicking`.
This test defines a `TrustedPreallocate` test type with
`max_allocation = u64::MAX` and calls `zcash_deserialize_external_count`
with `external_count = isize::MAX as usize + 1`. On `origin/main` this
panics inside `Vec::with_capacity` with "capacity overflow" before the
function reaches the reader. With the cap in place, the initial
allocation is bounded at `MAX_INITIAL_ALLOCATION = 1024`, the function
reaches the reader, and the short body surfaces as a clean
`SerializationError::Io(_)`. Verified by reverting the source change
locally: the new test fails on pre-fix code and passes with the fix.

The companion `external_count_above_initial_cap_still_deserializes`
test is retained to confirm the cap does not break correctness for
ordinary valid messages.
@oxarbitrage

Copy link
Copy Markdown
Contributor

Closing in favor of #10563 which addresses the same fix (and also closes the duplicate #10548). We prefer the less verbose approach — the core fix is a one-liner and the detailed regression tests don't feel necessary for a simple allocation cap. Thanks for the contribution @zmanian, and for crediting dingledropper's original report.

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.

bug: Vec deserializer preallocates full capacity from untrusted varint before reading data

2 participants