The nScore field in LocalServiceInfo could overflow during version handshakes that wrap from INT_MAX to INT_MIN triggering undefined behavior in address selection
SeenLocal() now saturates nScore at std::numeric_limits <int>::max(), so further increments stay capped
The test LocalAddress_nScore_Overflow covers the saturation behavior
This should fix the issue #24049
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34028.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
319 | @@ -320,7 +320,10 @@ bool SeenLocal(const CService& addr) 320 | LOCK(g_maplocalhost_mutex); 321 | const auto it = mapLocalHost.find(addr); 322 | if (it == mapLocalHost.end()) return false; 323 | - ++it->second.nScore; 324 | + 325 | + if (it->second.nScore < std::numeric_limits<int>::max()) {
I just ran a mutation test for this PR, here's a mutant that wasn't killed and might be worth addressing (writing tests that can detect this change):
diff --git a/src/net.cpp b/src/net.cpp
index bbe24c8d3e..ab159e0123 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -321,7 +321,7 @@ bool SeenLocal(const CService& addr)
const auto it = mapLocalHost.find(addr);
if (it == mapLocalHost.end()) return false;
- if (it->second.nScore < std::numeric_limits<int>::max()) {
+ if (it->second.nScore > std::numeric_limits<int>::max()) {
++it->second.nScore;
}
return true;
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/20009656888/job/58081216641</sub>
<sub>LLM reason (✨ experimental): Lint check failed due to trailing whitespace detected (trailing_whitespace) in the source, causing the docker lint run to exit with code 1.</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
Could turn into draft while CI is red? @codeabysss Are you still working on this?
Could turn into draft while CI is red?
@codeabysss Are you still working on this?
yes still working on this
824 | + BOOST_CHECK(SeenLocal(addr)); 825 | + BOOST_CHECK_EQUAL(GetnScore(addr), std::numeric_limits<int>::max()); 826 | + } 827 | + 828 | + RemoveLocal(addr); 829 | + g_reachable_nets.Remove(NET_IPV4);
This line needs to go, or other tests might fail because they assume it's there.
(Failure depends on the order tests run in, which is semi-random)
97 | @@ -102,6 +98,9 @@ static constexpr bool DEFAULT_V2_TRANSPORT{true}; 98 | 99 | typedef int64_t NodeId; 100 | 101 | +/** Get the score of a local address. */ 102 | +int GetnScore(const CService& addr);
I want to hear what others think, but I'm not a fan of changing the function signature for it to only be used in a test. I think the test and this change can go, and you can provide a small diff (that doesn't get merged) to apply on top of this PR so reviewers can see that the issue is fixed.
I think making an existing function public for testing some behaviour is fine, but agree that providing a small diff to test the fix would be ok too. This is a small patch, so maybe dropping is easier and less code to review. @Crypt-iQ can you also take another look at its current state?
Ok I will take another look
A couple comments:
It seems like some of the wording in the OP was copied from the other PR #33072 and I think it would be better if you put things in your own words (my personal preference). Discussion in the other PR also mentioned that this has little to do with memory efficiency since it's just 4 bytes at the end of the day. I think the commit message could also be cleaned up a bit with a short title and then a descriptive paragraph underneath.
AddLocal() at line 297 has the same overflow potential:
info.nScore = nScore + (is_newly_added ? 0 : 1);
When !is_newly_added and nScore == INT_MAX, this overflows. Same class of bug as the one you're fixing in SeenLocal(). Might be worth including in this PR for completeness.
AddLocal() at line 297 has the same overflow potential:
This doesn't overflow. AddLocal is only called with values 1-4 (LOCAL_IF to LOCAL_MANUAL). If the key doesn't exist in the map, then it is initialized to a value in that range. If the key does exist in the map, AddLocal does nothing after info.nScore >= 4 because of the nScore >= info.nScore branch.
809 | + const int initial_score = 1000; 810 | + BOOST_REQUIRE(AddLocal(addr, initial_score)); 811 | + BOOST_REQUIRE(IsLocal(addr)); 812 | + BOOST_CHECK_EQUAL(GetnScore(addr), initial_score); 813 | + 814 | + // SeenLocal increments the score
Can move the above "SeenLocal increments the nScore is below max" comment to here
this is done
813 | + 814 | + // SeenLocal increments the score 815 | + BOOST_CHECK(SeenLocal(addr)); 816 | + BOOST_CHECK_EQUAL(GetnScore(addr), initial_score + 1); 817 | + 818 | + // SeenLocal saturates at max
maybe "Set nScore to max"?
this is done
818 | + // SeenLocal saturates at max 819 | + RemoveLocal(addr); 820 | + BOOST_REQUIRE(AddLocal(addr, std::numeric_limits<int>::max())); 821 | + BOOST_CHECK_EQUAL(GetnScore(addr), std::numeric_limits<int>::max()); 822 | + 823 | + // a couple increments should saturate
maybe "Check that nScore is saturated"?
Also, could remove the for loop and do the increment once instead.
this is done
Left some nits about the test comments. I think because the functional tests use the local interface and there is a IsRoutable check (which itself checks that the address isn't local) before calling SeenLocal, this would be hard or impossible to check in a functional test. So I think changing the function signature is fine.
Please rebase once more to resolve the CI issue.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/25186523989/job/73846539268</sub>
<sub>LLM reason (✨ experimental): CI failed because IWYU reported (and enforced) include-file changes, triggering “Failure generated from IWYU” in the iwyu step.</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
The nScore field would wrap from INT_MAX to INT_MIN when incremented
repeatedly, causing undefined behavior.
Saturate nScore at INT_MAX in SeenLocal() to prevent overflow.
Add test for saturation behavior.
Signed-off-by: codeabysss <codeabysss@proton.me>
Signed-off-by: codeabysss <248203105+codeabysss@users.noreply.github.com>
37 | @@ -38,6 +38,15 @@ using namespace std::literals; 38 | using namespace util::hex_literals; 39 | using util::ToString; 40 | 41 | +namespace { 42 | +int GetLocalScore(const CService& addr)
This does work, but I think changing the function signature like you did originally was fine. If there's any change to GetnScore then this function would have to be updated to reflect that, which could be forgotten.
I agree, no reason to just duplicate the code
reverted back thanks
ACK 33103d5c4fe2c0df85d1d8acf81d21846df6ee54
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task i686, no IPC: https://github.com/bitcoin/bitcoin/actions/runs/25328203293/job/74366220499</sub>
<sub>LLM reason (✨ experimental): CI failed because sock_tests crashed with fatal assertions (critical check s != SOCKET(-1)) indicating socket setup/creation failed.</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
I think the sock_tests are flaking and unrelated.
ACK 33103d5c4fe2c0df85d1d8acf81d21846df6ee54
I'm not sure how useful the test is here as per #34028 (review) . I think it would be better to drop it.
I'm not sure how useful the test is here as per #34028 (review) . I think it would be better to drop it.
+1. It was good to verify the fix works when running with ubsan, but I think it can be dropped and the function signature reverted back to normal.