From c41a1162ac4da437c5d755e8fe2bf636bed22b0f Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 23 Apr 2021 15:15:23 +0200 Subject: net: use Sock in CNode Change `CNode` to use a pointer to `Sock` instead of a bare `SOCKET`. This will help mocking / testing / fuzzing more code. --- src/net.cpp | 48 ++++++++++++------------ src/net.h | 15 ++++++-- src/test/denialofservice_tests.cpp | 72 ++++++++++++++++++++++++++++++------ src/test/fuzz/util.h | 24 ++++++++++-- src/test/net_tests.cpp | 76 +++++++++++++++++++++++++------------- 5 files changed, 166 insertions(+), 69 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 89a4aee5d9..d94542887b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -505,7 +505,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo if (!addr_bind.IsValid()) { addr_bind = GetBindAddress(sock->Get()); } - CNode* pnode = new CNode(id, nLocalServices, sock->Release(), addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", conn_type, /* inbound_onion */ false); + CNode* pnode = new CNode(id, nLocalServices, std::move(sock), addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", conn_type, /* inbound_onion */ false); pnode->AddRef(); // We're making a new connection, harvest entropy from the time (and our peer count) @@ -518,10 +518,9 @@ void CNode::CloseSocketDisconnect() { fDisconnect = true; LOCK(cs_hSocket); - if (hSocket != INVALID_SOCKET) - { + if (m_sock) { LogPrint(BCLog::NET, "disconnecting peer=%d\n", id); - CloseSocket(hSocket); + m_sock.reset(); } } @@ -800,9 +799,10 @@ size_t CConnman::SocketSendData(CNode& node) const int nBytes = 0; { LOCK(node.cs_hSocket); - if (node.hSocket == INVALID_SOCKET) + if (!node.m_sock) { break; - nBytes = send(node.hSocket, reinterpret_cast(data.data()) + node.nSendOffset, data.size() - node.nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT); + } + nBytes = node.m_sock->Send(reinterpret_cast(data.data()) + node.nSendOffset, data.size() - node.nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT); } if (nBytes > 0) { node.m_last_send = GetTime(); @@ -1197,7 +1197,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, } const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end(); - CNode* pnode = new CNode(id, nodeServices, sock->Release(), addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, "", ConnectionType::INBOUND, inbound_onion); + CNode* pnode = new CNode(id, nodeServices, std::move(sock), addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, "", ConnectionType::INBOUND, inbound_onion); pnode->AddRef(); pnode->m_permissionFlags = permissionFlags; pnode->m_prefer_evict = discouraged; @@ -1382,16 +1382,17 @@ bool CConnman::GenerateSelectSet(const std::vector& nodes, } LOCK(pnode->cs_hSocket); - if (pnode->hSocket == INVALID_SOCKET) + if (!pnode->m_sock) { continue; + } - error_set.insert(pnode->hSocket); + error_set.insert(pnode->m_sock->Get()); if (select_send) { - send_set.insert(pnode->hSocket); + send_set.insert(pnode->m_sock->Get()); continue; } if (select_recv) { - recv_set.insert(pnode->hSocket); + recv_set.insert(pnode->m_sock->Get()); } } @@ -1562,11 +1563,12 @@ void CConnman::SocketHandlerConnected(const std::vector& nodes, bool errorSet = false; { LOCK(pnode->cs_hSocket); - if (pnode->hSocket == INVALID_SOCKET) + if (!pnode->m_sock) { continue; - recvSet = recv_set.count(pnode->hSocket) > 0; - sendSet = send_set.count(pnode->hSocket) > 0; - errorSet = error_set.count(pnode->hSocket) > 0; + } + recvSet = recv_set.count(pnode->m_sock->Get()) > 0; + sendSet = send_set.count(pnode->m_sock->Get()) > 0; + errorSet = error_set.count(pnode->m_sock->Get()) > 0; } if (recvSet || errorSet) { @@ -1575,9 +1577,10 @@ void CConnman::SocketHandlerConnected(const std::vector& nodes, int nBytes = 0; { LOCK(pnode->cs_hSocket); - if (pnode->hSocket == INVALID_SOCKET) + if (!pnode->m_sock) { continue; - nBytes = recv(pnode->hSocket, (char*)pchBuf, sizeof(pchBuf), MSG_DONTWAIT); + } + nBytes = pnode->m_sock->Recv(pchBuf, sizeof(pchBuf), MSG_DONTWAIT); } if (nBytes > 0) { @@ -2962,8 +2965,9 @@ ServiceFlags CConnman::GetLocalServices() const unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; } -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) - : m_connected{GetTime()}, +CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion) + : m_sock{sock}, + m_connected{GetTime()}, addr(addrIn), addrBind(addrBindIn), m_addr_name{addrNameIn.empty() ? addr.ToStringIPPort() : addrNameIn}, @@ -2975,7 +2979,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const nLocalServices(nLocalServicesIn) { if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND); - hSocket = hSocketIn; if (conn_type_in != ConnectionType::BLOCK_RELAY) { m_tx_relay = std::make_unique(); } @@ -2994,11 +2997,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const m_serializer = std::make_unique(V1TransportSerializer()); } -CNode::~CNode() -{ - CloseSocket(hSocket); -} - bool CConnman::NodeFullyConnected(const CNode* pnode) { return pnode && pnode->fSuccessfullyConnected && !pnode->fDisconnect; diff --git a/src/net.h b/src/net.h index 80fc93a5d0..e0e0e930bf 100644 --- a/src/net.h +++ b/src/net.h @@ -402,7 +402,17 @@ public: NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; std::atomic nServices{NODE_NONE}; - SOCKET hSocket GUARDED_BY(cs_hSocket); + + /** + * Socket used for communication with the node. + * May not own a Sock object (after `CloseSocketDisconnect()` or during tests). + * `shared_ptr` (instead of `unique_ptr`) is used to avoid premature close of + * the underlying file descriptor by one thread while another thread is + * poll(2)-ing it for activity. + * @see https://github.com/bitcoin/bitcoin/issues/21744 for details. + */ + std::shared_ptr m_sock GUARDED_BY(cs_hSocket); + /** Total size of all vSendMsg entries */ size_t nSendSize GUARDED_BY(cs_vSend){0}; /** Offset inside the first vSendMsg already sent */ @@ -578,8 +588,7 @@ public: * criterium in CConnman::AttemptToEvictConnection. */ std::atomic m_min_ping_time{std::chrono::microseconds::max()}; - 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); - ~CNode(); + CNode(NodeId id, ServiceFlags nLocalServicesIn, std::shared_ptr sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion); CNode(const CNode&) = delete; CNode& operator=(const CNode&) = delete; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 42ca41f4ee..a17cc87730 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -59,7 +59,16 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) // Mock an outbound peer CAddress addr1(ip(0xa0b0c001), NODE_NONE); - CNode dummyNode1(id++, ServiceFlags(NODE_NETWORK | NODE_WITNESS), INVALID_SOCKET, addr1, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, CAddress(), /*addrNameIn=*/"", ConnectionType::OUTBOUND_FULL_RELAY, /*inbound_onion=*/false); + CNode dummyNode1{id++, + ServiceFlags(NODE_NETWORK | NODE_WITNESS), + /*sock=*/nullptr, + addr1, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + CAddress(), + /*addrNameIn=*/"", + ConnectionType::OUTBOUND_FULL_RELAY, + /*inbound_onion=*/false}; dummyNode1.SetCommonVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode1); @@ -108,7 +117,16 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) static void AddRandomOutboundPeer(std::vector& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType) { CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE); - vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK | NODE_WITNESS), INVALID_SOCKET, addr, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, CAddress(), /*addrNameIn=*/"", connType, /*inbound_onion=*/false)); + vNodes.emplace_back(new CNode{id++, + ServiceFlags(NODE_NETWORK | NODE_WITNESS), + /*sock=*/nullptr, + addr, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + CAddress(), + /*addrNameIn=*/"", + connType, + /*inbound_onion=*/false}); CNode &node = *vNodes.back(); node.SetCommonVersion(PROTOCOL_VERSION); @@ -279,9 +297,16 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) std::array nodes; banman->ClearBanned(); - nodes[0] = new CNode{id++, NODE_NETWORK, INVALID_SOCKET, addr[0], /*nKeyedNetGroupIn=*/0, - /*nLocalHostNonceIn=*/0, CAddress(), /*addrNameIn=*/"", - ConnectionType::INBOUND, /*inbound_onion=*/false}; + nodes[0] = new CNode{id++, + NODE_NETWORK, + /*sock=*/nullptr, + addr[0], + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + CAddress(), + /*addrNameIn=*/"", + ConnectionType::INBOUND, + /*inbound_onion=*/false}; nodes[0]->SetCommonVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(nodes[0]); nodes[0]->fSuccessfullyConnected = true; @@ -295,9 +320,16 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) BOOST_CHECK(nodes[0]->fDisconnect); BOOST_CHECK(!banman->IsDiscouraged(other_addr)); // Different address, not discouraged - nodes[1] = new CNode{id++, NODE_NETWORK, INVALID_SOCKET, addr[1], /*nKeyedNetGroupIn=*/1, - /*nLocalHostNonceIn=*/1, CAddress(), /*addrNameIn=*/"", - ConnectionType::INBOUND, /*inbound_onion=*/false}; + nodes[1] = new CNode{id++, + NODE_NETWORK, + /*sock=*/nullptr, + addr[1], + /*nKeyedNetGroupIn=*/1, + /*nLocalHostNonceIn=*/1, + CAddress(), + /*addrNameIn=*/"", + ConnectionType::INBOUND, + /*inbound_onion=*/false}; nodes[1]->SetCommonVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(nodes[1]); nodes[1]->fSuccessfullyConnected = true; @@ -326,9 +358,16 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) // Make sure non-IP peers are discouraged and disconnected properly. - nodes[2] = new CNode{id++, NODE_NETWORK, INVALID_SOCKET, addr[2], /*nKeyedNetGroupIn=*/1, - /*nLocalHostNonceIn=*/1, CAddress(), /*addrNameIn=*/"", - ConnectionType::OUTBOUND_FULL_RELAY, /*inbound_onion=*/false}; + nodes[2] = new CNode{id++, + NODE_NETWORK, + /*sock=*/nullptr, + addr[2], + /*nKeyedNetGroupIn=*/1, + /*nLocalHostNonceIn=*/1, + CAddress(), + /*addrNameIn=*/"", + ConnectionType::OUTBOUND_FULL_RELAY, + /*inbound_onion=*/false}; nodes[2]->SetCommonVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(nodes[2]); nodes[2]->fSuccessfullyConnected = true; @@ -364,7 +403,16 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) SetMockTime(nStartTime); // Overrides future calls to GetTime() CAddress addr(ip(0xa0b0c001), NODE_NONE); - CNode dummyNode(id++, NODE_NETWORK, INVALID_SOCKET, addr, /*nKeyedNetGroupIn=*/4, /*nLocalHostNonceIn=*/4, CAddress(), /*addrNameIn=*/"", ConnectionType::INBOUND, /*inbound_onion=*/false); + CNode dummyNode{id++, + NODE_NETWORK, + /*sock=*/nullptr, + addr, + /*nKeyedNetGroupIn=*/4, + /*nLocalHostNonceIn=*/4, + CAddress(), + /*addrNameIn=*/"", + ConnectionType::INBOUND, + /*inbound_onion=*/false}; dummyNode.SetCommonVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode); dummyNode.fSuccessfullyConnected = true; diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index 93299f11a9..019c75c364 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -290,7 +290,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional(0, std::numeric_limits::max())); const ServiceFlags local_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS); - const SOCKET socket = INVALID_SOCKET; + const auto sock = std::make_shared(fuzzed_data_provider); const CAddress address = ConsumeAddress(fuzzed_data_provider); const uint64_t keyed_net_group = fuzzed_data_provider.ConsumeIntegral(); const uint64_t local_host_nonce = fuzzed_data_provider.ConsumeIntegral(); @@ -299,9 +299,27 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional(node_id, local_services, socket, address, keyed_net_group, local_host_nonce, addr_bind, addr_name, conn_type, inbound_onion); + return std::make_unique(node_id, + local_services, + sock, + address, + keyed_net_group, + local_host_nonce, + addr_bind, + addr_name, + conn_type, + inbound_onion); } else { - return CNode{node_id, local_services, socket, address, keyed_net_group, local_host_nonce, addr_bind, addr_name, conn_type, inbound_onion}; + return CNode{node_id, + local_services, + sock, + address, + keyed_net_group, + local_host_nonce, + addr_bind, + addr_name, + conn_type, + inbound_onion}; } } inline std::unique_ptr ConsumeNodeAsUniquePtr(FuzzedDataProvider& fdp, const std::optional& node_id_in = std::nullopt) { return ConsumeNode(fdp, node_id_in); } diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 86786af450..6f850114d3 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -43,7 +43,6 @@ BOOST_AUTO_TEST_CASE(cnode_listen_port) BOOST_AUTO_TEST_CASE(cnode_simple_test) { - SOCKET hSocket = INVALID_SOCKET; NodeId id = 0; in_addr ipv4Addr; @@ -52,12 +51,16 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) CAddress addr = CAddress(CService(ipv4Addr, 7777), NODE_NETWORK); std::string pszDest; - std::unique_ptr pnode1 = std::make_unique( - id++, NODE_NETWORK, hSocket, addr, - /* nKeyedNetGroupIn = */ 0, - /* nLocalHostNonceIn = */ 0, - CAddress(), pszDest, ConnectionType::OUTBOUND_FULL_RELAY, - /* inbound_onion = */ false); + std::unique_ptr pnode1 = std::make_unique(id++, + NODE_NETWORK, + /*sock=*/nullptr, + addr, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + CAddress(), + pszDest, + ConnectionType::OUTBOUND_FULL_RELAY, + /*inbound_onion=*/false); BOOST_CHECK(pnode1->IsFullOutboundConn() == true); BOOST_CHECK(pnode1->IsManualConn() == false); BOOST_CHECK(pnode1->IsBlockOnlyConn() == false); @@ -67,12 +70,16 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) BOOST_CHECK(pnode1->m_inbound_onion == false); BOOST_CHECK_EQUAL(pnode1->ConnectedThroughNetwork(), Network::NET_IPV4); - std::unique_ptr pnode2 = std::make_unique( - id++, NODE_NETWORK, hSocket, addr, - /* nKeyedNetGroupIn = */ 1, - /* nLocalHostNonceIn = */ 1, - CAddress(), pszDest, ConnectionType::INBOUND, - /* inbound_onion = */ false); + std::unique_ptr pnode2 = std::make_unique(id++, + NODE_NETWORK, + /*sock=*/nullptr, + addr, + /*nKeyedNetGroupIn=*/1, + /*nLocalHostNonceIn=*/1, + CAddress(), + pszDest, + ConnectionType::INBOUND, + /*inbound_onion=*/false); BOOST_CHECK(pnode2->IsFullOutboundConn() == false); BOOST_CHECK(pnode2->IsManualConn() == false); BOOST_CHECK(pnode2->IsBlockOnlyConn() == false); @@ -82,12 +89,16 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) BOOST_CHECK(pnode2->m_inbound_onion == false); BOOST_CHECK_EQUAL(pnode2->ConnectedThroughNetwork(), Network::NET_IPV4); - std::unique_ptr pnode3 = std::make_unique( - id++, NODE_NETWORK, hSocket, addr, - /* nKeyedNetGroupIn = */ 0, - /* nLocalHostNonceIn = */ 0, - CAddress(), pszDest, ConnectionType::OUTBOUND_FULL_RELAY, - /* inbound_onion = */ false); + std::unique_ptr pnode3 = std::make_unique(id++, + NODE_NETWORK, + /*sock=*/nullptr, + addr, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + CAddress(), + pszDest, + ConnectionType::OUTBOUND_FULL_RELAY, + /*inbound_onion=*/false); BOOST_CHECK(pnode3->IsFullOutboundConn() == true); BOOST_CHECK(pnode3->IsManualConn() == false); BOOST_CHECK(pnode3->IsBlockOnlyConn() == false); @@ -97,12 +108,16 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) BOOST_CHECK(pnode3->m_inbound_onion == false); BOOST_CHECK_EQUAL(pnode3->ConnectedThroughNetwork(), Network::NET_IPV4); - std::unique_ptr pnode4 = std::make_unique( - id++, NODE_NETWORK, hSocket, addr, - /* nKeyedNetGroupIn = */ 1, - /* nLocalHostNonceIn = */ 1, - CAddress(), pszDest, ConnectionType::INBOUND, - /* inbound_onion = */ true); + std::unique_ptr pnode4 = std::make_unique(id++, + NODE_NETWORK, + /*sock=*/nullptr, + addr, + /*nKeyedNetGroupIn=*/1, + /*nLocalHostNonceIn=*/1, + CAddress(), + pszDest, + ConnectionType::INBOUND, + /*inbound_onion=*/true); BOOST_CHECK(pnode4->IsFullOutboundConn() == false); BOOST_CHECK(pnode4->IsManualConn() == false); BOOST_CHECK(pnode4->IsBlockOnlyConn() == false); @@ -607,7 +622,16 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test) in_addr ipv4AddrPeer; ipv4AddrPeer.s_addr = 0xa0b0c001; CAddress addr = CAddress(CService(ipv4AddrPeer, 7777), NODE_NETWORK); - std::unique_ptr pnode = std::make_unique(0, NODE_NETWORK, INVALID_SOCKET, addr, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, CAddress{}, /*pszDest=*/std::string{}, ConnectionType::OUTBOUND_FULL_RELAY, /*inbound_onion=*/false); + std::unique_ptr pnode = std::make_unique(/*id=*/0, + NODE_NETWORK, + /*sock=*/nullptr, + addr, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + CAddress{}, + /*pszDest=*/std::string{}, + ConnectionType::OUTBOUND_FULL_RELAY, + /*inbound_onion=*/false); pnode->fSuccessfullyConnected.store(true); // the peer claims to be reaching us via IPv6 -- cgit v1.2.3