diff --git a/src/Makefile.gtest.include b/src/Makefile.gtest.include index 613b215be..a79fec340 100644 --- a/src/Makefile.gtest.include +++ b/src/Makefile.gtest.include @@ -15,7 +15,8 @@ hush_gtest_SOURCES = \ gtest/test_hdtransparent.cpp \ gtest/test_mnemonic_compat.cpp \ gtest/test_stratum_jobid.cpp \ - gtest/test_wallet_notedata.cpp + gtest/test_wallet_notedata.cpp \ + gtest/test_net_closesocket.cpp hush_gtest_CPPFLAGS = $(AM_CPPFLAGS) -DMULTICORE -fopenmp -DBINARY_OUTPUT -DCURVE_ALT_BN128 -DSTATIC $(BITCOIN_INCLUDES) hush_gtest_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) diff --git a/src/gtest/test_net_closesocket.cpp b/src/gtest/test_net_closesocket.cpp new file mode 100644 index 000000000..b6fcd76e1 --- /dev/null +++ b/src/gtest/test_net_closesocket.cpp @@ -0,0 +1,50 @@ +// Copyright (c) 2016-2024 The Hush developers +// Distributed under the GPLv3 software license, see the accompanying +// file COPYING or https://www.gnu.org/licenses/gpl-3.0.en.html + +#include + +#include +#include +#include + +#include "net.h" +#include "util/sock.h" + +// Regression test for the wolfSSL CloseSocket macro (see net.h): closing a CNode's socket must invalidate +// hSocket, so neither a repeated CloseSocketDisconnect nor ~CNode can close that fd number again once another +// owner (the wallet's BDB files, a block file, an RPC socket) has been handed it. +TEST(NetCloseSocket, ClosedNodeNeverClosesAReusedFdNumber) { + int pair[2]; + ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM, 0, pair)); + const int number = pair[0]; + + CNode* node = new CNode(number, CAddress(), "", true); // inbound: sends no version message + node->CloseSocketDisconnect(); + EXPECT_EQ(INVALID_SOCKET, node->hSocket) << "CloseSocketDisconnect left the closed fd number in hSocket"; + EXPECT_EQ(-1, fcntl(number, F_GETFD)) << "the node's socket wasn't closed"; + + // Another owner gets the freed number, as the kernel's lowest-free-fd rule does in a busy node. + const int other = open("/dev/null", O_RDONLY); + ASSERT_GE(other, 0); + ASSERT_EQ(number, dup2(other, number)); + + node->CloseSocketDisconnect(); // e.g. a send error after the disconnect was already handled + delete node; // ~CNode + EXPECT_NE(-1, fcntl(number, F_GETFD)) << "a closed CNode closed the fd number's new owner"; + + close(number); + close(other); + close(pair[1]); +} + +// util/sock.cpp's CloseSocket is the one that must be called: it closes once and invalidates. +TEST(NetCloseSocket, CloseSocketInvalidates) { + int pair[2]; + ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM, 0, pair)); + SOCKET s = pair[0]; + EXPECT_TRUE(CloseSocket(s)); + EXPECT_EQ(INVALID_SOCKET, s); + EXPECT_FALSE(CloseSocket(s)) << "a second close of an invalidated handle must be a no-op"; + close(pair[1]); +} diff --git a/src/net.cpp b/src/net.cpp index 2aab1dfcd..363930fc9 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -45,6 +45,13 @@ #include #include #include + +// CloseSocket must be util/sock.cpp's CloseSocket(SOCKET&), which invalidates the handle it closes. If +// wolfSSL's `#define CloseSocket(s) close(s)` survived into this file (see net.h), each close here would leave +// its stale fd number behind to be closed again after another thread reused it. +#ifdef CloseSocket +#error "CloseSocket is a macro here (wolfssl/wolfio.h): it would bypass CloseSocket(SOCKET&) in util/sock.cpp" +#endif using namespace hush; extern int32_t HUSH_TESTNODE; extern uint8_t ASSETCHAINS_CLEARNET; diff --git a/src/net.h b/src/net.h index 2627e071a..7e264502f 100644 --- a/src/net.h +++ b/src/net.h @@ -57,6 +57,14 @@ #define byte unsigned char #include #undef byte +// wolfssl/wolfio.h (pulled in by ssl.h) does `#ifndef CloseSocket / #define CloseSocket(s) close(s)`. Left +// defined, it turns every CloseSocket(hSocket) after this point -- all 15 calls in net.cpp, including the ones in +// CloseSocketDisconnect and ~CNode -- into a bare close() that never sets hSocket = INVALID_SOCKET, so util/sock.cpp's +// CloseSocket(SOCKET&) is never called. Every P2P disconnect then closed its fd number again later (a repeated +// CloseSocketDisconnect, then ~CNode), by which time another thread often owned that number: wallet.dat or a +// database/log.* fd (BDB then wrote wallet pages into whatever got the number next, e.g. a blocks/*.dat file +// opened "rb+"), a block file, or an HTTP RPC socket. wolfio.h is include-guarded, so it can't come back. +#undef CloseSocket class CAddrMan; class CBlockIndex;