#35888 net: reject oversized outbound messages
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35888 net: reject oversized outbound messages | Ready | 0 | Positive | 18 + 61 tests | |||
|
|
|
| ||||
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
Validates outbound message types and payload sizes in CConnman::PushMessage() before queuing them for transport, dropping and logging violations in release builds and triggering an Assume failure in debug builds. Updates the sendmsgtopeer RPC to reject payloads exceeding the 4 MB MAX_PROTOCOL_MESSAGE_LENGTH with an RPC error instead of sending them. Constrains fuzz inputs and adds unit tests verifying that messages with 12-byte types and 4 MB payloads are accepted while oversized ones are rejected.
Problem: CConnman did not enforce the 12-byte type limit or the 4 MB payload limit before queueing outbound messages. An oversized type causes V1 transport encoding to abort, and causes V2 transport to either send a truncated type or write out of bounds into its encoding buffer when the payload is shorter than the excess type bytes. Oversized payloads passed to sendmsgtopeer were sent over the wire and caused the remote peer to disconnect.
Evidence of importance:
- PR description and commit caea75f11b: 'V1 aborts on a longer type, while V2 sends a truncated type with the payload intact or writes past its encoding buffer when the payload is shorter than the excess type bytes.'
- Commit 0dfdd0c4a0: 'Record that PushMessage() queues oversized types and payloads, and that sendmsgtopeer sends an oversized payload which disconnects its peer.'
Risk of a mistake: medium (p2p, rpc_api). Incorrectly dropping outbound messages or failing assumptions in PushMessage could disrupt peer communications or crash debug builds.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: re-review following rebase, simplified tests/fuzz setup, and author's response 2026-09-29
Agreement
Positive
support without stated reasons, no open objection (ajtowns, naiyoma)
Thread notes: Author replied to naiyoma's suggestion on restricting message type copying, explaining that PushMessage already rejects oversized types and clamping would silently truncate invalid types.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| ajtowns (member) | question, approach | explain the motivation and thinking in relation to #35880 | answered · author explained context and rationale regarding receive-side assumptions | answered | yes | 2026-08-05: 'Can you explain your thinking / where you were coming from in relation to https://github.com/bitcoin/bitcoin/pull/35880#pullrequestreview-4858213755 some more?' Status from: 2026-08-06: l0rinc: 'I don't think you're missing anything on the receive side.' |
| ajtowns (member) | suggestion, style | define MAX_MESSAGE_TYPE with a static_assert for readability | fixed · fix pushed · author switched to std::string_view and materialized at call sites | answered | yes | 2026-08-05: 'would be easier for the reader I think.' Status from: 2026-08-06: l0rinc: 'Addressed your suggestions, thanks, pushed.' |
| ajtowns (member) | suggestion, interface | consider moving size checks to CSerializedNetMsg in protocol.h with documentation | answered · author addressed suggestions and updated code | answered | yes | 2026-08-05: 'Should this test be part of the `CSerializedNetMsg` class in protocol.h? Would probably be a useful place to add documentation that code constructing these msgs must ensure they're not oversized.' Status from: 2026-08-06: l0rinc: 'Addressed your suggestions, thanks, pushed.' |
| mzumsande (member) | suggestion, approach | frame the sendmsgtopeer change around preventing regressions rather than fixing a problem in a test RPC | fixed · fix pushed · author updated RPC behavior and PR description | answered | yes | 2026-08-10: 'The story should be that you want to add the assumption to prevent future regressions etc., and need to change the rpc for that' Status from: 2026-08-14: l0rinc: 'Pushed, let me know if I understood the request.' |
| mzumsande (member) | objection, correctness | returning false in production on oversized messages would stall the connection for minutes because SocketSendData makes no progress | fixed · fix pushed · author moved size assumptions to PushMessage before queueing | changed | yes | 2026-08-10: 'returning false would result in a stall of the connection for minutes (until it get disconnected for inactivity), since `SocketSendData` will never make progress' Status from: 2026-08-10: l0rinc: 'I moved both size assumptions to `PushMessage()`, before the message enters the send queue.' |
| mzumsande (member) | suggestion, approach | enforce application-level size limits at PushMessage rather than transport level | fixed · fix pushed · author relocated limits to PushMessage and credited mzumsande | answered | yes | 2026-08-10: 'wouldn't it be nicer from an architectural point of view if arbitrary application-level rules such as the 4MB maximum message length wouldn't be enforced at the transport level, but only at the higher' Status from: 2026-08-10: l0rinc: 'This also keeps the 4 MB application-level rule out of the transports - added you as coauthor, thanks for the review.' |
| naiyoma (member) | suggestion, safety | restrict message type copy to MESSAGE_TYPE_SIZE using std::copy_n | answered · author explained PushMessage already rejects oversized types and clamping would silently truncate invalid types | answered | yes | 2026-09-13: 'nit: Maybe it also makes sense to restrict the message type copy to `MESSAGE_TYPE_SIZE`?' Status from: 2026-09-29: l0rinc: '`PushMessage` already rejects oversized types before they reach v2, and clamping there would silently truncate an invalid type, so it's not required for this fix' |
Support:
- ajtowns (member) (Concept ACK, 2026-08-05): (no reason given) [not substantive]
- naiyoma (member) (Concept ACK, 2026-09-13): (no reason given) [not substantive]
Participants:
- ajtowns (member, support, 3 statements 2026-08-05 to 2026-08-05): Concept ACK; asked for fuzzing context and suggested test and interface improvements
- mzumsande (member, objection, 2 statements 2026-08-10 to 2026-08-10): objected to connection stalls from returning false; suggested moving checks to PushMessage
- naiyoma (member, support, 1 statements 2026-09-13 to 2026-09-13): Concept ACK; suggested using std::copy_n to bound message type copying
Files
72 lines under test/bench/ci.
- src/test/net_tests.cpp +47/-0
- test/functional/rpc_net.py +8/-4
- src/net.h +10/-1
- src/test/fuzz/p2p_private_broadcast.cpp +5/-6
- src/rpc/net.cpp +4/-1
- src/net.cpp +4/-0
- src/test/fuzz/connman.cpp +1/-1
Card
The short text the ranking pass compares across PRs.
Validates outbound message types and payload sizes in CConnman::PushMessage() before queuing, asserting in debug builds and dropping oversized messages in release builds. Outbound transports assume a 12-byte message type and a 4 MB payload limit; exceeding them previously caused V1 encoding to abort, caused V2 encoding to write out of bounds or truncate types, and allowed sendmsgtopeer to send payloads that triggered peer disconnections. The sendmsgtopeer RPC now returns an RPC error when payloads exceed 4 MB. It has no PR dependencies.
Data
thread read through 2026-09-29 (12 statements), head 48ca5ed0cd
labels: P2P
Categories
P2P
P1 · bug fix
Prevents memory corruption and crashes by enforcing type and payload length checks prior to transport encoding. Without this check, 'V1 aborts on a longer type, while V2 sends a truncated type with the payload intact or writes past its encoding buffer when the payload is shorter than the excess type bytes' (PR description and commit caea75f11b).
Membership: Labeled P2P and changes outbound message validation in src/net.cpp and src/net.h.
Factors: security/stability 2, bug 2, performance 0, user value 0, leverage 0