Skip to content

Commit

Permalink
Merge #6054: backport: merge bitcoin#21879, bitcoin#23604, bitcoin#24357
Browse files Browse the repository at this point in the history 
, bitcoin#25426, bitcoin#24378 (sockets backports)

c24804c merge bitcoin#24378: make bind() and listen() mockable/testable (Kittywhiskers Van Gogh)
be19868 merge bitcoin#25426: add new method Sock::GetSockName() that wraps getsockname() and use it in GetBindAddress() (Kittywhiskers Van Gogh)
6b159f1 merge bitcoin#24357: make setsockopt() and SetSocketNoDelay() mockable/testable (Kittywhiskers Van Gogh)
9c751ef merge bitcoin#23604: Use Sock in CNode (Kittywhiskers Van Gogh)
508044c merge bitcoin#21879: wrap accept() and extend usage of Sock (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependency for #6018

  ## Breaking Changes

  None expected.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK c24804c

Tree-SHA512: 5149de0f1983bb56517c30b31d137b33b8a49b0e695be2dada71ff3e3bb22908556db343391b7df7e3c7c2ed60ae1fc11a4f4af4f47e35a2a1d3ce7463c03d41
  • Loading branch information
@PastaPastaPasta
PastaPastaPasta committed Jun 12, 2024
2 parents 2f93ee4 + c24804c commit e4b22a6
Show file tree
Hide file tree
Showing 11 changed files with 505 additions and 172 deletions.
176 changes: 102 additions & 74 deletions src/net.cpp
View file

Large diffs are not rendered by default.

34 changes: 24 additions & 10 deletions src/net.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <uint256.h>
#include <util/check.h>
#include <util/edge.h>
#include <util/sock.h>
#include <util/system.h>
#include <util/wpipe.h>
#include <consensus/params.h>
Expand Down Expand Up @@ -439,7 +440,17 @@ class CNode

NetPermissionFlags m_permissionFlags{ NetPermissionFlags::None };
std::atomic<ServiceFlags> 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<Sock> m_sock GUARDED_BY(m_sock_mutex);

/** Total size of all vSendMsg entries */
size_t nSendSize GUARDED_BY(cs_vSend){0};
/** Offset inside the first vSendMsg already sent */
Expand All @@ -448,7 +459,7 @@ class CNode
std::list<std::vector<unsigned char>> vSendMsg GUARDED_BY(cs_vSend);
std::atomic<size_t> nSendMsgSize{0};
Mutex cs_vSend;
Mutex cs_hSocket;
Mutex m_sock_mutex;
Mutex cs_vRecv;

RecursiveMutex cs_vProcessMsg;
Expand Down Expand Up @@ -629,8 +640,7 @@ class CNode

bool IsBlockRelayOnly() const;

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);
~CNode();
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);
CNode(const CNode&) = delete;
CNode& operator=(const CNode&) = delete;

Expand Down Expand Up @@ -684,7 +694,7 @@ class CNode
nRefCount--;
}

void CloseSocketDisconnect(CConnman* connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_hSocket);
void CloseSocketDisconnect(CConnman* connman) EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex);

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

Expand Down Expand Up @@ -794,7 +804,7 @@ class CNode
* closed.
* Otherwise this unique_ptr is empty.
*/
std::unique_ptr<i2p::sam::Session> m_i2p_sam_session GUARDED_BY(cs_hSocket);
std::unique_ptr<i2p::sam::Session> m_i2p_sam_session GUARDED_BY(m_sock_mutex);
};

/**
Expand Down Expand Up @@ -1221,9 +1231,13 @@ friend class CNode;
private:
struct ListenSocket {
public:
SOCKET socket;
std::shared_ptr<Sock> sock;
inline void AddSocketPermissionFlags(NetPermissionFlags& flags) const { NetPermissions::AddFlag(flags, m_permissions); }
ListenSocket(SOCKET socket_, NetPermissionFlags permissions_) : socket(socket_), m_permissions(permissions_) {}
ListenSocket(std::shared_ptr<Sock> sock_, NetPermissionFlags permissions_)
: sock{sock_}, m_permissions{permissions_}
{
}

private:
NetPermissionFlags m_permissions;
};
Expand Down Expand Up @@ -1251,12 +1265,12 @@ friend class CNode;
/**
* Create a `CNode` object from a socket that has just been accepted and add the node to
* the `m_nodes` member.
* @param[in] hSocket Connected socket to communicate with the peer.
* @param[in] sock Connected socket to communicate with the peer.
* @param[in] permissionFlags The peer's permissions.
* @param[in] addr_bind The address and port at our side of the connection.
* @param[in] addr The address and port at the peer's side of the connection.
*/
void CreateNodeFromAcceptedSocket(SOCKET hSocket,
void CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
NetPermissionFlags permissionFlags,
const CAddress& addr_bind,
const CAddress& addr,
Expand Down
27 changes: 13 additions & 14 deletions src/netbase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -498,10 +498,11 @@ std::unique_ptr<Sock> CreateSockTCP(const CService& address_family)
return nullptr;
}

