Roll back composite sub-handlers when one rejects peer_connected#4595
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
|
No issues found. I reviewed the entire diff thoroughly:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4595 +/- ##
==========================================
- Coverage 86.84% 86.11% -0.73%
==========================================
Files 161 157 -4
Lines 109260 108772 -488
Branches 109260 108772 -488
==========================================
- Hits 94882 93668 -1214
- Misses 11797 12487 +690
- Partials 2581 2617 +36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jkczyz
left a comment
There was a problem hiding this comment.
LGTM aside from needing to add the debug_assert.
`composite_custom_message_handler!` expanded `peer_connected` to call every sub-handler and remember the last error, but never undo the already-succeeded ones. The `CustomMessageHandler::peer_connected` contract is that `PeerManager` will *not* invoke `peer_disconnected` when `peer_connected` returns `Err` — so any per-peer state allocated by an earlier sub-handler that returned `Ok` was leaked permanently once a later sub-handler returned `Err`. A peer who can elicit `Err` from any sub-handler in the composite (feature-bit gate, banlist, etc.) could repeatedly reconnect to grow that leaked state without bound (slow resource DoS), and "currently connected" predicates in the leaking sub-handler would lie about peers that were actually rejected. Mirror the rollback pattern `PeerManager` already uses for the four built-in handlers (`peer_handler.rs:2149-2188`): record each sub-handler's `peer_connected` result, and if any returned `Err`, call `peer_disconnected` on the ones that succeeded before propagating the failure. Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de>
7de6891 to
5455058
Compare
|
Backported to 0.1 in #4680. |
|
Backported to 0.2 in #4683. |
composite_custom_message_handler!expandedpeer_connectedto call every sub-handler and remember the last error, but never undo the already-succeeded ones. TheCustomMessageHandler::peer_connectedcontract is thatPeerManagerwill not invokepeer_disconnectedwhenpeer_connectedreturnsErr— so any per-peer state allocated by an earlier sub-handler that returnedOkwas leaked permanently once a later sub-handler returnedErr.A peer who can elicit
Errfrom any sub-handler in the composite (feature-bit gate, banlist, etc.) could repeatedly reconnect to grow that leaked state without bound (slow resource DoS), and "currently connected" predicates in the leaking sub-handler would lie about peers that were actually rejected.Mirror the rollback pattern
PeerManageralready uses for the four built-in handlers (peer_handler.rs:2149-2188): record each sub-handler'speer_connectedresult, and if any returnedErr, callpeer_disconnectedon the ones that succeeded before propagating the failure.Co-Authored-By: HAL 9000