Skip to content

fix(kerykeion): bound what a peer can accumulate during the handshake - #437

Merged
forkwright merged 3 commits into
mainfrom
fix/229-bound-peer-accumulation
Aug 21, 2026
Merged

fix(kerykeion): bound what a peer can accumulate during the handshake#437
forkwright merged 3 commits into
mainfrom
fix/229-bound-peer-accumulation

Conversation

@forkwright

Copy link
Copy Markdown
Owner

Summary

Another of #229's clauses: the handshake accumulated peer-supplied NodeInfo and Channel messages
into local vectors with no ceiling.

Why the existing timeout is not a bound

handshake_with_config already aborts after HandshakeConfig::timeout_secs, which is easy to mistake
for a limit. It bounds how long a device may talk, not how much it may say. Inside that window
a peer can stream messages as fast as the link allows, and both vectors grow for every one.

Both ceilings already exist

This applies limits the codebase already declares rather than inventing new ones:

  • MAX_LIVE_NODES was added under Bound node-table and topology cardinality to stop OTA memory-exhaustion DoS #204 as the shared ceiling for unauthenticated node identities,
    with a comment stating the reason outright — "from on an inbound frame is unauthenticated, so
    nothing stops a hostile peer from announcing distinct node identities without bound". NodeDb and
    MeshTopology honour it. This local accumulation did not, so the handshake was a way around a bound
    the rest of the crate respects.
  • MAX_CHANNELS is the protocol's own maximum. Anything past it is a peer repeating itself.

The node list warns once as it fills, because a real mesh reaching four thousand identities is worth
seeing rather than silently truncating.

Test

FloodMock sends twenty times the channel maximum before completing the handshake; the collected list
must stop at MAX_CHANNELS.

an_ordinary_channel_count_is_collected_in_full is its anti-vacuity partner — three channels sent,
three collected. Without it the cap would pass against a loop that collects nothing at all.

Scope

The outbound pending queue, #229's remaining unbounded-growth clause, is deliberately not here.
OutboundQueue::enqueue returns () and inserts unconditionally, so bounding it means deciding what
happens to the message that does not fit — reject it, or displace the lowest-priority one — and that
changes the caller-visible contract. It deserves its own change rather than riding along with a fix
that needed no such decision.

Refs #229

forkwright added 3 commits August 21, 2026 01:06
The handshake loop pushed every NodeInfo and every Channel a peer sent into
local vectors with no ceiling. Its timeout bounds how long a device may talk,
not how much it may say, so a peer could fill memory inside that window.

Neither bound is new. MAX_LIVE_NODES already exists as the shared ceiling for
unauthenticated node identities, added for this exact threat under #204, and
NodeDb honours it -- this local accumulation simply did not. MAX_CHANNELS is the
protocol's own maximum, and anything past it is a peer repeating itself.

So this applies ceilings the codebase already declares rather than inventing
new ones. The node list warns once when it fills, since a real mesh reaching
four thousand identities is worth seeing.

The test floods twenty times the channel maximum and requires the list to stop
at it, with an ordinary three-channel handshake alongside so the cap cannot pass
against a loop that collects nothing.

Refs #229
Channel has a single field, so ..Default::default() specified nothing.
The handshake requires it before anything else, so both new tests failed with
'radio did not send MyNodeInfo' before reaching the accumulation they exercise.
@forkwright
forkwright merged commit 04c22d7 into main Aug 21, 2026
11 checks passed
@forkwright
forkwright deleted the fix/229-bound-peer-accumulation branch August 21, 2026 06:26
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