p2p: Saturate LocalServiceInfo::nScore updates at INT_MAX
What the developer wrote
Authored by codeabysss
p2p: Saturate LocalServiceInfo::nScore updates at INT_MAX
Signed overflow on nScore updates is undefined behavior. Use
SaturatingAdd in AddLocal() and SeenLocal() so increments saturate at
INT_MAX instead of overflowing.
Add unit test coverage for saturation in both code paths.
What changed, and why it matters
This commit fixes a subtle programming bug in Bitcoin Core's peer-to-peer networking code. A local network address score counter could, in theory, overflow when repeatedly incremented, which is 'undefined behavior' in C++ and could potentially cause the program to misbehave or crash. The fix makes the counter stop increasing once it reaches the maximum integer value, and adds tests to confirm that behavior.
Treat as a low-to-moderate hardening fix. Review whether any other signed counters in the p2p layer use plain increment/addition and could overflow. No immediate emergency response is indicated, but the fix should be included in routine maintenance/backports.
Security signals we found
Fixes undefined behavior (signed integer overflow)
Uses saturating arithmetic helper (SaturatingAdd)
Adds unit tests for overflow/saturation behavior
Touches local address advertisement scoring (p2p network self-advertisement)
Evidence from the diff
The patch replaces signed-integer additions on LocalServiceInfo::nScore in AddLocal() and SeenLocal() with SaturatingAdd(), preventing signed overflow (undefined behavior in C++). nScore is an int used to rank locally-advertised service addresses. AddLocal() previously computed nScore + 1 for existing entries, and SeenLocal() used prefix ++. Both could overflow if nScore reached INT_MAX. The patch also adds a unit test verifying saturation in both code paths.
Changed components
src/net.cpp (AddLocal, SeenLocal)LocalServiceInfo::nScoresrc/test/net_tests.cppInspect captured patch +38 / −2
diff --git a/src/net.cpp b/src/net.cpp
index f8149ef3..ca0289c8 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -29,6 +29,7 @@
#include <random.h>
#include <scheduler.h>
#include <util/fs.h>
+#include <util/overflow.h>
#include <util/sock.h>
#include <util/strencodings.h>
#include <util/thread.h>
@@ -296,7 +297,7 @@ bool AddLocal(const CService& addr_, int nScore)
const auto [it, is_newly_added] = mapLocalHost.emplace(addr, LocalServiceInfo());
LocalServiceInfo &info = it->second;
if (is_newly_added || nScore >= info.nScore) {
- info.nScore = nScore + (is_newly_added ? 0 : 1);
+ info.nScore = SaturatingAdd(nScore, is_newly_added ? 0 : 1);
info.nPort = addr.GetPort();
}
}
@@ -325,7 +326,7 @@ bool SeenLocal(const CService& addr)
LOCK(g_maplocalhost_mutex);
const auto it = mapLocalHost.find(addr);
if (it == mapLocalHost.end()) return false;
- ++it->second.nScore;
+ it->second.nScore = SaturatingAdd(it->second.nScore, 1);
return true;
}
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index 32801d97..7168d3dc 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -802,6 +802,41 @@ BOOST_AUTO_TEST_CASE(LocalAddress_BasicLifecycle)
BOOST_CHECK(!IsLocal(addr));
}
+BOOST_AUTO_TEST_CASE(LocalAddress_nScore_Overflow)
+{
+ g_reachable_nets.Add(NET_IPV4);
+ const CService addr{UtilBuildAddress(0x002, 0x001, 0x001, 0x001), 1000}; // 2.1.1.1:1000
+
+ const auto get_score = [](const CService& service) -> int {
+ LOCK(g_maplocalhost_mutex);
+ const auto it = mapLocalHost.find(service);
+ return it != mapLocalHost.end() ? it->second.nScore : 0;
+ };
+
+ const int initial_score = 1000;
+ BOOST_REQUIRE(AddLocal(addr, initial_score));
+ BOOST_REQUIRE(IsLocal(addr));
+ BOOST_CHECK_EQUAL(get_score(addr), initial_score);
+
+ // SeenLocal should increment nScore by 1.
+ BOOST_CHECK(SeenLocal(addr));
+ BOOST_CHECK_EQUAL(get_score(addr), initial_score + 1);
+
+ // AddLocal() saturates nScore when updating an existing entry at INT_MAX.
+ BOOST_REQUIRE(AddLocal(addr, std::numeric_limits<int>::max()));
+ BOOST_CHECK_EQUAL(get_score(addr), std::numeric_limits<int>::max());
+
+ BOOST_CHECK(AddLocal(addr, std::numeric_limits<int>::max()));
+ BOOST_CHECK_EQUAL(get_score(addr), std::numeric_limits<int>::max());
+
+ // SeenLocal() also saturates at INT_MAX.
+ BOOST_CHECK(SeenLocal(addr));
+ BOOST_CHECK_EQUAL(get_score(addr), std::numeric_limits<int>::max());
+
+ RemoveLocal(addr);
+ BOOST_CHECK(!IsLocal(addr));
+}
+
BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
{
LOCK(NetEventsInterface::g_msgproc_mutex);
Why this scored 37/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.