fuzz: Restore SendMessages coverage in process_message(s) fuzz targets #34302

1. 
   maflcko commented at 11:45 am on January 15, 2026: member

   Found and reported by Crypt-iQ (thanks!)

   Currently the process_message(s) fuzz targets do not have any meaningful SendMessages code coverage. This is not ideal.

   Fix the problem by adding back the coverage, and by hardening the code here, so that the problem hopefully does not happen again in the future.

   Historic context for this regression

   The regression was introduced in commit fa11eea4059a608f591db4469c07a341fd33a031, which built a new deterministic peerman object. However, the patch was incomplete, because it was missing one hunk to replace g_setup->m_node.peerman->SendMessages(&p2p_node); with peerman->SendMessages(&p2p_node);.

   This means the stale and empty peerman from the node context and not the freshly created and deterministic peerman was used.

   A simple fix would be to just submit the missing patch hunk. However, this still leaves the risk that the issue is re-introduced at any time in the future. So instead, I think the stale and empty peerman should be de-constructed, so that any call to it will lead to a hard sanitizer error and fuzz failure.

   Doing that also uncovered another issue: The connman was holding on to a reference to a stale and empty addrman.

   So fix all issues by:

   • Allowing the addrman reference in connman to be re-seatable
   • Clearing all stale objects, before creating new objects, and then using references to the new objects in all code
2. DrahtBot added the label Fuzzing on Jan 15, 2026
3. 
   DrahtBot commented at 11:46 am on January 15, 2026: contributor

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

   Code Coverage & Benchmarks

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	Crypt-iQ, frankomosh, marcofleon, sedited

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #34181 (refactor: [p2p] Make ProcessMessage private again, Use references when non-null by maflcko)
   • #34162 (net: Avoid undershooting in GetAddressesUnsafe by fjahr)
   • #33878 (refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation by fjahr)
   • #33663 (net: Filter addrman during address selection via AddrPolicy to avoid underfill by waketraindev)
   • #28792 (build: Embedded ASMap [3/3]: Build binary dump header file by fjahr)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

4. maflcko force-pushed on Jan 15, 2026
5. DrahtBot added the label CI failed on Jan 15, 2026
6. 
   maflcko commented at 2:13 pm on January 15, 2026: member

   The CI fails, because I forgot to re-seat the dangling pointer. So I guess it is nice to know that the CI will catch this issue, going forward. Not sure how to re-seat the pointer. I guess I could use placement new to just overwrite the memory, but I guess this is UB without std::launder. So I’ll just use a plain std::reference_wrapper.
7. refactor: Use std::reference_wrapper<AddrMan> in Connman

   The addrman field is already a reference. However, some tests would
   benefit from the reference being re-seatable, so that they do not have
   to create a full Connman each time.

   fac7fed397
8. fuzz: Restore SendMessages coverage in process_message(s) fuzz targets fabf8d1c5b
9. maflcko force-pushed on Jan 15, 2026
10. maflcko marked this as ready for review on Jan 15, 2026
11. in src/test/fuzz/process_message.cpp:95 in fabf8d1c5b

     98                                          .deterministic_rng = true,
     99                                      });
    100 
    101-    connman.SetMsgProc(peerman.get());
    102+    connman.SetMsgProc(node.peerman.get());
    103+    connman.SetAddrman(*node.addrman);
    

    ---

    ---

    

    Crypt-iQ commented at 3:39 pm on January 15, 2026:

    Is it possible that one of connman’s threads tries to use addrman after node.addrman.reset() but before connman.SetAddrman?

    ---

    

    maflcko commented at 7:25 pm on January 15, 2026:

    In theory, yes. In practise, it should not, because all nodes are stopped in the end: node.connman->StopNodes();. Also, connman should not try to ask the addrman to open connections to remote nodes during any of the unit tests.

    So I guess, if the fuzz test were to run into a use-after-free here, it would be something to look into and fix (likely all unit tests will be affected).

    So I think it is better to leave as-is?

    ---

    

    Crypt-iQ commented at 8:08 pm on January 15, 2026:

    Makes sense, I’m ok leaving it as-is and fixing a use-after-free if it comes up.
