highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-21 15:50:07 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#30394: net: fix race condition in self-connect detection

Browse Source 
16bd283b3a Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner)
0dbcd4c148 net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner)
66673f1c13 net: fix race condition in self-connect detection (Sebastian Falbesoner)

Pull request description:

  This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368).

  Initiating an outbound network connection currently involves the following steps after the socket connection is established (see [`CConnman::OpenNetworkConnection`](bd5d1688b4/src/net.cpp (L2923-L2930)) method):
  1. set up node state
  2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](bd5d1688b4/src/net_processing.cpp (L1662-L1683)))
  3. add new node to vector `m_nodes`

  If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`).

  Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`.

  The temporarily reverted test introduced in #30362 is readded. Fixes #30368.

  Thanks go to vasild, mzumsande and dergoegge for suggestions on how to fix this (see https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200625017 ff. and https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789).

ACKs for top commit:
  naiyoma:
    tested ACK [16bd283b3a),  built and tested locally,  test passes successfully.
  mzumsande:
    ACK 16bd283b3a
  tdb3:
    ACK 16bd283b3a
  glozow:
    ACK 16bd283b3a
  dergoegge:
    ACK 16bd283b3a

Tree-SHA512: 5b8aced6cda8deb38d4cd3fe4980b8af505d37ffa0925afaa734c5d81efe9d490dc48a42e1d0d45dd2961c0e1172a3d5b6582ae9a2d642f2592a17fbdc184445
This commit is contained in:
glozow
2024-07-16 09:34:15 +01:00
parent d41f4a69e7 16bd283b3a
commit 35dddbccf1
4 changed files with 25 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
src/net_processing.cpp
View File

@@ -243,6 +243,9 @@ struct Peer {
* Most peers use headers-first syncing, which doesn't use this mechanism */
uint256 m_continuation_block GUARDED_BY(m_block_inv_mutex) {};
/** Set to true once initial VERSION message was sent (only relevant for outbound peers). */
bool m_outbound_version_message_sent GUARDED_BY(NetEventsInterface::g_msgproc_mutex){false};
/** This peer's reported block height when we connected */
std::atomic<int> m_starting_height{-1};
@@ -498,7 +501,7 @@ public:
EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex);
/** Implement NetEventsInterface */
void InitializeNode(CNode& node, ServiceFlags our_services) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
void InitializeNode(const CNode& node, ServiceFlags our_services) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
void FinalizeNode(const CNode& node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_headers_presync_mutex);
bool HasAllDesirableServiceFlags(ServiceFlags services) const override;
bool ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt) override
@@ -1659,7 +1662,7 @@ void PeerManagerImpl::UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_s
if (state) state->m_last_block_announcement = time_in_seconds;
}
void PeerManagerImpl::InitializeNode(CNode& node, ServiceFlags our_services)
void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_services)
{
NodeId nodeid = node.GetId();
{
@@ -1677,9 +1680,6 @@ void PeerManagerImpl::InitializeNode(CNode& node, ServiceFlags our_services)
LOCK(m_peer_mutex);
m_peer_map.emplace_hint(m_peer_map.end(), nodeid, peer);
}
if (!node.IsInboundConn()) {
PushNodeVersion(node, *peer);
}
}
void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler)
@@ -5326,6 +5326,10 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
PeerRef peer = GetPeerRef(pfrom->GetId());
if (peer == nullptr) return false;
// For outbound connections, ensure that the initial VERSION message
// has been sent first before processing any incoming messages
if (!pfrom->IsInboundConn() && !peer->m_outbound_version_message_sent) return false;
{
LOCK(peer->m_getdata_requests_mutex);
if (!peer->m_getdata_requests.empty()) {
@@ -5817,6 +5821,12 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
// disconnect misbehaving peers even before the version handshake is complete.
if (MaybeDiscourageAndDisconnect(*pto, *peer)) return true;
// Initiate version handshake for outbound connections
if (!pto->IsInboundConn() && !peer->m_outbound_version_message_sent) {
PushNodeVersion(*pto, *peer);
peer->m_outbound_version_message_sent = true;
}
// Don't send anything until the version handshake is complete
if (!pto->fSuccessfullyConnected || pto->fDisconnect)
return true;