Skip to content

fix: bound decode allocation by decoded elements, not remaining bytes#417

Merged
matthias-wright merged 3 commits into
audit-may-2026from
m/consensus-state-alloc
Jun 30, 2026
Merged

fix: bound decode allocation by decoded elements, not remaining bytes#417
matthias-wright merged 3 commits into
audit-may-2026from
m/consensus-state-alloc

Conversation

@matthias-wright

Copy link
Copy Markdown
Collaborator

Addresses #314.

Changes:

  • types/consensus_state.rs — drop the capacity hint for deposit_queue, protocol_param_changes, added_validators inner, removed_validators, pending_execution_requests
  • types/withdrawal.rs — drop it for the schedule pubkeys queue
  • types/dynamic_epocher.rs — drop it for segments (kept the existing segments_len == 0 reject)
  • Collections now grow on push; the loops already bail on the first EndOfBuffer, so only genuinely decoded elements are ever allocated
  • Added 3 regression tests asserting a huge nested count fails cheaply without pre-allocating (consensus_state, withdrawal schedule, dynamic_epocher)

@sebastian-osec

Copy link
Copy Markdown

This looks good to me.

One small remaining allocation-hardening case in the same decoder: captured_bytes still allocates from a decoded length before checking that the buffer actually contains that many bytes:

let len = buf.try_get_u32().map_err(|_| Error::EndOfBuffer)? as usize;
let mut bytes = vec![0u8; len];
buf.try_copy_to_slice(&mut bytes)
    .map_err(|_| Error::EndOfBuffer)?;

A malformed checkpoint/state blob could set has_captured = true and a huge len, causing allocation before the later EndOfBuffer. Could we apply the same guard used for pending_execution_requests here?

if len > buf.remaining() {
    return Err(Error::EndOfBuffer);
}

@matthias-wright

Copy link
Copy Markdown
Collaborator Author

Update(210a427):

  • Bound captured_bytes decode allocation by remaining bytes

@sebastian-osec

Copy link
Copy Markdown

Looks good to me.

I re-checked the broader #314 pattern, and the risky with_capacity(len.min(buf.remaining())) sites appear to be removed from the affected decoders. The remaining length-prefixed byte allocations are guarded before allocation.

Optional test nit: a few of the regression tests stop immediately after the huge count, so buf.remaining() == 0; the old code would also have allocated zero in those exact cases. Adding some trailing bytes would make the tests reproduce the original over-allocation shape more directly.

@matthias-wright matthias-wright force-pushed the m/consensus-state-alloc branch from 210a427 to d4873d9 Compare June 30, 2026 13:03
@matthias-wright matthias-wright merged commit 19e25a9 into audit-may-2026 Jun 30, 2026
4 checks passed
@matthias-wright matthias-wright deleted the m/consensus-state-alloc branch June 30, 2026 13:22
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.

2 participants