Public commit record
60/100 · Adequate
What the developer wrote
Authored by TheCharlatan
net: Add interrupt to pcp retry loop
Without this interrupt bitcoind takes a long time to exit if requested
to do so after a failed pcp lookup on startup.
✓ Descriptive subject✓ Names a concrete action or component✓ Provides an explanatory body
What changed, and why it matters
This change makes Bitcoin Core shut down faster when a network port-mapping feature (PCP/NAT-PMP) fails during startup. Previously, the retry loop kept waiting for responses even after the user asked the program to quit, causing a long delay. There is no security vulnerability being fixed here—only a responsiveness/usability improvement.
Recommended action
No security action required. Treat as normal code-quality/responsiveness improvement. Reviewers may verify that g_mapport_interrupt is set reliably during shutdown and that tests still pass.
Security signals we found
01
No memory corruption, injection, or authentication issue visible in diff
02
Change is a responsiveness/usability fix for shutdown path
03
Retry loop now honors process interrupt signal
04
No bounds, parsing, or cryptographic changes
Evidence from the diff
The commit threads a CThreadInterrupt reference through PCPSendRecv, NATPMPRequestPortMap, and PCPRequestPortMap so the UDP send/receive retry loop can exit early when g_mapport_interrupt is set. This prevents bitcoind from blocking on repeated socket waits during shutdown after a failed PCP/NAT-PMP lookup at startup. The change is purely about shutdown responsiveness and denial-of-service self-mitigation, not about memory safety, authentication, or remote exploitation.
Changed components
src/common/pcp.cppsrc/common/pcp.hsrc/mapport.cppsrc/test/fuzz/pcp.cppsrc/test/pcp_tests.cppInspect captured patch +36 / −24
diff --git a/src/common/pcp.cpp b/src/common/pcp.cpp
index 8ababab3..869a1621 100644
--- a/src/common/pcp.cpp
+++ b/src/common/pcp.cpp
@@ -15,6 +15,7 @@
#include <util/readwritefile.h>
#include <util/sock.h>
#include <util/strencodings.h>
+#include <util/threadinterrupt.h>
namespace {
@@ -217,7 +218,8 @@ CNetAddr PCPUnwrapAddress(std::span<const uint8_t> wrapped_addr)
//! PCP or NAT-PMP send-receive loop.
std::optional<std::vector<uint8_t>> PCPSendRecv(Sock &sock, const std::string &protocol, std::span<const uint8_t> request, int num_tries,
std::chrono::milliseconds timeout_per_try,
- std::function<bool(std::span<const uint8_t>)> check_packet)
+ std::function<bool(std::span<const uint8_t>)> check_packet,
+ CThreadInterrupt& interrupt)
{
using namespace std::chrono;
// UDP is a potentially lossy protocol, so we try to send again a few times.
@@ -238,6 +240,7 @@ std::optional<std::vector<uint8_t>> PCPSendRecv(Sock &sock, const std::string &p
auto cur_time = time_point_cast<milliseconds>(MockableSteadyClock::now());
auto deadline = cur_time + timeout_per_try;
while ((cur_time = time_point_cast<milliseconds>(MockableSteadyClock::now())) < deadline) {
+ if (interrupt) return std::nullopt;
Sock::Event occurred = 0;
if (!sock.Wait(deadline - cur_time, Sock::RECV, &occurred)) {
LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "%s: Could not wait on socket: %s\n", protocol, NetworkErrorString(WSAGetLastError()));
@@ -271,7 +274,7 @@ std::optional<std::vector<uint8_t>> PCPSendRecv(Sock &sock, const std::string &p
}
-std::variant<MappingResult, MappingError> NATPMPRequestPortMap(const CNetAddr &gateway, uint16_t port, uint32_t lifetime, int num_tries, std::chrono::milliseconds timeout_per_try)
+std::variant<MappingResult, MappingError> NATPMPRequestPortMap(const CNetAddr &gateway, uint16_t port, uint32_t lifetime, CThreadInterrupt& interrupt, int num_tries, std::chrono::milliseconds timeout_per_try)
{
struct sockaddr_storage dest_addr;
socklen_t dest_addrlen = sizeof(struct sockaddr_storage);
@@ -319,7 +322,8 @@ std::variant<MappingResult, MappingError> NATPMPRequestPortMap(const CNetAddr &g
return false; // Wasn't response to what we expected, try receiving next packet.
}
return true;
- });
+ },
+ interrupt);
struct in_addr external_addr;
if (recv_res) {
@@ -361,7 +365,8 @@ std::variant<MappingResult, MappingError> NATPMPRequestPortMap(const CNetAddr &g
return false; // Wasn't response to what we expected, try receiving next packet.
}
return true;
- });
+ },
+ interrupt);
if (recv_res) {
const std::span<uint8_t> response = *recv_res;
@@ -384,7 +389,7 @@ std::variant<MappingResult, MappingError> NATPMPRequestPortMap(const CNetAddr &g
}
}
-std::variant<MappingResult, MappingError> PCPRequestPortMap(const PCPMappingNonce &nonce, const CNetAddr &gateway, const CNetAddr &bind, uint16_t port, uint32_t lifetime, int num_tries, std::chrono::milliseconds timeout_per_try)
+std::variant<MappingResult, MappingError> PCPRequestPortMap(const PCPMappingNonce &nonce, const CNetAddr &gateway, const CNetAddr &bind, uint16_t port, uint32_t lifetime, CThreadInterrupt& interrupt, int num_tries, std::chrono::milliseconds timeout_per_try)
{
struct sockaddr_storage dest_addr, bind_addr;
socklen_t dest_addrlen = sizeof(struct sockaddr_storage), bind_addrlen = sizeof(struct sockaddr_storage);
@@ -484,7 +489,8 @@ std::variant<MappingResult, MappingError> PCPRequestPortMap(const PCPMappingNonc
return false; // Wasn't response to what we expected, try receiving next packet.
}
return true;
- });
+ },
+ interrupt);
if (!recv_res) {
return MappingError::NETWORK_ERROR;
diff --git a/src/common/pcp.h b/src/common/pcp.h
index 44f9285c..b3e36d13 100644
--- a/src/common/pcp.h
+++ b/src/common/pcp.h
@@ -6,6 +6,7 @@
#define BITCOIN_COMMON_PCP_H
#include <netaddress.h>
+#include <util/threadinterrupt.h>
#include <variant>
@@ -51,7 +52,7 @@ struct MappingResult {
//! * num_tries: Number of tries in case of no response.
//!
//! Returns the external_ip:external_port of the mapping if successful, otherwise a MappingError.
-std::variant<MappingResult, MappingError> NATPMPRequestPortMap(const CNetAddr &gateway, uint16_t port, uint32_t lifetime, int num_tries = 3, std::chrono::milliseconds timeout_per_try = std::chrono::milliseconds(1000));
+std::variant<MappingResult, MappingError> NATPMPRequestPortMap(const CNetAddr &gateway, uint16_t port, uint32_t lifetime, CThreadInterrupt& interrupt, int num_tries = 3, std::chrono::milliseconds timeout_per_try = std::chrono::milliseconds(1000));
//! Try to open a port using RFC 6887 Port Control Protocol (PCP). Handles IPv4 and IPv6.
//!
@@ -63,6 +64,6 @@ std::variant<MappingResult, MappingError> NATPMPRequestPortMap(const CNetAddr &g
//! * num_tries: Number of tries in case of no response.
//!
//! Returns the external_ip:external_port of the mapping if successful, otherwise a MappingError.
-std::variant<MappingResult, MappingError> PCPRequestPortMap(const PCPMappingNonce &nonce, const CNetAddr &gateway, const CNetAddr &bind, uint16_t port, uint32_t lifetime, int num_tries = 3, std::chrono::milliseconds timeout_per_try = std::chrono::milliseconds(1000));
+std::variant<MappingResult, MappingError> PCPRequestPortMap(const PCPMappingNonce &nonce, const CNetAddr &gateway, const CNetAddr &bind, uint16_t port, uint32_t lifetime, CThreadInterrupt& interrupt, int num_tries = 3, std::chrono::milliseconds timeout_per_try = std::chrono::milliseconds(1000));
#endif // BITCOIN_COMMON_PCP_H
diff --git a/src/mapport.cpp b/src/mapport.cpp
index 83105f51..976572c5 100644
--- a/src/mapport.cpp
+++ b/src/mapport.cpp
@@ -74,11 +74,11 @@ static void ProcessPCP()
// Open a port mapping on whatever local address we have toward the gateway.
struct in_addr inaddr_any;
inaddr_any.s_addr = htonl(INADDR_ANY);
- auto res = PCPRequestPortMap(pcp_nonce, *gateway4, CNetAddr(inaddr_any), private_port, requested_lifetime);
+ auto res = PCPRequestPortMap(pcp_nonce, *gateway4, CNetAddr(inaddr_any), private_port, requested_lifetime, g_mapport_interrupt);
MappingError* pcp_err = std::get_if<MappingError>(&res);
if (pcp_err && *pcp_err == MappingError::UNSUPP_VERSION) {
LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "portmap: Got unsupported PCP version response, falling back to NAT-PMP\n");
- res = NATPMPRequestPortMap(*gateway4, private_port, requested_lifetime);
+ res = NATPMPRequestPortMap(*gateway4, private_port, requested_lifetime, g_mapport_interrupt);
}
handle_mapping(res);
}
@@ -93,7 +93,7 @@ static void ProcessPCP()
// Try to open pinholes for all routable local IPv6 addresses.
for (const auto &addr: GetLocalAddresses()) {
if (!addr.IsRoutable() || !addr.IsIPv6()) continue;
- auto res = PCPRequestPortMap(pcp_nonce, *gateway6, addr, private_port, requested_lifetime);
+ auto res = PCPRequestPortMap(pcp_nonce, *gateway6, addr, private_port, requested_lifetime, g_mapport_interrupt);
handle_mapping(res);
}
}
diff --git a/src/test/fuzz/pcp.cpp b/src/test/fuzz/pcp.cpp
index 76fdded1..0beebe10 100644
--- a/src/test/fuzz/pcp.cpp
+++ b/src/test/fuzz/pcp.cpp
@@ -9,6 +9,7 @@
#include <common/pcp.h>
#include <util/check.h>
+#include <util/threadinterrupt.h>
using namespace std::literals;
@@ -43,7 +44,8 @@ FUZZ_TARGET(pcp_request_port_map, .init = port_map_target_init)
const auto local_addr{ConsumeNetAddr(fuzzed_data_provider)};
const auto port{fuzzed_data_provider.ConsumeIntegral<uint16_t>()};
const auto lifetime{fuzzed_data_provider.ConsumeIntegral<uint32_t>()};
- const auto res{PCPRequestPortMap(PCP_NONCE, gateway_addr, local_addr, port, lifetime, NUM_TRIES, TIMEOUT)};
+ CThreadInterrupt interrupt;
+ const auto res{PCPRequestPortMap(PCP_NONCE, gateway_addr, local_addr, port, lifetime, interrupt, NUM_TRIES, TIMEOUT)};
// In case of success the mapping must be consistent with the request.
if (const MappingResult* mapping = std::get_if<MappingResult>(&res)) {
@@ -69,7 +71,8 @@ FUZZ_TARGET(natpmp_request_port_map, .init = port_map_target_init)
const auto gateway_addr{ConsumeNetAddr(fuzzed_data_provider)};
const auto port{fuzzed_data_provider.ConsumeIntegral<uint16_t>()};
const auto lifetime{fuzzed_data_provider.ConsumeIntegral<uint32_t>()};
- const auto res{NATPMPRequestPortMap(gateway_addr, port, lifetime, NUM_TRIES, TIMEOUT)};
+ CThreadInterrupt interrupt;
+ const auto res{NATPMPRequestPortMap(gateway_addr, port, lifetime, interrupt, NUM_TRIES, TIMEOUT)};
// In case of success the mapping must be consistent with the request.
if (const MappingResult* mapping = std::get_if<MappingResult>(&res)) {
diff --git a/src/test/pcp_tests.cpp b/src/test/pcp_tests.cpp
index bb9a7fff..c71c9de1 100644
--- a/src/test/pcp_tests.cpp
+++ b/src/test/pcp_tests.cpp
@@ -15,6 +15,8 @@
using namespace std::literals;
+static CThreadInterrupt g_interrupt;
+
/// UDP test server operation.
struct TestOp {
std::chrono::milliseconds delay;
@@ -295,7 +297,7 @@ BOOST_AUTO_TEST_CASE(natpmp_ipv4)
return std::unique_ptr<PCPTestSock>();
};
- auto res = NATPMPRequestPortMap(default_gateway_ipv4, 1234, 1000, 1, 200ms);
+ auto res = NATPMPRequestPortMap(default_gateway_ipv4, 1234, 1000, g_interrupt, 1, 200ms);
MappingResult* mapping = std::get_if<MappingResult>(&res);
BOOST_REQUIRE(mapping);
@@ -339,7 +341,7 @@ BOOST_AUTO_TEST_CASE(pcp_ipv4)
return std::unique_ptr<PCPTestSock>();
};
- auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv4, bind_any_ipv4, 1234, 1000, 1, 1000ms);
+ auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv4, bind_any_ipv4, 1234, 1000, g_interrupt, 1, 1000ms);
MappingResult* mapping = std::get_if<MappingResult>(&res);
BOOST_REQUIRE(mapping);
@@ -383,7 +385,7 @@ BOOST_AUTO_TEST_CASE(pcp_ipv6)
return std::unique_ptr<PCPTestSock>();
};
- auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv6, default_local_ipv6, 1234, 1000, 1, 1000ms);
+ auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv6, default_local_ipv6, 1234, 1000, g_interrupt, 1, 1000ms);
MappingResult* mapping = std::get_if<MappingResult>(&res);
BOOST_REQUIRE(mapping);
@@ -406,7 +408,7 @@ BOOST_AUTO_TEST_CASE(pcp_timeout)
ASSERT_DEBUG_LOG("pcp: Retrying (2)");
ASSERT_DEBUG_LOG("pcp: Timeout");
- auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv4, bind_any_ipv4, 1234, 1000, 3, 2000ms);
+ auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv4, bind_any_ipv4, 1234, 1000, g_interrupt, 3, 2000ms);
MappingError* err = std::get_if<MappingError>(&res);
BOOST_REQUIRE(err);
@@ -435,7 +437,7 @@ BOOST_AUTO_TEST_CASE(pcp_connrefused)
ASSERT_DEBUG_LOG("pcp: Could not receive response");
- auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv4, bind_any_ipv4, 1234, 1000, 3, 2000ms);
+ auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv4, bind_any_ipv4, 1234, 1000, g_interrupt, 3, 2000ms);
MappingError* err = std::get_if<MappingError>(&res);
BOOST_REQUIRE(err);
@@ -495,7 +497,7 @@ BOOST_AUTO_TEST_CASE(pcp_ipv6_timeout_success)
ASSERT_DEBUG_LOG("pcp: Retrying (1)");
ASSERT_DEBUG_LOG("pcp: Timeout");
- auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv6, default_local_ipv6, 1234, 1000, 2, 2000ms);
+ auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv6, default_local_ipv6, 1234, 1000, g_interrupt, 2, 2000ms);
BOOST_CHECK(std::get_if<MappingResult>(&res));
}
@@ -534,7 +536,7 @@ BOOST_AUTO_TEST_CASE(pcp_ipv4_fail_no_resources)
return std::unique_ptr<PCPTestSock>();
};
- auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv4, bind_any_ipv4, 1234, 1000, 3, 1000ms);
+ auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv4, bind_any_ipv4, 1234, 1000, g_interrupt, 3, 1000ms);
MappingError* err = std::get_if<MappingError>(&res);
BOOST_REQUIRE(err);
@@ -570,7 +572,7 @@ BOOST_AUTO_TEST_CASE(pcp_ipv4_fail_unsupported_version)
return std::unique_ptr<PCPTestSock>();
};
- auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv4, bind_any_ipv4, 1234, 1000, 3, 1000ms);
+ auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv4, bind_any_ipv4, 1234, 1000, g_interrupt, 3, 1000ms);
MappingError* err = std::get_if<MappingError>(&res);
BOOST_REQUIRE(err);
@@ -602,7 +604,7 @@ BOOST_AUTO_TEST_CASE(natpmp_protocol_error)
return std::unique_ptr<PCPTestSock>();
};
- auto res = NATPMPRequestPortMap(default_gateway_ipv4, 1234, 1000, 1, 200ms);
+ auto res = NATPMPRequestPortMap(default_gateway_ipv4, 1234, 1000, g_interrupt, 1, 200ms);
MappingError* err = std::get_if<MappingError>(&res);
BOOST_REQUIRE(err);
@@ -647,7 +649,7 @@ BOOST_AUTO_TEST_CASE(natpmp_protocol_error)
return std::unique_ptr<PCPTestSock>();
};
- res = NATPMPRequestPortMap(default_gateway_ipv4, 1234, 1000, 1, 200ms);
+ res = NATPMPRequestPortMap(default_gateway_ipv4, 1234, 1000, g_interrupt, 1, 200ms);
err = std::get_if<MappingError>(&res);
BOOST_REQUIRE(err);
@@ -688,7 +690,7 @@ BOOST_AUTO_TEST_CASE(pcp_protocol_error)
return std::unique_ptr<PCPTestSock>();
};
- auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv4, bind_any_ipv4, 1234, 1000, 1, 1000ms);
+ auto res = PCPRequestPortMap(TEST_NONCE, default_gateway_ipv4, bind_any_ipv4, 1234, 1000, g_interrupt, 1, 1000ms);
MappingError* err = std::get_if<MappingError>(&res);
BOOST_REQUIRE(err);
Risk score
Our methodology →Why this scored 19/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.