Skip to content

Commit 9c751ef

Browse files
kwvgkwvg
committed
merge bitcoin#23604: Use Sock in CNode
1 parent 508044c commit 9c751ef

File tree

5 files changed

+240
-123
lines changed

5 files changed

+240
-123
lines changed

‎src/net.cpp‎

Lines changed: 64 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -574,7 +574,17 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
574574
if (!addr_bind.IsValid()) {
575575
addr_bind = GetBindAddress(sock->Get());
576576
}
577-
CNode* pnode = new CNode(id, nLocalServices, sock->Release(), addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", conn_type, /* inbound_onion */ false, std::move(i2p_transient_session));
577+
CNode* pnode = new CNode(id,
578+
nLocalServices,
579+
std::move(sock),
580+
addrConnect,
581+
CalculateKeyedNetGroup(addrConnect),
582+
nonce,
583+
addr_bind,
584+
pszDest ? pszDest : "",
585+
conn_type,
586+
/*inbound_onion=*/false,
587+
std::move(i2p_transient_session));
578588
pnode->AddRef();
579589
statsClient.inc("peers.connect", 1.0f);
580590

@@ -589,15 +599,15 @@ void CNode::CloseSocketDisconnect(CConnman* connman)
589599
AssertLockHeld(connman->m_nodes_mutex);
590600

591601
fDisconnect = true;
592-
LOCK2(connman->cs_mapSocketToNode, cs_hSocket);
593-
if (hSocket == INVALID_SOCKET) {
602+
LOCK2(connman->cs_mapSocketToNode, m_sock_mutex);
603+
if (!m_sock) {
594604
return;
595605
}
596606

597607
fHasRecvData = false;
598608
fCanSendData = false;
599609

600-
connman->mapSocketToNode.erase(hSocket);
610+
connman->mapSocketToNode.erase(m_sock->Get());
601611
{
602612
LOCK(connman->cs_sendable_receivable_nodes);
603613
connman->mapReceivableNodes.erase(GetId());
@@ -611,12 +621,12 @@ void CNode::CloseSocketDisconnect(CConnman* connman)
611621
}
612622
}
613623

614-
if (connman->m_edge_trig_events && !connman->m_edge_trig_events->UnregisterEvents(hSocket)) {
624+
if (connman->m_edge_trig_events && !connman->m_edge_trig_events->UnregisterEvents(m_sock->Get())) {
615625
LogPrint(BCLog::NET, "EdgeTriggeredEvents::UnregisterEvents() failed\n");
616626
}
617627

618628
LogPrint(BCLog::NET, "disconnecting peer=%d\n", id);
619-
CloseSocket(hSocket);
629+
m_sock.reset();
620630
m_i2p_sam_session.reset();
621631

622632
statsClient.inc("peers.disconnect", 1.0f);
@@ -909,10 +919,11 @@ size_t CConnman::SocketSendData(CNode& node)
909919
assert(data.size() > node.nSendOffset);
910920
int nBytes = 0;
911921
{
912-
LOCK(node.cs_hSocket);
913-
if (node.hSocket == INVALID_SOCKET)
922+
LOCK(node.m_sock_mutex);
923+
if (!node.m_sock) {
914924
break;
915-
nBytes = send(node.hSocket, reinterpret_cast<const char*>(data.data()) + node.nSendOffset, data.size() - node.nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT);
925+
}
926+
nBytes = node.m_sock->Send(reinterpret_cast<const char*>(data.data()) + node.nSendOffset, data.size() - node.nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT);
916927
}
917928
if (nBytes > 0) {
918929
node.m_last_send = GetTime<std::chrono::seconds>();
@@ -1347,29 +1358,41 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
13471358
}
13481359

13491360
const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end();
1350-
CNode* pnode = new CNode(id, nodeServices, sock->Release(), addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, "", ConnectionType::INBOUND, inbound_onion);
1361+
CNode* pnode = new CNode(id,
1362+
nodeServices,
1363+
std::move(sock),
1364+
addr,
1365+
CalculateKeyedNetGroup(addr),
1366+
nonce,
1367+
addr_bind,
1368+
/*addrNameIn=*/"",
1369+
ConnectionType::INBOUND,
1370+
inbound_onion);
13511371
pnode->AddRef();
13521372
pnode->m_permissionFlags = permissionFlags;
13531373
// If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility)
13541374
pnode->m_legacyWhitelisted = legacyWhitelisted;
13551375
pnode->m_prefer_evict = discouraged;
13561376
m_msgproc->InitializeNode(pnode);
13571377

1358-
if (fLogIPs) {
1359-
LogPrint(BCLog::NET_NETCONN, "connection from %s accepted, sock=%d, peer=%d\n", addr.ToString(), sock->Get(), pnode->GetId());
1360-
} else {
1361-
LogPrint(BCLog::NET_NETCONN, "connection accepted, sock=%d, peer=%d\n", sock->Get(), pnode->GetId());
1378+
{
1379+
LOCK(pnode->m_sock_mutex);
1380+
if (fLogIPs) {
1381+
LogPrint(BCLog::NET_NETCONN, "connection from %s accepted, sock=%d, peer=%d\n", addr.ToString(), pnode->m_sock->Get(), pnode->GetId());
1382+
} else {
1383+
LogPrint(BCLog::NET_NETCONN, "connection accepted, sock=%d, peer=%d\n", pnode->m_sock->Get(), pnode->GetId());
1384+
}
13621385
}
13631386

13641387
{
13651388
LOCK(m_nodes_mutex);
13661389
m_nodes.push_back(pnode);
13671390
}
13681391
{
1369-
LOCK(pnode->cs_hSocket);
1370-
WITH_LOCK(cs_mapSocketToNode, mapSocketToNode.emplace(pnode->hSocket, pnode));
1392+
LOCK2(cs_mapSocketToNode, pnode->m_sock_mutex);
1393+
mapSocketToNode.emplace(pnode->m_sock->Get(), pnode);
13711394
if (m_edge_trig_events) {
1372-
if (!m_edge_trig_events->RegisterEvents(pnode->hSocket)) {
1395+
if (!m_edge_trig_events->RegisterEvents(pnode->m_sock->Get())) {
13731396
LogPrint(BCLog::NET, "EdgeTriggeredEvents::RegisterEvents() failed\n");
13741397
}
13751398
}
@@ -1456,10 +1479,10 @@ void CConnman::DisconnectNodes()
14561479
if (GetTimeMillis() < pnode->nDisconnectLingerTime) {
14571480
// everything flushed to the kernel?
14581481
if (!pnode->fSocketShutdown && pnode->nSendMsgSize == 0) {
1459-
LOCK(pnode->cs_hSocket);
1460-
if (pnode->hSocket != INVALID_SOCKET) {
1482+
LOCK(pnode->m_sock_mutex);
1483+
if (pnode->m_sock) {
14611484
// Give the other side a chance to detect the disconnect as early as possible (recv() will return 0)
1462-
::shutdown(pnode->hSocket, SD_SEND);
1485+
::shutdown(pnode->m_sock->Get(), SD_SEND);
14631486
}
14641487
pnode->fSocketShutdown = true;
14651488
}
@@ -1661,16 +1684,17 @@ bool CConnman::GenerateSelectSet(const std::vector<CNode*>& nodes,
16611684
bool select_recv = !pnode->fHasRecvData;
16621685
bool select_send = !pnode->fCanSendData;
16631686

1664-
LOCK(pnode->cs_hSocket);
1665-
if (pnode->hSocket == INVALID_SOCKET)
1687+
LOCK(pnode->m_sock_mutex);
1688+
if (!pnode->m_sock) {
16661689
continue;
1690+
}
16671691

1668-
error_set.insert(pnode->hSocket);
1692+
error_set.insert(pnode->m_sock->Get());
16691693
if (select_send) {
1670-
send_set.insert(pnode->hSocket);
1694+
send_set.insert(pnode->m_sock->Get());
16711695
}
16721696
if (select_recv) {
1673-
recv_set.insert(pnode->hSocket);
1697+
recv_set.insert(pnode->m_sock->Get());
16741698
}
16751699
}
16761700

@@ -2137,10 +2161,10 @@ size_t CConnman::SocketRecvData(CNode *pnode)
21372161
uint8_t pchBuf[0x10000];
21382162
int nBytes = 0;
21392163
{
2140-
LOCK(pnode->cs_hSocket);
2141-
if (pnode->hSocket == INVALID_SOCKET)
2164+
LOCK(pnode->m_sock_mutex);
2165+
if (!pnode->m_sock)
21422166
return 0;
2143-
nBytes = recv(pnode->hSocket, (char*)pchBuf, sizeof(pchBuf), MSG_DONTWAIT);
2167+
nBytes = recv(pnode->m_sock->Get(), (char*)pchBuf, sizeof(pchBuf), MSG_DONTWAIT);
21442168
if (nBytes < (int)sizeof(pchBuf)) {
21452169
pnode->fHasRecvData = false;
21462170
}
@@ -3047,8 +3071,8 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
30473071
}
30483072

30493073
{
3050-
LOCK(pnode->cs_hSocket);
3051-
LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- successfully connected to %s, sock=%d, peer=%d\n", __func__, getIpStr(), pnode->hSocket, pnode->GetId());
3074+
LOCK(pnode->m_sock_mutex);
3075+
LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- successfully connected to %s, sock=%d, peer=%d\n", __func__, getIpStr(), pnode->m_sock->Get(), pnode->GetId());
30523076
}
30533077

30543078
if (grantOutbound)
@@ -3060,17 +3084,19 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
30603084
pnode->m_masternode_probe_connection = true;
30613085

30623086
{
3063-
LOCK2(cs_mapSocketToNode, pnode->cs_hSocket);
3064-
mapSocketToNode.emplace(pnode->hSocket, pnode);
3087+
LOCK2(cs_mapSocketToNode, pnode->m_sock_mutex);
3088+
mapSocketToNode.emplace(pnode->m_sock->Get(), pnode);
30653089
}
30663090

30673091
m_msgproc->InitializeNode(pnode);
30683092
{
30693093
LOCK(m_nodes_mutex);
30703094
m_nodes.push_back(pnode);
3095+
}
3096+
{
30713097
if (m_edge_trig_events) {
3072-
LOCK(pnode->cs_hSocket);
3073-
if (!m_edge_trig_events->RegisterEvents(pnode->hSocket)) {
3098+
LOCK(pnode->m_sock_mutex);
3099+
if (!m_edge_trig_events->RegisterEvents(pnode->m_sock->Get())) {
30743100
LogPrint(BCLog::NET, "EdgeTriggeredEvents::RegisterEvents() failed\n");
30753101
}
30763102
}
@@ -3579,7 +3605,7 @@ void CConnman::StopNodes()
35793605
pnode->CloseSocketDisconnect(this);
35803606
}
35813607
for (ListenSocket& hListenSocket : vhListenSocket) {
3582-
if (hListenSocket.sock->Get() != INVALID_SOCKET) {
3608+
if (hListenSocket.sock) {
35833609
if (m_edge_trig_events && !m_edge_trig_events->RemoveSocket(hListenSocket.sock->Get())) {
35843610
LogPrintf("EdgeTriggeredEvents::RemoveSocket() failed\n");
35853611
}
@@ -4046,8 +4072,9 @@ ServiceFlags CConnman::GetLocalServices() const
40464072

40474073
unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; }
40484074

4049-
CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion, std::unique_ptr<i2p::sam::Session>&& i2p_sam_session)
4050-
: nTimeConnected{GetTimeSeconds()},
4075+
CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr<Sock> sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion, std::unique_ptr<i2p::sam::Session>&& i2p_sam_session)
4076+
: m_sock{sock},
4077+
nTimeConnected{GetTimeSeconds()},
40514078
addr{addrIn},
40524079
addrBind{addrBindIn},
40534080
m_addr_name{addrNameIn.empty() ? addr.ToStringIPPort() : addrNameIn},
@@ -4060,7 +4087,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const
40604087
m_i2p_sam_session{std::move(i2p_sam_session)}
40614088
{
40624089
if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND);
4063-
hSocket = hSocketIn;
40644090

40654091
for (const std::string &msg : getAllNetMessageTypes())
40664092
mapRecvBytesPerMsgCmd[msg] = 0;
@@ -4076,11 +4102,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const
40764102
m_serializer = std::make_unique<V1TransportSerializer>(V1TransportSerializer());
40774103
}
40784104

4079-
CNode::~CNode()
4080-
{
4081-
CloseSocket(hSocket);
4082-
}
4083-
40844105
bool CConnman::NodeFullyConnected(const CNode* pnode)
40854106
{
40864107
return pnode && pnode->fSuccessfullyConnected && !pnode->fDisconnect;

‎src/net.h‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -440,7 +440,17 @@ class CNode
440440

441441
NetPermissionFlags m_permissionFlags{ NetPermissionFlags::None };
442442
std::atomic<ServiceFlags> nServices{NODE_NONE};
443-
SOCKET hSocket GUARDED_BY(cs_hSocket);
443+
444+
/**
445+
* Socket used for communication with the node.
446+
* May not own a Sock object (after `CloseSocketDisconnect()` or during tests).
447+
* `shared_ptr` (instead of `unique_ptr`) is used to avoid premature close of
448+
* the underlying file descriptor by one thread while another thread is
449+
* poll(2)-ing it for activity.
450+
* @see https://github.com/bitcoin/bitcoin/issues/21744 for details.
451+
*/
452+
std::shared_ptr<Sock> m_sock GUARDED_BY(m_sock_mutex);
453+
444454
/** Total size of all vSendMsg entries */
445455
size_t nSendSize GUARDED_BY(cs_vSend){0};
446456
/** Offset inside the first vSendMsg already sent */
@@ -449,7 +459,7 @@ class CNode
449459
std::list<std::vector<unsigned char>> vSendMsg GUARDED_BY(cs_vSend);
450460
std::atomic<size_t> nSendMsgSize{0};
451461
Mutex cs_vSend;
452-
Mutex cs_hSocket;
462+
Mutex m_sock_mutex;
453463
Mutex cs_vRecv;
454464

455465
RecursiveMutex cs_vProcessMsg;
@@ -630,8 +640,7 @@ class CNode
630640

631641
bool IsBlockRelayOnly() const;
632642

633-
CNode(NodeId id, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn, ConnectionType conn_type_in, bool inbound_onion, std::unique_ptr<i2p::sam::Session>&& i2p_sam_session = nullptr);
634-
~CNode();
643+
CNode(NodeId id, ServiceFlags nLocalServicesIn, std::shared_ptr<Sock> sock, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn, ConnectionType conn_type_in, bool inbound_onion, std::unique_ptr<i2p::sam::Session>&& i2p_sam_session = nullptr);
635644
CNode(const CNode&) = delete;
636645
CNode& operator=(const CNode&) = delete;
637646

@@ -685,7 +694,7 @@ class CNode
685694
nRefCount--;
686695
}
687696

688-
void CloseSocketDisconnect(CConnman* connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_hSocket);
697+
void CloseSocketDisconnect(CConnman* connman) EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex);
689698

690699
void CopyStats(CNodeStats& stats) EXCLUSIVE_LOCKS_REQUIRED(!m_subver_mutex, !m_addr_local_mutex, !cs_vSend, !cs_vRecv);
691700

@@ -795,7 +804,7 @@ class CNode
795804
* closed.
796805
* Otherwise this unique_ptr is empty.
797806
*/
798-
std::unique_ptr<i2p::sam::Session> m_i2p_sam_session GUARDED_BY(cs_hSocket);
807+
std::unique_ptr<i2p::sam::Session> m_i2p_sam_session GUARDED_BY(m_sock_mutex);
799808
};
800809

801810
/**

‎src/test/denialofservice_tests.cpp‎

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,16 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
7474

7575
// Mock an outbound peer
7676
CAddress addr1(ip(0xa0b0c001), NODE_NONE);
77-
CNode dummyNode1(id++, ServiceFlags(NODE_NETWORK), INVALID_SOCKET, addr1, /* nKeyedNetGroupIn */ 0, /* nLocalHostNonceIn */ 0, CAddress(), /* pszDest */ "", ConnectionType::OUTBOUND_FULL_RELAY, /* inbound_onion */ false);
77+
CNode dummyNode1{id++,
78+
ServiceFlags(NODE_NETWORK),
79+
/*sock=*/nullptr,
80+
addr1,
81+
/*nKeyedNetGroupIn=*/0,
82+
/*nLocalHostNonceIn=*/0,
83+
CAddress(),
84+
/*addrNameIn=*/"",
85+
ConnectionType::OUTBOUND_FULL_RELAY,
86+
/*inbound_onion=*/false};
7887
dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
7988

8089
peerLogic->InitializeNode(&dummyNode1);
@@ -124,7 +133,16 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
124133
static void AddRandomOutboundPeer(std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman)
125134
{
126135
CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE);
127-
vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK), INVALID_SOCKET, addr, /* nKeyedNetGroupIn */ 0, /* nLocalHostNonceIn */ 0, CAddress(), /* pszDest */ "", ConnectionType::OUTBOUND_FULL_RELAY, /* inbound_onion */ false));
136+
vNodes.emplace_back(new CNode{id++,
137+
ServiceFlags(NODE_NETWORK),
138+
/*sock=*/nullptr,
139+
addr,
140+
/*nKeyedNetGroupIn=*/0,
141+
/*nLocalHostNonceIn=*/0,
142+
CAddress(),
143+
/*addrNameIn=*/"",
144+
ConnectionType::OUTBOUND_FULL_RELAY,
145+
/*inbound_onion=*/false});
128146
CNode &node = *vNodes.back();
129147
node.SetCommonVersion(PROTOCOL_VERSION);
130148

@@ -220,7 +238,16 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
220238

221239
banman->ClearBanned();
222240
CAddress addr1(ip(0xa0b0c001), NODE_NONE);
223-
CNode dummyNode1(id++, NODE_NETWORK, INVALID_SOCKET, addr1, /* nKeyedNetGroupIn */ 0, /* nLocalHostNonceIn */ 0, CAddress(), /* pszDest */ "", ConnectionType::INBOUND, /* inbound_onion */ false);
241+
CNode dummyNode1{id++,
242+
NODE_NETWORK,
243+
/*sock=*/nullptr,
244+
addr1,
245+
/*nKeyedNetGroupIn=*/0,
246+
/*nLocalHostNonceIn=*/0,
247+
CAddress(),
248+
/*addrNameIn=*/"",
249+
ConnectionType::INBOUND,
250+
/*inbound_onion=*/false};
224251
dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
225252
peerLogic->InitializeNode(&dummyNode1);
226253
dummyNode1.fSuccessfullyConnected = true;
@@ -233,7 +260,16 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
233260
BOOST_CHECK(!banman->IsDiscouraged(ip(0xa0b0c001|0x0000ff00))); // Different IP, not discouraged
234261

235262
CAddress addr2(ip(0xa0b0c002), NODE_NONE);
236-
CNode dummyNode2(id++, NODE_NETWORK, INVALID_SOCKET, addr2, /* nKeyedNetGroupIn */ 1, /* nLocalHostNonceIn */ 1, CAddress(), /* pszDest */ "", ConnectionType::INBOUND, /* inbound_onion */ false);
263+
CNode dummyNode2{id++,
264+
NODE_NETWORK,
265+
/*sock=*/nullptr,
266+
addr2,
267+
/*nKeyedNetGroupIn=*/1,
268+
/*nLocalHostNonceIn=*/1,
269+
CAddress(),
270+
/*pszDest=*/"",
271+
ConnectionType::INBOUND,
272+
/*inbound_onion=*/false};
237273
dummyNode2.SetCommonVersion(PROTOCOL_VERSION);
238274
peerLogic->InitializeNode(&dummyNode2);
239275
dummyNode2.fSuccessfullyConnected = true;
@@ -271,7 +307,16 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
271307
SetMockTime(nStartTime); // Overrides future calls to GetTime()
272308

273309
CAddress addr(ip(0xa0b0c001), NODE_NONE);
274-
CNode dummyNode(id++, NODE_NETWORK, INVALID_SOCKET, addr, /* nKeyedNetGroupIn */ 4, /* nLocalHostNonceIn */ 4, CAddress(), /* pszDest */ "", ConnectionType::INBOUND, /* inbound_onion */ false);
310+
CNode dummyNode{id++,
311+
NODE_NETWORK,
312+
/*sock=*/nullptr,
313+
addr,
314+
/*nKeyedNetGroupIn=*/4,
315+
/*nLocalHostNonceIn=*/4,
316+
CAddress(),
317+
/*addrNameIn=*/"",
318+
ConnectionType::INBOUND,
319+
/*inbound_onion=*/false};
275320
dummyNode.SetCommonVersion(PROTOCOL_VERSION);
276321
peerLogic->InitializeNode(&dummyNode);
277322
dummyNode.fSuccessfullyConnected = true;

0 commit comments

Comments
 (0)