12. DrahtBot removed the label CI failed on Jan 15, 2026
13. 
    Crypt-iQ commented at 8:08 pm on January 15, 2026: contributor

    crACK fabf8d1c5bdb6a944762792cdc762caa6c1a760b
14. 
    maflcko commented at 8:12 pm on January 15, 2026: member

    For reference, I used the following patch to confirm that this pull request fixes the coverage problem and that the mentioned commit introduced the coverage problem. (When the assert is hit, coverage exists. When the fuzz program runs fine, no coverage exists):

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
     1index 1202eaf152..099ef3eb5e 100644
     2--- a/src/net_processing.cpp
     3+++ b/src/net_processing.cpp
     4@@ -5680,2 +5680,5 @@ bool PeerManagerImpl::SetupAddressRelay(const CNode& node, Peer& peer)
     5 
     6+extern bool g_crash;
     7+bool g_crash{true};
     8+
     9 bool PeerManagerImpl::SendMessages(CNode* pto)
    10@@ -5687,2 +5690,3 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
    11     if (!peer) return false;
    12+    assert(!g_crash); // is this line covered ?
    13     const Consensus::Params& consensusParams = m_chainparams.GetConsensus();
    14diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
    15index ef8cb686ce..dfefafd61b 100644
    16--- a/src/test/fuzz/process_message.cpp
    17+++ b/src/test/fuzz/process_message.cpp
    18@@ -32,2 +32,3 @@
    19 
    20+extern bool g_crash;
    21 namespace {
    22@@ -67,2 +68,3 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
    23 {
    24+    g_crash = false;
    25     SeedRandomStateForTest(SeedRand::ZEROS);
    26@@ -105,2 +107,3 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
    27     FillNode(fuzzed_data_provider, connman, p2p_node);
    28+    g_crash = true;
    29 
    30diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp
    31index f36f528b0e..a1c006bade 100644
    32--- a/src/test/fuzz/process_messages.cpp
    33+++ b/src/test/fuzz/process_messages.cpp
    34@@ -28,2 +28,3 @@
    35 
    36+extern bool g_crash;
    37 namespace {
    38@@ -57,2 +58,3 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages)
    39 {
    40+    g_crash = false;
    41     SeedRandomStateForTest(SeedRand::ZEROS);
    42@@ -98,2 +100,3 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages)
    43 
    44+    g_crash = true;
    45     LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 30)
    

15. 
    Crypt-iQ commented at 9:30 pm on January 16, 2026: contributor

    Is it possible that these ever create a valid tx given a good dictionary? If so, I think resetting the mempool would be a good idea. I think this is probably irrelevant though.
16. 
    maflcko commented at 10:17 am on January 19, 2026: member

    Is it possible that these ever create a valid tx given a good dictionary? If so, I think resetting the mempool would be a good idea. I think this is probably irrelevant though.

    I think the only problem it could cause is non-determinism. Similar to fa1a14a13a15ecfb7587a94ee86b4ace7c819519 (for the chainman), a fix could make sense here. I’ll do that in a follow-up, so that this one is focussed on restoring the regressed coverage.

17. 
    frankomosh commented at 7:15 am on January 20, 2026: contributor

    ACK fabf8d1c5bdb6a944762792cdc762caa6c1a760b restores SendMessages coverage by ensuring fuzz targets use freshly-created deterministic objects instead of stale ones.
18. 
    marcofleon commented at 3:25 pm on January 20, 2026: contributor

    code review ACK fabf8d1c5bdb6a944762792cdc762caa6c1a760b
19. sedited approved
20. 
    sedited commented at 3:32 pm on January 20, 2026: contributor

    ACK fabf8d1c5bdb6a944762792cdc762caa6c1a760b

    Indeed seems to increase coverage a bit.

21. sedited merged this on Jan 20, 2026
22. sedited closed this on Jan 20, 2026

    ---

23. maflcko deleted the branch on Jan 20, 2026

Contributors
maflcko DrahtBot Crypt-iQ frankomosh marcofleon sedited

Labels
Fuzzing