Fix panic when peer is mid-handshake #2842
Conversation
WalkthroughThe codebase has been updated with a new functionality in the fuzz testing suite and minor refactoring in the peer handler of the lightning module. The fuzz testing now includes a scenario for broadcasting node announcements, enhancing the robustness of the network communication features. Meanwhile, the lightning module has seen a reorganization of code for better readability, along with the addition of log initialization within certain conditions, although the core logic remains untouched. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- fuzz/src/full_stack.rs (3 hunks)
- lightning/src/ln/peer_handler.rs (3 hunks)
Files skipped from review due to trivial changes (1)
- lightning/src/ln/peer_handler.rs
Additional comments: 1
fuzz/src/full_stack.rs (1)
- 717-719: The addition of the match arm at line 17 to call
broadcast_node_announcementduring the handshake process is consistent with the PR's objective to replicate the panic scenario for the fuzz test.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2842 +/- ##
==========================================
- Coverage 88.54% 88.53% -0.01%
==========================================
Files 115 115
Lines 92277 92277
Branches 92277 92277
==========================================
- Hits 81706 81702 -4
- Misses 8068 8073 +5
+ Partials 2503 2502 -1 ☔ View full report in Codecov by Sentry. |
|
Actually, can you drop the fuzz change commit? I have a larger patch locally that I want to upstream that adds broader coverage. |
Peer::their_node_id is set to Some during the handshake process. However, df3ab2e accesses the field unconditionally, causing a panic. This may be triggered if a gossip message is received mid-handshake from another peer or if the user calls broadcast_node_announcement during this time. The latter tends to be executed on a timer. Ensure that Peer::their_node_id is only accessed once the handshake is complete.
jkczyz
force-pushed
the
2024-01-fix-peer-handler-unwrap
branch
from
3dee0cf to
c7465bd
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- lightning/src/ln/peer_handler.rs (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- lightning/src/ln/peer_handler.rs
lightning/src/ln/peer_handler.rs
Outdated
| if !peer.handshake_complete() || | ||
| !peer.should_forward_channel_announcement(msg.contents.short_channel_id) { | ||
| continue | ||
| } | ||
| debug_assert!(peer.their_node_id.is_some()); | ||
| debug_assert!(peer.channel_encryptor.is_ready_for_encryption()); | ||
| let logger = WithContext::from(&self.logger, Some(peer.their_node_id.unwrap().0), None); |
There was a problem hiding this comment.
Can we also remove the unwrap while we're here?
| let logger = WithContext::from(&self.logger, Some(peer.their_node_id.unwrap().0), None); | |
| let logger = WithContext::from(&self.logger, peer.their_node_id.map(|p| p.0), None); |
There was a problem hiding this comment.
Done here and few other places
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- lightning/src/ln/peer_handler.rs (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- lightning/src/ln/peer_handler.rs
Peer::their_node_idis set toSomeduring the handshake process. However, df3ab2e accesses the field unconditionally, causing a panic. This may be triggered if a gossip message is received mid-handshake from another peer or if the user callsbroadcast_node_announcementduring this time. The latter tends to be executed on a timer.Update the fuzz test to call
broadcast_node_announcementmid-handshake on an inbound channel in order to trigger a panic. Ensure thatPeer::their_node_idis only accessed once the handshake is complete.