From e883b37768812d96feec207a37202c7d1b603c1f Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 15 Nov 2024 15:06:39 +0100 Subject: [PATCH 1/7] fuzz: set the output argument of FuzzedSock::Accept() `FuzzedSock::Accept()` properly returns a new socket, but it forgot to set the output argument `addr`, like `accept(2)` is expected to. This could lead to reading uninitialized data during testing when we read it, e.g. from `CService::SetSockAddr()` which reads the `sa_family` member. Set `addr` to a fuzzed IPv4 or IPv6 address. --- src/test/fuzz/util/net.cpp | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/test/fuzz/util/net.cpp b/src/test/fuzz/util/net.cpp index 8cbf6bdffec..e49c043364e 100644 --- a/src/test/fuzz/util/net.cpp +++ b/src/test/fuzz/util/net.cpp @@ -312,6 +312,33 @@ std::unique_ptr FuzzedSock::Accept(sockaddr* addr, socklen_t* addr_len) co SetFuzzedErrNo(m_fuzzed_data_provider, accept_errnos); return std::unique_ptr(); } + if (addr != nullptr) { + // Set a fuzzed address in the output argument addr. + memset(addr, 0x00, *addr_len); + if (m_fuzzed_data_provider.ConsumeBool()) { + // IPv4 + const socklen_t write_len = static_cast(sizeof(sockaddr_in)); + if (*addr_len >= write_len) { + *addr_len = write_len; + auto addr4 = reinterpret_cast(addr); + addr4->sin_family = AF_INET; + const auto sin_addr_bytes = m_fuzzed_data_provider.ConsumeBytes(sizeof(addr4->sin_addr)); + memcpy(&addr4->sin_addr, sin_addr_bytes.data(), sin_addr_bytes.size()); + addr4->sin_port = m_fuzzed_data_provider.ConsumeIntegralInRange(1, 65535); + } + } else { + // IPv6 + const socklen_t write_len = static_cast(sizeof(sockaddr_in6)); + if (*addr_len >= write_len) { + *addr_len = write_len; + auto addr6 = reinterpret_cast(addr); + addr6->sin6_family = AF_INET6; + const auto sin_addr_bytes = m_fuzzed_data_provider.ConsumeBytes(sizeof(addr6->sin6_addr)); + memcpy(&addr6->sin6_addr, sin_addr_bytes.data(), sin_addr_bytes.size()); + addr6->sin6_port = m_fuzzed_data_provider.ConsumeIntegralInRange(1, 65535); + } + } + } return std::make_unique(m_fuzzed_data_provider); } From e6a917c8f8e0f1a0fa71dc9bbb6e1074f81edea3 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 9 Jun 2025 13:29:19 +0200 Subject: [PATCH 2/7] fuzz: add Fuzzed NetEventsInterface and use it in connman tests --- src/test/fuzz/connman.cpp | 2 ++ src/test/fuzz/util/net.h | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index a62d227da8e..8513f07fead 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -51,6 +51,7 @@ FUZZ_TARGET(connman, .init = initialize_connman) } } AddrManDeterministic& addr_man{*addr_man_ptr}; + auto net_events{ConsumeNetEvents(fuzzed_data_provider)}; ConnmanTestMsg connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addr_man, @@ -60,6 +61,7 @@ FUZZ_TARGET(connman, .init = initialize_connman) const uint64_t max_outbound_limit{fuzzed_data_provider.ConsumeIntegral()}; CConnman::Options options; + options.m_msgproc = &net_events; options.nMaxOutboundLimit = max_outbound_limit; connman.Init(options); diff --git a/src/test/fuzz/util/net.h b/src/test/fuzz/util/net.h index 698001a7f15..200638441ee 100644 --- a/src/test/fuzz/util/net.h +++ b/src/test/fuzz/util/net.h @@ -139,6 +139,25 @@ public: } }; +class FuzzedNetEvents : public NetEventsInterface +{ +public: + FuzzedNetEvents(FuzzedDataProvider& fdp) : m_fdp(fdp) {} + + virtual void InitializeNode(const CNode&, ServiceFlags) override {} + + virtual void FinalizeNode(const CNode&) override {} + + virtual bool HasAllDesirableServiceFlags(ServiceFlags) const override { return m_fdp.ConsumeBool(); } + + virtual bool ProcessMessages(CNode*, std::atomic&) override { return m_fdp.ConsumeBool(); } + + virtual bool SendMessages(CNode*) override { return m_fdp.ConsumeBool(); } + +private: + FuzzedDataProvider& m_fdp; +}; + class FuzzedSock : public Sock { FuzzedDataProvider& m_fuzzed_data_provider; @@ -203,6 +222,11 @@ public: bool IsConnected(std::string& errmsg) const override; }; +[[nodiscard]] inline FuzzedNetEvents ConsumeNetEvents(FuzzedDataProvider& fdp) noexcept +{ + return FuzzedNetEvents{fdp}; +} + [[nodiscard]] inline FuzzedSock ConsumeSock(FuzzedDataProvider& fuzzed_data_provider) { return FuzzedSock{fuzzed_data_provider}; From 50da7432ec1e5431b243aa30f8a9339f8e8ed97d Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 15 Apr 2021 10:40:45 +0200 Subject: [PATCH 3/7] fuzz: add CConnman::OpenNetworkConnection() to the tests Now that all network calls done by `CConnman::OpenNetworkConnection()` are done via `Sock` they can be redirected (mocked) to `FuzzedSocket` for testing. --- src/test/fuzz/connman.cpp | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 8513f07fead..e65cf5ce58b 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -52,6 +53,19 @@ FUZZ_TARGET(connman, .init = initialize_connman) } AddrManDeterministic& addr_man{*addr_man_ptr}; auto net_events{ConsumeNetEvents(fuzzed_data_provider)}; + + // Mock CreateSock() to create FuzzedSock. + auto CreateSockOrig = CreateSock; + CreateSock = [&fuzzed_data_provider](int, int, int) { + return std::make_unique(fuzzed_data_provider); + }; + + // Mock g_dns_lookup() to return a fuzzed address. + auto g_dns_lookup_orig = g_dns_lookup; + g_dns_lookup = [&fuzzed_data_provider](const std::string&, bool) { + return std::vector{ConsumeNetAddr(fuzzed_data_provider)}; + }; + ConnmanTestMsg connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addr_man, @@ -66,6 +80,7 @@ FUZZ_TARGET(connman, .init = initialize_connman) connman.Init(options); CNetAddr random_netaddr; + CAddress random_address; CNode random_node = ConsumeNode(fuzzed_data_provider); CSubNet random_subnet; std::string random_string; @@ -81,6 +96,9 @@ FUZZ_TARGET(connman, .init = initialize_connman) [&] { random_netaddr = ConsumeNetAddr(fuzzed_data_provider); }, + [&] { + random_address = ConsumeAddress(fuzzed_data_provider); + }, [&] { random_subnet = ConsumeSubNet(fuzzed_data_provider); }, @@ -145,6 +163,21 @@ FUZZ_TARGET(connman, .init = initialize_connman) }, [&] { connman.SetTryNewOutboundPeer(fuzzed_data_provider.ConsumeBool()); + }, + [&] { + ConnectionType conn_type{ + fuzzed_data_provider.PickValueInArray(ALL_CONNECTION_TYPES)}; + if (conn_type == ConnectionType::INBOUND) { // INBOUND is not allowed + conn_type = ConnectionType::OUTBOUND_FULL_RELAY; + } + + connman.OpenNetworkConnection( + /*addrConnect=*/random_address, + /*fCountFailure=*/fuzzed_data_provider.ConsumeBool(), + /*grant_outbound=*/{}, + /*strDest=*/fuzzed_data_provider.ConsumeBool() ? nullptr : random_string.c_str(), + /*conn_type=*/conn_type, + /*use_v2transport=*/fuzzed_data_provider.ConsumeBool()); }); } (void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool()); @@ -164,4 +197,6 @@ FUZZ_TARGET(connman, .init = initialize_connman) (void)connman.ASMapHealthCheck(); connman.ClearTestNodes(); + g_dns_lookup = g_dns_lookup_orig; + CreateSock = CreateSockOrig; } From 91cbf4dbd864b65ba6b107957f087d1d305914b2 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 15 Apr 2021 14:01:52 +0200 Subject: [PATCH 4/7] fuzz: add CConnman::CreateNodeFromAcceptedSocket() to the tests --- src/test/fuzz/connman.cpp | 9 +++++++++ src/test/util/net.h | 8 ++++++++ 2 files changed, 17 insertions(+) diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index e65cf5ce58b..ea7ad3c4574 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -178,6 +178,15 @@ FUZZ_TARGET(connman, .init = initialize_connman) /*strDest=*/fuzzed_data_provider.ConsumeBool() ? nullptr : random_string.c_str(), /*conn_type=*/conn_type, /*use_v2transport=*/fuzzed_data_provider.ConsumeBool()); + }, + [&] { + connman.SetNetworkActive(fuzzed_data_provider.ConsumeBool()); + const auto peer = ConsumeAddress(fuzzed_data_provider); + connman.CreateNodeFromAcceptedSocketPublic( + /*sock=*/CreateSock(AF_INET, SOCK_STREAM, IPPROTO_TCP), + /*permissions=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS), + /*addr_bind=*/ConsumeAddress(fuzzed_data_provider), + /*addr_peer=*/peer); }); } (void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool()); diff --git a/src/test/util/net.h b/src/test/util/net.h index 6bf2bf73007..19264ca5570 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -68,6 +68,14 @@ struct ConnmanTestMsg : public CConnman { m_nodes.clear(); } + void CreateNodeFromAcceptedSocketPublic(std::unique_ptr sock, + NetPermissionFlags permissions, + const CAddress& addr_bind, + const CAddress& addr_peer) + { + CreateNodeFromAcceptedSocket(std::move(sock), permissions, addr_bind, addr_peer); + } + void Handshake(CNode& node, bool successfully_connected, ServiceFlags remote_services, From 3265df63a48db187e0d240ce801ee573787fed80 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 15 Apr 2021 16:51:00 +0200 Subject: [PATCH 5/7] fuzz: add CConnman::InitBinds() to the tests --- src/test/fuzz/connman.cpp | 19 +++++++++++++++++++ src/test/fuzz/util/net.h | 12 ++++++++++++ src/test/util/net.h | 5 +++++ 3 files changed, 36 insertions(+) diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index ea7ad3c4574..461362afe37 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -187,6 +187,25 @@ FUZZ_TARGET(connman, .init = initialize_connman) /*permissions=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS), /*addr_bind=*/ConsumeAddress(fuzzed_data_provider), /*addr_peer=*/peer); + }, + [&] { + CConnman::Options options; + + options.vBinds = ConsumeServiceVector(fuzzed_data_provider); + + options.vWhiteBinds = std::vector{ + fuzzed_data_provider.ConsumeIntegralInRange(0, 5)}; + for (auto& wb : options.vWhiteBinds) { + wb.m_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS); + wb.m_service = ConsumeService(fuzzed_data_provider); + } + + options.onion_binds = ConsumeServiceVector(fuzzed_data_provider); + + options.bind_on_any = options.vBinds.empty() && options.vWhiteBinds.empty() && + options.onion_binds.empty(); + + connman.InitBindsPublic(options); }); } (void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool()); diff --git a/src/test/fuzz/util/net.h b/src/test/fuzz/util/net.h index 200638441ee..2756e49cba6 100644 --- a/src/test/fuzz/util/net.h +++ b/src/test/fuzz/util/net.h @@ -249,6 +249,18 @@ inline CService ConsumeService(FuzzedDataProvider& fuzzed_data_provider) noexcep return {ConsumeNetAddr(fuzzed_data_provider), fuzzed_data_provider.ConsumeIntegral()}; } +inline std::vector ConsumeServiceVector(FuzzedDataProvider& fuzzed_data_provider, + size_t max_vector_size = 5) noexcept +{ + std::vector ret; + const size_t size = fuzzed_data_provider.ConsumeIntegralInRange(0, max_vector_size); + ret.reserve(size); + for (size_t i = 0; i < size; ++i) { + ret.emplace_back(ConsumeService(fuzzed_data_provider)); + } + return ret; +} + CAddress ConsumeAddress(FuzzedDataProvider& fuzzed_data_provider) noexcept; template diff --git a/src/test/util/net.h b/src/test/util/net.h index 19264ca5570..26e5c028d89 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -76,6 +76,11 @@ struct ConnmanTestMsg : public CConnman { CreateNodeFromAcceptedSocket(std::move(sock), permissions, addr_bind, addr_peer); } + bool InitBindsPublic(const CConnman::Options& options) + { + return InitBinds(options); + } + void Handshake(CNode& node, bool successfully_connected, ServiceFlags remote_services, From 6d9e5d130d2e1d052044e9a72d44cfffb5d3c771 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 30 Apr 2021 17:07:35 +0200 Subject: [PATCH 6/7] fuzz: add CConnman::SocketHandler() to the tests --- src/test/fuzz/connman.cpp | 3 +++ src/test/util/net.h | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 461362afe37..ddb56fd804c 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -206,6 +206,9 @@ FUZZ_TARGET(connman, .init = initialize_connman) options.onion_binds.empty(); connman.InitBindsPublic(options); + }, + [&] { + connman.SocketHandlerPublic(); }); } (void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool()); diff --git a/src/test/util/net.h b/src/test/util/net.h index 26e5c028d89..66d209aed65 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -81,6 +81,11 @@ struct ConnmanTestMsg : public CConnman { return InitBinds(options); } + void SocketHandlerPublic() + { + SocketHandler(); + } + void Handshake(CNode& node, bool successfully_connected, ServiceFlags remote_services, From 0802398e749c5e16fa7085cd87c91a31bbe043bd Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Wed, 6 Nov 2024 11:12:35 +0100 Subject: [PATCH 7/7] fuzz: make it possible to mock (fuzz) CThreadInterrupt * Make the methods of `CThreadInterrupt` virtual and store a pointer to it in `CConnman`, thus making it possible to override with a mocked instance. * Initialize `CConnman::m_interrupt_net` from the constructor, making it possible for callers to supply mocked version. * Introduce `FuzzedThreadInterrupt` and `ConsumeThreadInterrupt()` and use them in `src/test/fuzz/connman.cpp` and `src/test/fuzz/i2p.cpp`. This improves the CPU utilization of the `connman` fuzz test. As a nice side effect, the `std::shared_ptr` used for `CConnman::m_interrupt_net` resolves the possible lifetime issues with it (see the removed comment for that variable). --- src/i2p.cpp | 8 +-- src/i2p.h | 14 ++---- src/net.cpp | 70 +++++++++++++++----------- src/net.h | 15 +++--- src/test/fuzz/connman.cpp | 4 +- src/test/fuzz/i2p.cpp | 9 ++-- src/test/fuzz/util/CMakeLists.txt | 1 + src/test/fuzz/util/threadinterrupt.cpp | 22 ++++++++ src/test/fuzz/util/threadinterrupt.h | 33 ++++++++++++ src/test/i2p_tests.cpp | 12 ++--- src/util/threadinterrupt.cpp | 7 ++- src/util/threadinterrupt.h | 24 +++++++-- 12 files changed, 154 insertions(+), 65 deletions(-) create mode 100644 src/test/fuzz/util/threadinterrupt.cpp create mode 100644 src/test/fuzz/util/threadinterrupt.h diff --git a/src/i2p.cpp b/src/i2p.cpp index 8b48615afeb..80f3bde4cf2 100644 --- a/src/i2p.cpp +++ b/src/i2p.cpp @@ -119,7 +119,7 @@ namespace sam { Session::Session(const fs::path& private_key_file, const Proxy& control_host, - CThreadInterrupt* interrupt) + std::shared_ptr interrupt) : m_private_key_file{private_key_file}, m_control_host{control_host}, m_interrupt{interrupt}, @@ -127,7 +127,7 @@ Session::Session(const fs::path& private_key_file, { } -Session::Session(const Proxy& control_host, CThreadInterrupt* interrupt) +Session::Session(const Proxy& control_host, std::shared_ptr interrupt) : m_control_host{control_host}, m_interrupt{interrupt}, m_transient{true} @@ -162,7 +162,7 @@ bool Session::Accept(Connection& conn) std::string errmsg; bool disconnect{false}; - while (!*m_interrupt) { + while (!m_interrupt->interrupted()) { Sock::Event occurred; if (!conn.sock->Wait(MAX_WAIT_FOR_IO, Sock::RECV, &occurred)) { errmsg = "wait on socket failed"; @@ -205,7 +205,7 @@ bool Session::Accept(Connection& conn) return true; } - if (*m_interrupt) { + if (m_interrupt->interrupted()) { LogPrintLevel(BCLog::I2P, BCLog::Level::Debug, "Accept was interrupted\n"); } else { LogPrintLevel(BCLog::I2P, BCLog::Level::Debug, "Error accepting%s: %s\n", disconnect ? " (will close the session)" : "", errmsg); diff --git a/src/i2p.h b/src/i2p.h index 153263399df..a41406f380f 100644 --- a/src/i2p.h +++ b/src/i2p.h @@ -63,13 +63,11 @@ public: * private key will be generated and saved into the file. * @param[in] control_host Location of the SAM proxy. * @param[in,out] interrupt If this is signaled then all operations are canceled as soon as - * possible and executing methods throw an exception. Notice: only a pointer to the - * `CThreadInterrupt` object is saved, so it must not be destroyed earlier than this - * `Session` object. + * possible and executing methods throw an exception. */ Session(const fs::path& private_key_file, const Proxy& control_host, - CThreadInterrupt* interrupt); + std::shared_ptr interrupt); /** * Construct a transient session which will generate its own I2P private key @@ -78,11 +76,9 @@ public: * the session will be lazily created later when first used. * @param[in] control_host Location of the SAM proxy. * @param[in,out] interrupt If this is signaled then all operations are canceled as soon as - * possible and executing methods throw an exception. Notice: only a pointer to the - * `CThreadInterrupt` object is saved, so it must not be destroyed earlier than this - * `Session` object. + * possible and executing methods throw an exception. */ - Session(const Proxy& control_host, CThreadInterrupt* interrupt); + Session(const Proxy& control_host, std::shared_ptr interrupt); /** * Destroy the session, closing the internally used sockets. The sockets that have been @@ -235,7 +231,7 @@ private: /** * Cease network activity when this is signaled. */ - CThreadInterrupt* const m_interrupt; + const std::shared_ptr m_interrupt; /** * Mutex protecting the members that can be concurrently accessed. diff --git a/src/net.cpp b/src/net.cpp index 217d9a89037..85c25543d38 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -471,7 +471,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo LOCK(m_unused_i2p_sessions_mutex); if (m_unused_i2p_sessions.empty()) { i2p_transient_session = - std::make_unique(proxy, &interruptNet); + std::make_unique(proxy, m_interrupt_net); } else { i2p_transient_session.swap(m_unused_i2p_sessions.front()); m_unused_i2p_sessions.pop(); @@ -2094,7 +2094,7 @@ void CConnman::SocketHandler() // empty sets. events_per_sock = GenerateWaitSockets(snap.Nodes()); if (events_per_sock.empty() || !events_per_sock.begin()->first->WaitMany(timeout, events_per_sock)) { - interruptNet.sleep_for(timeout); + m_interrupt_net->sleep_for(timeout); } // Service (send/receive) each of the already connected nodes. @@ -2111,8 +2111,9 @@ void CConnman::SocketHandlerConnected(const std::vector& nodes, AssertLockNotHeld(m_total_bytes_sent_mutex); for (CNode* pnode : nodes) { - if (interruptNet) + if (m_interrupt_net->interrupted()) { return; + } // // Receive @@ -2207,7 +2208,7 @@ void CConnman::SocketHandlerConnected(const std::vector& nodes, void CConnman::SocketHandlerListening(const Sock::EventsPerSock& events_per_sock) { for (const ListenSocket& listen_socket : vhListenSocket) { - if (interruptNet) { + if (m_interrupt_net->interrupted()) { return; } const auto it = events_per_sock.find(listen_socket.sock); @@ -2221,8 +2222,7 @@ void CConnman::ThreadSocketHandler() { AssertLockNotHeld(m_total_bytes_sent_mutex); - while (!interruptNet) - { + while (!m_interrupt_net->interrupted()) { DisconnectNodes(); NotifyNumConnectionsChanged(); SocketHandler(); @@ -2246,9 +2246,10 @@ void CConnman::ThreadDNSAddressSeed() auto start = NodeClock::now(); constexpr std::chrono::seconds SEEDNODE_TIMEOUT = 30s; LogPrintf("-seednode enabled. Trying the provided seeds for %d seconds before defaulting to the dnsseeds.\n", SEEDNODE_TIMEOUT.count()); - while (!interruptNet) { - if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) + while (!m_interrupt_net->interrupted()) { + if (!m_interrupt_net->sleep_for(500ms)) { return; + } // Abort if we have spent enough time without reaching our target. // Giving seed nodes 30 seconds so this does not become a race against fixedseeds (which triggers after 1 min) @@ -2309,7 +2310,7 @@ void CConnman::ThreadDNSAddressSeed() // early to see if we have enough peers and can stop // this thread entirely freeing up its resources std::chrono::seconds w = std::min(DNSSEEDS_DELAY_FEW_PEERS, to_wait); - if (!interruptNet.sleep_for(w)) return; + if (!m_interrupt_net->sleep_for(w)) return; to_wait -= w; if (GetFullOutboundConnCount() >= SEED_OUTBOUND_CONNECTION_THRESHOLD) { @@ -2325,13 +2326,13 @@ void CConnman::ThreadDNSAddressSeed() } } - if (interruptNet) return; + if (m_interrupt_net->interrupted()) return; // hold off on querying seeds if P2P network deactivated if (!fNetworkActive) { LogPrintf("Waiting for network to be reactivated before querying DNS seeds.\n"); do { - if (!interruptNet.sleep_for(std::chrono::seconds{1})) return; + if (!m_interrupt_net->sleep_for(1s)) return; } while (!fNetworkActive); } @@ -2526,12 +2527,14 @@ void CConnman::ThreadOpenConnections(const std::vector connect, std OpenNetworkConnection(addr, false, {}, strAddr.c_str(), ConnectionType::MANUAL, /*use_v2transport=*/use_v2transport); for (int i = 0; i < 10 && i < nLoop; i++) { - if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) + if (!m_interrupt_net->sleep_for(500ms)) { return; + } } } - if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) + if (!m_interrupt_net->sleep_for(500ms)) { return; + } PerformReconnections(); } } @@ -2555,8 +2558,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, std LogPrintf("Fixed seeds are disabled\n"); } - while (!interruptNet) - { + while (!m_interrupt_net->interrupted()) { if (add_addr_fetch) { add_addr_fetch = false; const auto& seed{SpanPopBack(seed_nodes)}; @@ -2571,14 +2573,16 @@ void CConnman::ThreadOpenConnections(const std::vector connect, std ProcessAddrFetch(); - if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) + if (!m_interrupt_net->sleep_for(500ms)) { return; + } PerformReconnections(); CountingSemaphoreGrant<> grant(*semOutbound); - if (interruptNet) + if (m_interrupt_net->interrupted()) { return; + } const std::unordered_set fixed_seed_networks{GetReachableEmptyNetworks()}; if (add_fixed_seeds && !fixed_seed_networks.empty()) { @@ -2752,8 +2756,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, std int nTries = 0; const auto reachable_nets{g_reachable_nets.All()}; - while (!interruptNet) - { + while (!m_interrupt_net->interrupted()) { if (anchor && !m_anchors.empty()) { const CAddress addr = m_anchors.back(); m_anchors.pop_back(); @@ -2855,7 +2858,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, std if (addrConnect.IsValid()) { if (fFeeler) { // Add small amount of random noise before connection to avoid synchronization. - if (!interruptNet.sleep_for(rng.rand_uniform_duration(FEELER_SLEEP_WINDOW))) { + if (!m_interrupt_net->sleep_for(rng.rand_uniform_duration(FEELER_SLEEP_WINDOW))) { return; } LogDebug(BCLog::NET, "Making feeler connection to %s\n", addrConnect.ToStringAddrPort()); @@ -2966,14 +2969,15 @@ void CConnman::ThreadOpenAddedConnections() tried = true; CAddress addr(CService(), NODE_NONE); OpenNetworkConnection(addr, false, std::move(grant), info.m_params.m_added_node.c_str(), ConnectionType::MANUAL, info.m_params.m_use_v2transport); - if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) return; + if (!m_interrupt_net->sleep_for(500ms)) return; grant = CountingSemaphoreGrant<>(*semAddnode, /*fTry=*/true); } // See if any reconnections are desired. PerformReconnections(); // Retry every 60 seconds if a connection was attempted, otherwise two seconds - if (!interruptNet.sleep_for(std::chrono::seconds(tried ? 60 : 2))) + if (!m_interrupt_net->sleep_for(tried ? 60s : 2s)) { return; + } } } @@ -2986,7 +2990,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai // // Initiate outbound network connection // - if (interruptNet) { + if (m_interrupt_net->interrupted()) { return; } if (!fNetworkActive) { @@ -3074,13 +3078,13 @@ void CConnman::ThreadI2PAcceptIncoming() i2p::Connection conn; auto SleepOnFailure = [&]() { - interruptNet.sleep_for(err_wait); + m_interrupt_net->sleep_for(err_wait); if (err_wait < err_wait_cap) { err_wait += 1s; } }; - while (!interruptNet) { + while (!m_interrupt_net->interrupted()) { if (!m_i2p_sam_session->Listen(conn)) { if (advertising_listen_addr && conn.me.IsValid()) { @@ -3202,12 +3206,18 @@ void CConnman::SetNetworkActive(bool active) } } -CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, AddrMan& addrman_in, - const NetGroupManager& netgroupman, const CChainParams& params, bool network_active) +CConnman::CConnman(uint64_t nSeed0In, + uint64_t nSeed1In, + AddrMan& addrman_in, + const NetGroupManager& netgroupman, + const CChainParams& params, + bool network_active, + std::shared_ptr interrupt_net) : addrman(addrman_in) , m_netgroupman{netgroupman} , nSeed0(nSeed0In) , nSeed1(nSeed1In) + , m_interrupt_net{interrupt_net} , m_params(params) { SetTryNewOutboundPeer(false); @@ -3303,7 +3313,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) Proxy i2p_sam; if (GetProxy(NET_I2P, i2p_sam) && connOptions.m_i2p_accept_incoming) { m_i2p_sam_session = std::make_unique(gArgs.GetDataDirNet() / "i2p_private_key", - i2p_sam, &interruptNet); + i2p_sam, m_interrupt_net); } // Randomize the order in which we may query seednode to potentially prevent connecting to the same one every restart (and signal that we have restarted) @@ -3340,7 +3350,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) // Start threads // assert(m_msgproc); - interruptNet.reset(); + m_interrupt_net->reset(); flagInterruptMsgProc = false; { @@ -3416,7 +3426,7 @@ void CConnman::Interrupt() } condMsgProc.notify_all(); - interruptNet(); + (*m_interrupt_net)(); g_socks5_interrupt(); if (semOutbound) { diff --git a/src/net.h b/src/net.h index 4cb4cb906e2..66a830da905 100644 --- a/src/net.h +++ b/src/net.h @@ -1118,8 +1118,13 @@ public: whitelist_relay = connOptions.whitelist_relay; } - CConnman(uint64_t seed0, uint64_t seed1, AddrMan& addrman, const NetGroupManager& netgroupman, - const CChainParams& params, bool network_active = true); + CConnman(uint64_t seed0, + uint64_t seed1, + AddrMan& addrman, + const NetGroupManager& netgroupman, + const CChainParams& params, + bool network_active = true, + std::shared_ptr interrupt_net = std::make_shared()); ~CConnman(); @@ -1543,11 +1548,9 @@ private: /** * This is signaled when network activity should cease. - * A pointer to it is saved in `m_i2p_sam_session`, so make sure that - * the lifetime of `interruptNet` is not shorter than - * the lifetime of `m_i2p_sam_session`. + * A copy of this is saved in `m_i2p_sam_session`. */ - CThreadInterrupt interruptNet; + const std::shared_ptr m_interrupt_net; /** * I2P SAM session. diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index ddb56fd804c..56034185b36 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -71,7 +72,8 @@ FUZZ_TARGET(connman, .init = initialize_connman) addr_man, netgroupman, Params(), - fuzzed_data_provider.ConsumeBool()}; + fuzzed_data_provider.ConsumeBool(), + ConsumeThreadInterrupt(fuzzed_data_provider)}; const uint64_t max_outbound_limit{fuzzed_data_provider.ConsumeIntegral()}; CConnman::Options options; diff --git a/src/test/fuzz/i2p.cpp b/src/test/fuzz/i2p.cpp index 29a11123d9a..64f30719c58 100644 --- a/src/test/fuzz/i2p.cpp +++ b/src/test/fuzz/i2p.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -35,15 +36,15 @@ FUZZ_TARGET(i2p, .init = initialize_i2p) const fs::path private_key_path = gArgs.GetDataDirNet() / "fuzzed_i2p_private_key"; const CService addr{in6_addr(IN6ADDR_LOOPBACK_INIT), 7656}; const Proxy sam_proxy{addr, /*tor_stream_isolation=*/false}; - CThreadInterrupt interrupt; + auto interrupt{ConsumeThreadInterrupt(fuzzed_data_provider)}; - i2p::sam::Session session{private_key_path, sam_proxy, &interrupt}; + i2p::sam::Session session{private_key_path, sam_proxy, interrupt}; i2p::Connection conn; if (session.Listen(conn)) { if (session.Accept(conn)) { try { - (void)conn.sock->RecvUntilTerminator('\n', 10ms, interrupt, i2p::sam::MAX_MSG_SIZE); + (void)conn.sock->RecvUntilTerminator('\n', 10ms, *interrupt, i2p::sam::MAX_MSG_SIZE); } catch (const std::runtime_error&) { } } @@ -53,7 +54,7 @@ FUZZ_TARGET(i2p, .init = initialize_i2p) if (session.Connect(CService{}, conn, proxy_error)) { try { - conn.sock->SendComplete("verack\n", 10ms, interrupt); + conn.sock->SendComplete("verack\n", 10ms, *interrupt); } catch (const std::runtime_error&) { } } diff --git a/src/test/fuzz/util/CMakeLists.txt b/src/test/fuzz/util/CMakeLists.txt index 878286b0f44..282de3d708f 100644 --- a/src/test/fuzz/util/CMakeLists.txt +++ b/src/test/fuzz/util/CMakeLists.txt @@ -7,6 +7,7 @@ add_library(test_fuzz STATIC EXCLUDE_FROM_ALL descriptor.cpp mempool.cpp net.cpp + threadinterrupt.cpp ../fuzz.cpp ../util.cpp ) diff --git a/src/test/fuzz/util/threadinterrupt.cpp b/src/test/fuzz/util/threadinterrupt.cpp new file mode 100644 index 00000000000..5dd87e05888 --- /dev/null +++ b/src/test/fuzz/util/threadinterrupt.cpp @@ -0,0 +1,22 @@ +// Copyright (c) 2024-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include + +FuzzedThreadInterrupt::FuzzedThreadInterrupt(FuzzedDataProvider& fuzzed_data_provider) + : m_fuzzed_data_provider{fuzzed_data_provider} +{ +} + +bool FuzzedThreadInterrupt::interrupted() const +{ + return m_fuzzed_data_provider.ConsumeBool(); +} + +bool FuzzedThreadInterrupt::sleep_for(Clock::duration) +{ + SetMockTime(ConsumeTime(m_fuzzed_data_provider)); // Time could go backwards. + return m_fuzzed_data_provider.ConsumeBool(); +} diff --git a/src/test/fuzz/util/threadinterrupt.h b/src/test/fuzz/util/threadinterrupt.h new file mode 100644 index 00000000000..d56aefd919f --- /dev/null +++ b/src/test/fuzz/util/threadinterrupt.h @@ -0,0 +1,33 @@ +// Copyright (c) 2024-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_TEST_FUZZ_UTIL_THREADINTERRUPT_H +#define BITCOIN_TEST_FUZZ_UTIL_THREADINTERRUPT_H + +#include +#include + +#include + +/** + * Mocked CThreadInterrupt that returns "randomly" whether it is interrupted and never sleeps. + */ +class FuzzedThreadInterrupt : public CThreadInterrupt +{ +public: + explicit FuzzedThreadInterrupt(FuzzedDataProvider& fuzzed_data_provider); + + virtual bool interrupted() const override; + virtual bool sleep_for(Clock::duration) override; + +private: + FuzzedDataProvider& m_fuzzed_data_provider; +}; + +[[nodiscard]] inline std::shared_ptr ConsumeThreadInterrupt(FuzzedDataProvider& fuzzed_data_provider) +{ + return std::make_shared(fuzzed_data_provider); +} + +#endif // BITCOIN_TEST_FUZZ_UTIL_THREADINTERRUPT_H diff --git a/src/test/i2p_tests.cpp b/src/test/i2p_tests.cpp index bdb3408b66c..da9cbb5d228 100644 --- a/src/test/i2p_tests.cpp +++ b/src/test/i2p_tests.cpp @@ -50,10 +50,10 @@ BOOST_AUTO_TEST_CASE(unlimited_recv) return std::make_unique(std::string(i2p::sam::MAX_MSG_SIZE + 1, 'a')); }; - CThreadInterrupt interrupt; + auto interrupt{std::make_shared()}; const std::optional addr{Lookup("127.0.0.1", 9000, false)}; const Proxy sam_proxy(addr.value(), /*tor_stream_isolation=*/false); - i2p::sam::Session session(gArgs.GetDataDirNet() / "test_i2p_private_key", sam_proxy, &interrupt); + i2p::sam::Session session(gArgs.GetDataDirNet() / "test_i2p_private_key", sam_proxy, interrupt); { ASSERT_DEBUG_LOG("Creating persistent SAM session"); @@ -112,12 +112,12 @@ BOOST_AUTO_TEST_CASE(listen_ok_accept_fail) // clang-format on }; - CThreadInterrupt interrupt; + auto interrupt{std::make_shared()}; const CService addr{in6_addr(IN6ADDR_LOOPBACK_INIT), /*port=*/7656}; const Proxy sam_proxy(addr, /*tor_stream_isolation=*/false); i2p::sam::Session session(gArgs.GetDataDirNet() / "test_i2p_private_key", sam_proxy, - &interrupt); + interrupt); i2p::Connection conn; for (size_t i = 0; i < 5; ++i) { @@ -155,10 +155,10 @@ BOOST_AUTO_TEST_CASE(damaged_private_key) "391 bytes"}}) { BOOST_REQUIRE(WriteBinaryFile(i2p_private_key_file, file_contents)); - CThreadInterrupt interrupt; + auto interrupt{std::make_shared()}; const CService addr{in6_addr(IN6ADDR_LOOPBACK_INIT), /*port=*/7656}; const Proxy sam_proxy{addr, /*tor_stream_isolation=*/false}; - i2p::sam::Session session(i2p_private_key_file, sam_proxy, &interrupt); + i2p::sam::Session session(i2p_private_key_file, sam_proxy, interrupt); { ASSERT_DEBUG_LOG("Creating persistent SAM session"); diff --git a/src/util/threadinterrupt.cpp b/src/util/threadinterrupt.cpp index 3ea406d4a8e..aaa9e831a91 100644 --- a/src/util/threadinterrupt.cpp +++ b/src/util/threadinterrupt.cpp @@ -9,11 +9,16 @@ CThreadInterrupt::CThreadInterrupt() : flag(false) {} -CThreadInterrupt::operator bool() const +bool CThreadInterrupt::interrupted() const { return flag.load(std::memory_order_acquire); } +CThreadInterrupt::operator bool() const +{ + return interrupted(); +} + void CThreadInterrupt::reset() { flag.store(false, std::memory_order_release); diff --git a/src/util/threadinterrupt.h b/src/util/threadinterrupt.h index 0b79b382763..8b393c26dfc 100644 --- a/src/util/threadinterrupt.h +++ b/src/util/threadinterrupt.h @@ -27,11 +27,27 @@ class CThreadInterrupt { public: using Clock = std::chrono::steady_clock; + CThreadInterrupt(); - explicit operator bool() const; - void operator()() EXCLUSIVE_LOCKS_REQUIRED(!mut); - void reset(); - bool sleep_for(Clock::duration rel_time) EXCLUSIVE_LOCKS_REQUIRED(!mut); + + virtual ~CThreadInterrupt() = default; + + /// Return true if `operator()()` has been called. + virtual bool interrupted() const; + + /// An alias for `interrupted()`. + virtual explicit operator bool() const; + + /// Interrupt any sleeps. After this `interrupted()` will return `true`. + virtual void operator()() EXCLUSIVE_LOCKS_REQUIRED(!mut); + + /// Reset to an non-interrupted state. + virtual void reset(); + + /// Sleep for the given duration. + /// @retval true The time passed. + /// @retval false The sleep was interrupted. + virtual bool sleep_for(Clock::duration rel_time) EXCLUSIVE_LOCKS_REQUIRED(!mut); private: std::condition_variable cond;