net: stop wolfSSL's CloseSocket macro from turning P2P closes into double closes
wolfssl/wolfio.h (included by <wolfssl/ssl.h> in net.h) does
`#ifndef CloseSocket / #define CloseSocket(s) close(s)`. Every TU that
includes net.h after that point compiles CloseSocket(x) to a bare
close(x). That includes all 15 calls in net.cpp, among them the ones
in CloseSocketDisconnect and ~CNode. util/sock.cpp's
CloseSocket(SOCKET&), the only thing that sets the handle to
INVALID_SOCKET, was never called from net.cpp. This has been the case
since OpenSSL was replaced by wolfSSL (Hush 9ff756590, 2020), so every
DragonX release is affected.
So CNode::hSocket kept its number after the socket was closed. Every
P2P disconnect closed that same number again, 2 to 4 times:
- CloseSocketDisconnect (net.cpp:572) runs once per disconnect trigger
(ThreadSocketHandler, SocketSendData, the TLS socket handler,
ThreadMessageHandler), and each run still saw a valid-looking hSocket;
- ~CNode (net.cpp:2819) then closed it once more.
Meanwhile the socket handler kept FD_SETting, send()ing and recv()ing on
the stale number. In between, other threads get that number from the
kernel's lowest-free-fd rule:
- BDB reopening wallet.dat or database/log.* (ThreadFlushWalletDB, ~11/s);
- fopen("rb+") of a block file for getdata/getblock;
- an HTTP RPC accept.
The stray close then closes it. BDB keeps doing I/O on the dead number:
'wallet.dat: write failed for page N' / 'Illegal seek' / 'BDB0060
PANIC', or silent wallet-page writes into a blocks/blk*.dat that got
the number next. Its failing reads later threw out of ThreadNotifyWallets
(std::terminate). A closed HTTP connection fd makes libevent re-arm
and later close whatever owns that number next ('Epoll ADD ... failed').
Reproduced on isolated regtest with an LD_PRELOAD close() guard:
- one disconnectnode gives exactly one EBADF double close (49
disconnects, 50 EBADF; with strace and no shim, 29 disconnects gave
30 'close(N) = -1 EBADF');
- P2P churn alone gives non-owner closes of block files and wallet.dat;
- RPC-only and Tor-only loads give none.
Disassembly of CloseSocketDisconnect and ~CNode shows 'call close@plt'
before this change and 'call CloseSocket(unsigned int&)' after.
Undefine the macro right after the wolfSSL include in net.h; wolfio.h
is include-guarded, so it can't come back. net.cpp gets an #error
tripwire in case it ever does. NetCloseSocket.ClosedNodeNeverClosesAReusedFdNumber
fails on the old code (a closed CNode closes the fd number's new owner)
and passes now.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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 <gtest/gtest.h>
|
||||
|
||||
#include <fcntl.h>
|
||||
#include <sys/socket.h>
|
||||
#include <unistd.h>
|
||||
|
||||
#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]);
|
||||
}
|
||||
@@ -45,6 +45,13 @@
|
||||
#include <wolfssl/options.h>
|
||||
#include <wolfssl/ssl.h>
|
||||
#include <hush/tlsmanager.cpp>
|
||||
|
||||
// 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;
|
||||
|
||||
@@ -57,6 +57,14 @@
|
||||
#define byte unsigned char
|
||||
#include <wolfssl/ssl.h>
|
||||
#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;
|
||||
|
||||
Reference in New Issue
Block a user