Skip to content

Commit

Permalink
net: fix race condition in self-connect detection
Browse files Browse the repository at this point in the history
Initiating an outbound network connection currently involves the
following steps after the socket connection is established (see
 `CConnman::OpenNetworkConnection` method):
    1. set up node state
    2. queue VERSION message
    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`.

Thanks go to vasild, mzumsande, dergoegge and sipa for suggestions on
how to fix this.

Github-Pull: bitcoin#30394
Rebased-From: 66673f1
  • Loading branch information
theStack authored and fanquake committed Jul 17, 2024
1 parent fa90989 commit 0933cf5
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 5 deletions.
2 changes: 1 addition & 1 deletion src/net.h
Original file line number Diff line number Diff line change
Expand Up @@ -999,7 +999,7 @@ class NetEventsInterface
/** Mutex for anything that is only accessed via the msg processing thread */
static Mutex g_msgproc_mutex;

/** Initialize a peer (setup state, queue any initial messages) */
/** Initialize a peer (setup state) */
virtual void InitializeNode(CNode& node, ServiceFlags our_services) = 0;

/** Handle removal of a peer (clear state) */
Expand Down
16 changes: 13 additions & 3 deletions src/net_processing.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,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};

Expand Down Expand Up @@ -1576,9 +1579,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)
Expand Down Expand Up @@ -5060,6 +5060,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()) {
Expand Down Expand Up @@ -5548,6 +5552,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;
Expand Down
3 changes: 2 additions & 1 deletion src/test/util/net.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ void ConnmanTestMsg::Handshake(CNode& node,
auto& connman{*this};

peerman.InitializeNode(node, local_services);
FlushSendBuffer(node); // Drop the version message added by InitializeNode.
peerman.SendMessages(&node);
FlushSendBuffer(node); // Drop the version message added by SendMessages.

CSerializedNetMsg msg_version{
NetMsg::Make(NetMsgType::VERSION,
Expand Down

0 comments on commit 0933cf5

Please sign in to comment.