auto sock = std::make_unique<Sock>(hSocket);

// Ensure that waiting for I/O on this socket won't result in undefined
// behavior.
if (!IsSelectableSocket(hSocket)) {
CloseSocket(hSocket);
if (!IsSelectableSocket(sock->Get())) {
LogPrintf("Cannot create connection: non-selectable socket created (fd >= FD_SETSIZE ?)\n");
return nullptr;
}
Expand All @@ -510,19 +511,24 @@ std::unique_ptr<Sock> CreateSockTCP(const CService& address_family)
int set = 1;
// Set the no-sigpipe option on the socket for BSD systems, other UNIXes
// should use the MSG_NOSIGNAL flag for every send.
setsockopt(hSocket, SOL_SOCKET, SO_NOSIGPIPE, (void*)&set, sizeof(int));
if (sock->SetSockOpt(SOL_SOCKET, SO_NOSIGPIPE, (void*)&set, sizeof(int)) == SOCKET_ERROR) {
LogPrintf("Error setting SO_NOSIGPIPE on socket: %s, continuing anyway\n",
NetworkErrorString(WSAGetLastError()));
}
#endif

// Set the no-delay option (disable Nagle's algorithm) on the TCP socket.
SetSocketNoDelay(hSocket);
const int on{1};
if (sock->SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == SOCKET_ERROR) {
LogPrint(BCLog::NET, "Unable to set TCP_NODELAY on a newly created socket, continuing anyway\n");
}

// Set the non-blocking option on the socket.
if (!SetSocketNonBlocking(hSocket)) {
CloseSocket(hSocket);
if (!SetSocketNonBlocking(sock->Get())) {
LogPrintf("Error setting socket to non-blocking: %s\n", NetworkErrorString(WSAGetLastError()));
return nullptr;
}
return std::make_unique<Sock>(hSocket);
return sock;
}

std::function<std::unique_ptr<Sock>(const CService&)> CreateSock = CreateSockTCP;
Expand Down Expand Up @@ -729,13 +735,6 @@ bool SetSocketNonBlocking(const SOCKET& hSocket)
return true;
}

bool SetSocketNoDelay(const SOCKET& hSocket)
{
int set = 1;
int rc = setsockopt(hSocket, IPPROTO_TCP, TCP_NODELAY, (const char*)&set, sizeof(int));
return rc == 0;
}

void InterruptSocks5(bool interrupt)
{
interruptSocks5Recv = interrupt;
Expand Down
2 changes: 0 additions & 2 deletions src/netbase.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,6 @@ bool ConnectThroughProxy(const Proxy& proxy, const std::string& strDest, uint16_

/** Enable non-blocking mode for a socket */
bool SetSocketNonBlocking(const SOCKET& hSocket);
/** Set the TCP_NODELAY flag on a socket */
bool SetSocketNoDelay(const SOCKET& hSocket);
void InterruptSocks5(bool interrupt);

/**
Expand Down
55 changes: 50 additions & 5 deletions src/test/denialofservice_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,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), INVALID_SOCKET, addr1, /* nKeyedNetGroupIn */ 0, /* nLocalHostNonceIn */ 0, CAddress(), /* pszDest */ "", ConnectionType::OUTBOUND_FULL_RELAY, /* inbound_onion */ false);
CNode dummyNode1{id++,
ServiceFlags(NODE_NETWORK),
/*sock=*/nullptr,
addr1,
/*nKeyedNetGroupIn=*/0,
/*nLocalHostNonceIn=*/0,
CAddress(),
/*addrNameIn=*/"",
ConnectionType::OUTBOUND_FULL_RELAY,
/*inbound_onion=*/false};
dummyNode1.SetCommonVersion(PROTOCOL_VERSION);

peerLogic->InitializeNode(&dummyNode1);
Expand Down Expand Up @@ -124,7 +133,16 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
static void AddRandomOutboundPeer(std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman)
{
CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE);
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));
vNodes.emplace_back(new CNode{id++,
ServiceFlags(NODE_NETWORK),
/*sock=*/nullptr,
addr,
/*nKeyedNetGroupIn=*/0,
/*nLocalHostNonceIn=*/0,
CAddress(),
/*addrNameIn=*/"",
ConnectionType::OUTBOUND_FULL_RELAY,
/*inbound_onion=*/false});
CNode &node = *vNodes.back();
node.SetCommonVersion(PROTOCOL_VERSION);

Expand Down Expand Up @@ -220,7 +238,16 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)

