p2p: Prevent integer overflow in LocalServiceInfo::nScore #34028

pull codeabysss wants to merge 2 commits into bitcoin:master from codeabysss:p2p-fix-nscore-overflow changing 3 files +42 −3
  1. codeabysss commented at 8:08 PM on December 7, 2025: none

    The overflow for signed arithmetic yields undefined behavior. This changes prevents undefined behavior in local address scoring by saturating nScore updates at INT_MAX in both SeenLocal() and AddLocal() update paths.

    This should fix the issue #24049

  2. DrahtBot commented at 8:08 PM on December 7, 2025: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34028.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK sedited
    Stale ACK Crypt-iQ

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  3. fanquake renamed this:
    [p2p] Saturate LocalServiceInfo::nScore to prevent overflow
    p2p: saturate LocalServiceInfo::nScore to prevent overflow
    on Dec 8, 2025
  4. DrahtBot added the label P2P on Dec 8, 2025
  5. in src/net.cpp:324 in f11b165f35 outdated 
     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()) {
    brunoerg commented at 5:45 PM on December 10, 2025:

    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;
  6. DrahtBot added the label CI failed on Dec 15, 2025
  7. DrahtBot commented at 5:31 PM on December 15, 2025: contributor

    <!--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>

  8. DrahtBot commented at 9:34 AM on December 22, 2025: contributor

    Could turn into draft while CI is red? @codeabysss Are you still working on this?

  9. codeabysss commented at 10:24 AM on December 22, 2025: none

    Could turn into draft while CI is red?

    @codeabysss Are you still working on this?

    yes still working on this

  10. DrahtBot marked this as a draft on Dec 22, 2025
  11. codeabysss force-pushed on Dec 22, 2025
  12. codeabysss requested review from brunoerg on Dec 22, 2025
  13. codeabysss marked this as ready for review on Dec 22, 2025
  14. DrahtBot removed the label CI failed on Dec 23, 2025
  15. luke-jr referenced this in commit d8fc27f1a3 on Jan 13, 2026
  16. in src/test/net_tests.cpp:829 in a364f6303c 
     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);
    luke-jr commented at 10:36 PM on January 26, 2026:

    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)

  17. codeabysss force-pushed on Feb 5, 2026
  18. codeabysss requested review from luke-jr on Feb 5, 2026
  19. luke-jr referenced this in commit 8caf0836a8 on Feb 7, 2026
  20. DrahtBot added the label CI failed on Feb 7, 2026
  21. DrahtBot closed this on Feb 9, 2026
  22. DrahtBot reopened this on Feb 9, 2026
  23. DrahtBot removed the label CI failed on Feb 9, 2026
  24. in src/net.h:102 in 10100f86f5 outdated 
      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);
    Crypt-iQ commented at 2:46 PM on February 11, 2026:

    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.

    sedited commented at 8:38 AM on April 20, 2026:

    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?

    Crypt-iQ commented at 2:19 PM on April 22, 2026:

    Ok I will take another look

    l0rinc commented at 2:01 PM on May 12, 2026:

    Alternatively we could add a test-local lambda like:

    BOOST_AUTO_TEST_CASE(LocalAddress_nScore_Overflow)
{
    g_reachable_nets.Add(NET_IPV4);
    CService addr{UtilBuildAddress(0x002, 0x001, 0x001, 0x001), 1000}; // 2.1.1.1:1000
    auto check_score{[&](int expected) {
        const auto local_addresses{m_node.connman->getNetLocalAddresses()};
        const auto it{local_addresses.find(addr)};
        BOOST_REQUIRE(it != local_addresses.end());
        BOOST_CHECK_EQUAL(it->second.nScore, expected);
    }};

    const int initial_score = 1000;
    BOOST_REQUIRE(AddLocal(addr, initial_score));
    BOOST_REQUIRE(IsLocal(addr));
    check_score(initial_score);

...
    Crypt-iQ commented at 10:59 PM on May 12, 2026:

    I'm ~0 on a test being added if we're doing a SaturatedAdd in SeenLocal & AddLocal, then we're just testing SaturatedAdd?. Using m_node.connman->getNetLocalAddresses does have the benefit of not needing to change the function signature though.

    l0rinc commented at 12:54 PM on May 13, 2026:

    Using m_node.connman->getNetLocalAddresses does have the benefit of not needing to change the function signature though.

    Yes, that was the reason for suggesting it.

    then we're just testing SaturatedAdd

    We're testing the current behavior black-box style to document it and avoid regressing later.

    Crypt-iQ commented at 1:40 PM on May 13, 2026:

    We're testing the current behavior black-box style to document it and avoid regressing later.

    Sure, I just think it's a bit overkill for what could be a ~3-line change. Hopefully somebody would see saturation and not remove it, but maybe that's too much to ask and not robust. I don't really mind at this point (hence the ~0), the issue has been open for over 3 years with ~4-5 PRs trying to fix what should have been an extremely simple fix.

  25. Crypt-iQ commented at 2:46 PM on February 11, 2026: contributor

    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.

  26. codeabysss force-pushed on Feb 12, 2026
  27. codeabysss renamed this:
    p2p: saturate LocalServiceInfo::nScore to prevent overflow
    p2p: Prevent integer overflow in LocalServiceInfo::nScore
    on Feb 12, 2026
  28. chriszeng1010 commented at 10:34 PM on February 25, 2026: none

    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.

    https://github.com/bitcoin/bitcoin/pull/34674

  29. Crypt-iQ commented at 3:40 PM on April 22, 2026: contributor

    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.

  30. in src/test/net_tests.cpp:814 in 3fc5948e1f 
     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
    Crypt-iQ commented at 7:35 PM on April 23, 2026:

    Can move the above "SeenLocal increments the nScore is below max" comment to here

    codeabysss commented at 7:34 PM on April 30, 2026:

    this is done

  31. in src/test/net_tests.cpp:818 in 3fc5948e1f 
     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
    Crypt-iQ commented at 7:38 PM on April 23, 2026:

    maybe "Set nScore to max"?

    codeabysss commented at 7:34 PM on April 30, 2026:

    this is done

  32. in src/test/net_tests.cpp:823 in 3fc5948e1f 
     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
    Crypt-iQ commented at 7:39 PM on April 23, 2026:

    maybe "Check that nScore is saturated"?

    Crypt-iQ commented at 1:07 PM on April 24, 2026:

    Also, could remove the for loop and do the increment once instead.

    codeabysss commented at 7:34 PM on April 30, 2026:

    this is done

  33. Crypt-iQ commented at 7:43 PM on April 23, 2026: contributor

    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.

  34. codeabysss force-pushed on Apr 30, 2026
  35. codeabysss force-pushed on Apr 30, 2026
  36. codeabysss requested review from Crypt-iQ on Apr 30, 2026
  37. hebasto commented at 8:23 PM on April 30, 2026: member

    Please rebase once more to resolve the CI issue.

  38. DrahtBot added the label CI failed on Apr 30, 2026
  39. DrahtBot commented at 9:31 PM on April 30, 2026: contributor

    <!--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>

  40. codeabysss force-pushed on May 1, 2026
  41. DrahtBot removed the label CI failed on May 1, 2026
  42. p2p: Prevent integer overflow in LocalServiceInfo::nScore
    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>
    33103d5c4f
  43. in src/test/net_tests.cpp:42 in 39191c6308 
      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)
    Crypt-iQ commented at 9:09 AM on May 1, 2026:

    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.

    luke-jr commented at 7:18 PM on May 2, 2026:

    I agree, no reason to just duplicate the code

    codeabysss commented at 3:37 PM on May 4, 2026:

    reverted back thanks

  44. codeabysss force-pushed on May 4, 2026
  45. Crypt-iQ commented at 3:33 AM on May 5, 2026: contributor

    ACK 33103d5c4fe2c0df85d1d8acf81d21846df6ee54

  46. DrahtBot added the label CI failed on May 5, 2026
  47. DrahtBot commented at 8:42 AM on May 5, 2026: contributor

    <!--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>

  48. Crypt-iQ commented at 9:41 AM on May 5, 2026: contributor

    I think the sock_tests are flaking and unrelated.

  49. DrahtBot removed the label CI failed on May 5, 2026
  50. sedited approved
  51. sedited commented at 4:12 PM on May 10, 2026: contributor

    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.

  52. Crypt-iQ commented at 8:32 AM on May 11, 2026: contributor

    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.

  53. in src/test/net_tests.cpp:811 in 33103d5c4f outdated 
     806 | +{
 807 | +    g_reachable_nets.Add(NET_IPV4);
 808 | +    CService addr{UtilBuildAddress(0x002, 0x001, 0x001, 0x001), 1000}; // 2.1.1.1:1000
 809 | +
 810 | +    const int initial_score = 1000;
 811 | +    BOOST_REQUIRE(AddLocal(addr, initial_score));
    l0rinc commented at 1:39 PM on May 12, 2026:

    We should probably cover the nScore update in https://github.com/bitcoin/bitcoin/blob/11713c9fa91ca583063750cc826d0138b3a611f6/src/net.cpp#L299 as well:

                info.nScore = SaturatingAdd(nScore, is_newly_added ? 0 : 1);
    codeabysss commented at 11:27 AM on May 22, 2026:

    done thanks

  54. in src/test/net_tests.cpp:828 in 33103d5c4f outdated 
     823 | +
 824 | +    // Check that nScore is saturated.
 825 | +    BOOST_CHECK(SeenLocal(addr));
 826 | +    BOOST_CHECK_EQUAL(GetnScore(addr), std::numeric_limits<int>::max());
 827 | +
 828 | +    RemoveLocal(addr);
    l0rinc commented at 1:52 PM on May 12, 2026:

    The existing-entry update in AddLocal() still computes nScore + 1 when the supplied score is at least the stored score.

    That leaves the same signed-overflow boundary reachable through the public local-address scoring helper if an existing entry is updated with std::numeric_limits<int>::max().

        BOOST_CHECK(AddLocal(addr, std::numeric_limits<int>::max()));
    BOOST_CHECK_EQUAL(GetnScore(addr), std::numeric_limits<int>::max());

    (Please make sure to update the code comments after fixing this.)

    codeabysss commented at 11:27 AM on May 22, 2026:

    Done added checks for AddLocal and updated the test comments. Score is read via mapLocalHost since GetnScore() is static again.

  55. in src/net.cpp:330 in 33103d5c4f 
     324 | @@ -325,7 +325,9 @@ bool SeenLocal(const CService& addr)
 325 |      LOCK(g_maplocalhost_mutex);
 326 |      const auto it = mapLocalHost.find(addr);
 327 |      if (it == mapLocalHost.end()) return false;
 328 | -    ++it->second.nScore;
 329 | +    if (it->second.nScore < std::numeric_limits<int>::max()) {
 330 | +        ++it->second.nScore;
 331 | +    }
    l0rinc commented at 2:02 PM on May 12, 2026:

    We already have https://github.com/bitcoin/bitcoin/blob/8b49e2dd4eed93d08a3d9681d444aaf441ab0037/src/util/overflow.h#L45-L60 for this exact usecase, let's use it here

        it->second.nScore = SaturatingAdd(it->second.nScore, 1);
  56. in src/test/net_tests.cpp:808 in 33103d5c4f 
     801 | @@ -802,6 +802,33 @@ BOOST_AUTO_TEST_CASE(LocalAddress_BasicLifecycle)
 802 |      BOOST_CHECK(!IsLocal(addr));
 803 |  }
 804 |  
 805 | +BOOST_AUTO_TEST_CASE(LocalAddress_nScore_Overflow)
 806 | +{
 807 | +    g_reachable_nets.Add(NET_IPV4);
 808 | +    CService addr{UtilBuildAddress(0x002, 0x001, 0x001, 0x001), 1000}; // 2.1.1.1:1000
    l0rinc commented at 2:04 PM on May 12, 2026:

    nit: can be const

  57. in src/test/net_tests.cpp:820 in 33103d5c4f 
     815 | +    // SeenLocal increments nScore when it is below max.
 816 | +    BOOST_CHECK(SeenLocal(addr));
 817 | +    BOOST_CHECK_EQUAL(GetnScore(addr), initial_score + 1);
 818 | +
 819 | +    // Set nScore to max.
 820 | +    RemoveLocal(addr);
    l0rinc commented at 2:09 PM on May 12, 2026:

    Why is this necessarry? We could probably avoid removing and re-adding the address here.

  58. l0rinc changes_requested
  59. l0rinc commented at 2:21 PM on May 12, 2026: contributor

    wrap from INT_MAX to INT_MIN triggering undefined behavior

    Signed overflow is UB, so the wrap is not guaranteed, please reword the PR description, see the C++ draft:

    Overflow for signed arithmetic yields undefined behavior

  60. sedited commented at 8:30 AM on May 22, 2026: contributor

    @codeabysss do you want to address the remaining comments here?

  61. 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.

Signed-off-by: codeabysss <248203105+codeabysss@users.noreply.github.com>
    6d37cfe0eb
  62. codeabysss commented at 11:28 AM on May 22, 2026: none

    Thanks for the review.

    updates in the latest push

    • Use SaturatingAdd() in both AddLocal() and SeenLocal()
    • Reverted GetnScore() back to static
    • Updated PR description to describe signed overflow as UB not a guaranteed wrap to INT_MIN
    • Kept a focused unit test that covers saturation in both update paths, with the nits addressed const addr
  63. sedited approved
  64. sedited commented at 2:28 PM on May 25, 2026: contributor

    Re-ACK 6d37cfe0eb696825760261b7f1c5ba441d7f8011

  65. DrahtBot requested review from Crypt-iQ on May 25, 2026
  66. fanquake commented at 5:05 PM on May 25, 2026: member

    cc @maflcko

  67. Crypt-iQ commented at 1:26 PM on May 26, 2026: contributor

    @codeabysss could you squash your 2 commits into one commit so the second commit is not changing what the first introduced?

Contributors
codeabysssDrahtBotbrunoergluke-jrCrypt-iQseditedl0rincchriszeng1010hebastofanquake


luke-jrCrypt-iQbrunoerg

Labels
P2P
Linked (view graph)
#24049 net: signed-integer-overflow in LocalServiceInfo#34674 net: prevent signed integer overflow in AddLocal nScore update
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-06-01 11:51 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me