#35936 net: reject oversized locators before allocating

ReviewabilityReviewsAgreementSize
#35936 l0rinc net: reject oversized locators before allocatingReady0 (+2)Strong36 + 26 tests
  • Disconnect peers that send oversized block locator requests, even when payloads are truncated
  • Ensure malformed locator messages trigger immediate disconnection instead of being silently dropped
  • Ready to review
  • Substantive support with no open objections
  • Tested and confirmed that truncated oversized locators now trigger disconnects (jeanpablojp)
  • Resolved finding where counts above maximum size bypassed disconnect checks (jeanpablojp)
  • Nonblocking question on using exceptions rather than manual loops answered by author (sedited)
  • Approval without detailed stated reasons (achow101, ajtowns)
  • 36 lines added or modified outside tests
  • 26 lines added or modified in tests
  • 20 lines removed in total
  • 4 files, 4 commits
P2P#34 of 71
P3 · DoS protection
  • P3 because it closes an edge case where peers sending oversized locators evade disconnection
  • Truncated messages with excessive counts are now disconnected rather than silently dropped
  • Ranking pass: Disconnect-evasion edge case grouped with the other message-handling bug fixes rather than with refactors.
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Enforces the block locator size limit in incoming getblocks and getheaders messages before allocating and deserializing locator hashes. Introduces a dedicated LimitedVectorExceededError exception to the limited vector deserializer and adds CBlockLocator::LimitedRead to catch oversized locator counts. In net processing, oversized locators now trigger peer disconnection immediately, even if the payload is truncated.

Problem: Peers sending oversized block locators in getblocks and getheaders are supposed to be disconnected, but the MAX_LOCATOR_SZ check occurred only after full deserialization. A truncated message specifying an oversized count failed in generic vector deserialization before reaching the limit check, causing the message handler to drop the message but leave the peer connected.

Evidence of importance:

  • PR description: "A truncated locator advertising an oversized count makes generic vector deserialization fail before the disconnect check, leaving the peer connected."
  • Commit e327916668: "Use CBlockLocator::LimitedRead to detect the oversized count before allocating hashes. The message handler catches only LimitedVectorExceededError and disconnects the peer. Other deserialization failures keep following the generic non-disconnecting exception path."

Reviewability

Ready

Nothing known that would invalidate a review now.

Agreement

Strong

substantive support, no open objection (jeanpablojp, jeanpablojp)

Thread notes: Initial record population. Jeanpablojp identified an edge case where counts > MAX_SIZE bypassed disconnection, which l0rinc fixed. sedited and mzumsande questioned the exception design vs manual loops in net_processing, which ajtowns and l0rinc answered.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
jeanpablojp (contributor)objection, correctnessa locator advertising more than MAX_SIZE (33,554,432) hashes bypasses the disconnect check because ReadCompactSize throws generic std::ios_base::failure instead of the limited vector errorfixed · fix pushed · v1, not yet re-checkedansweredyes2026-08-09: 'The same message still leaves the peer connected once the count goes above MAX_SIZE.'
Status from: 2026-09-04: l0rinc: 'addressed @jeanpablojp's finding: counts above MAX_SIZE now also disconnect, with regression coverage'
sedited (member)objection, approachusing an exception for what could be normal control flow is clunky compared to net_processing parsinganswered · v1, not yet re-checkedansweredyes2026-09-01: 'I think I'd have a slight preference towards using the same approach. Adding another exception, to what should just be normal control flow reads a bit clunky.'
Status from: 2026-09-04: l0rinc: 'checking the count locally means we can no longer use ordinary vector deserialization since we've consumed the size already and have to reproduce the element-reading loop in net_processing.'

Support:

  • jeanpablojp (contributor) (Approach ACK, 2026-08-09): The dedicated exception subtype is the right shape, and the truncated case it targets is fixed.
  • ajtowns (member) (2026-08-09): Untested, but looks fine to me. [not substantive]
  • achow101 (member) (ACK, 2026-08-17): ACK 725bf357d9ef652eacd809a1bf2f6f79ec73a0b4 [not substantive]
  • jeanpablojp (contributor) (Tested ACK, 2026-08-19): tACK 725bf357d9ef652eacd809a1bf2f6f79ec73a0b4

Participants:

  • ajtowns (member, support, 3 statements 2026-08-09 to 2026-09-01): reviewed, suggested minor nit on ignore(), defended exception approach against open-coding loops
  • jeanpablojp (contributor, support, 2 statements 2026-08-09 to 2026-08-19): Approach ACK and tested ACK; found edge case with counts above MAX_SIZE which author resolved
  • achow101 (member, support, 1 statements 2026-08-17 to 2026-08-17): ACKed commit 725bf357d9
  • mzumsande (member, question, 1 statements 2026-09-01 to 2026-09-01): asked why this touches serialization code instead of doing it locally like HEADERS
  • sedited (member, neutral, 1 statements 2026-09-01 to 2026-09-01): expressed slight preference for HEADERS approach without exceptions, but did not block

Files

26 lines under test/bench/ci.

  • src/net_processing.cpp +16/-14
  • src/test/net_tests.cpp +26/-0
  • src/serialize.h +12/-6
  • src/primitives/block.h +8/-0

Card

The short text the ranking pass compares across PRs.

Validates block locator counts against MAX_LOCATOR_SZ in getblocks and getheaders messages before deserializing the hashes. Previously, the size check ran only after deserialization, allowing peers that sent truncated messages with oversized counts to bypass disconnection because generic deserialization failures do not disconnect. The change introduces a distinct exception for limited vector overflows and disconnects the peer upon receiving an oversized count regardless of whether the message was truncated.

Data

dossier JSON · extract JSON · model ledger, generated 2026-09-18T03:40, confidence high, input hash f5fcec04811573a4
thread read through 2026-09-04 (11 statements), head e327916668
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: P2P

Categories

P2P

P3 · DoS protection (ranking pass: Disconnect-evasion edge case grouped with the other message-handling bug fixes rather than with refactors.)

Prevents peers sending truncated oversized locators from evading disconnection. Prior to this change, truncated messages specifying an oversized locator count failed during generic vector deserialization and were dropped without disconnecting the peer; this enforces immediate disconnect.

Membership: Has the P2P label, modifies src/net_processing.cpp, and changes peer disconnection behavior for malformed getblocks and getheaders messages.

Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 0