banman->ClearBanned();
CAddress addr1(ip(0xa0b0c001), NODE_NONE);
CNode dummyNode1(id++, NODE_NETWORK, INVALID_SOCKET, addr1, /* nKeyedNetGroupIn */ 0, /* nLocalHostNonceIn */ 0, CAddress(), /* pszDest */ "", ConnectionType::INBOUND, /* inbound_onion */ false);
CNode dummyNode1{id++,
NODE_NETWORK,
/*sock=*/nullptr,
addr1,
/*nKeyedNetGroupIn=*/0,
/*nLocalHostNonceIn=*/0,
CAddress(),
/*addrNameIn=*/"",
ConnectionType::INBOUND,
/*inbound_onion=*/false};
dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
peerLogic->InitializeNode(&dummyNode1);
dummyNode1.fSuccessfullyConnected = true;
Expand All @@ -233,7 +260,16 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
BOOST_CHECK(!banman->IsDiscouraged(ip(0xa0b0c001|0x0000ff00))); // Different IP, not discouraged

CAddress addr2(ip(0xa0b0c002), NODE_NONE);
CNode dummyNode2(id++, NODE_NETWORK, INVALID_SOCKET, addr2, /* nKeyedNetGroupIn */ 1, /* nLocalHostNonceIn */ 1, CAddress(), /* pszDest */ "", ConnectionType::INBOUND, /* inbound_onion */ false);
CNode dummyNode2{id++,
NODE_NETWORK,
/*sock=*/nullptr,
addr2,
/*nKeyedNetGroupIn=*/1,
/*nLocalHostNonceIn=*/1,
CAddress(),
/*pszDest=*/"",
ConnectionType::INBOUND,
/*inbound_onion=*/false};
dummyNode2.SetCommonVersion(PROTOCOL_VERSION);
peerLogic->InitializeNode(&dummyNode2);
dummyNode2.fSuccessfullyConnected = true;
Expand Down Expand Up @@ -271,7 +307,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(), /* pszDest */ "", 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;
Expand Down
82 changes: 82 additions & 0 deletions src/test/fuzz/util.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
#include <util/time.h>
#include <version.h>

#include <memory>

FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider)
: m_fuzzed_data_provider{fuzzed_data_provider}
{
Expand Down Expand Up @@ -155,6 +157,59 @@ int FuzzedSock::Connect(const sockaddr*, socklen_t) const
return 0;
}

int FuzzedSock::Bind(const sockaddr*, socklen_t) const
{
// Have a permanent error at bind_errnos[0] because when the fuzzed data is exhausted
// SetFuzzedErrNo() will always set the global errno to bind_errnos[0]. We want to
// avoid this method returning -1 and setting errno to a temporary error (like EAGAIN)
// repeatedly because proper code should retry on temporary errors, leading to an
// infinite loop.
constexpr std::array bind_errnos{
EACCES,
EADDRINUSE,
EADDRNOTAVAIL,
EAGAIN,
};
if (m_fuzzed_data_provider.ConsumeBool()) {
SetFuzzedErrNo(m_fuzzed_data_provider, bind_errnos);
return -1;
}
return 0;
}

int FuzzedSock::Listen(int) const
{
// Have a permanent error at listen_errnos[0] because when the fuzzed data is exhausted
// SetFuzzedErrNo() will always set the global errno to listen_errnos[0]. We want to
// avoid this method returning -1 and setting errno to a temporary error (like EAGAIN)
// repeatedly because proper code should retry on temporary errors, leading to an
// infinite loop.
constexpr std::array listen_errnos{
EADDRINUSE,
EINVAL,
EOPNOTSUPP,
};
if (m_fuzzed_data_provider.ConsumeBool()) {
SetFuzzedErrNo(m_fuzzed_data_provider, listen_errnos);
return -1;
}
return 0;
}

std::unique_ptr<Sock> FuzzedSock::Accept(sockaddr* addr, socklen_t* addr_len) const
{
constexpr std::array accept_errnos{
ECONNABORTED,
EINTR,
ENOMEM,
};
if (m_fuzzed_data_provider.ConsumeBool()) {
SetFuzzedErrNo(m_fuzzed_data_provider, accept_errnos);
return std::unique_ptr<FuzzedSock>();
}
return std::make_unique<FuzzedSock>(m_fuzzed_data_provider);
}

int FuzzedSock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const
{
constexpr std::array getsockopt_errnos{
Expand All @@ -174,6 +229,33 @@ int FuzzedSock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* op
return 0;
}

int FuzzedSock::SetSockOpt(int, int, const void*, socklen_t) const
{
constexpr std::array setsockopt_errnos{
ENOMEM,
ENOBUFS,
};
if (m_fuzzed_data_provider.ConsumeBool()) {
SetFuzzedErrNo(m_fuzzed_data_provider, setsockopt_errnos);
return -1;
}
return 0;
}

int FuzzedSock::GetSockName(sockaddr* name, socklen_t* name_len) const
{
constexpr std::array getsockname_errnos{
ECONNRESET,
ENOBUFS,
};
if (m_fuzzed_data_provider.ConsumeBool()) {
SetFuzzedErrNo(m_fuzzed_data_provider, getsockname_errnos);
return -1;
}
*name_len = m_fuzzed_data_provider.ConsumeData(name, *name_len);
return 0;
}

bool FuzzedSock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred) const
{
constexpr std::array wait_errnos{
Expand Down
Loading

0 comments on commit e4b22a6

Please sign in to comment.