fuzz: exercise ForNode/ForEachNode callbacks in connman fuzz harness #34934

frankomosh commented at 9:01 AM on March 27, 2026: contributor

Track inserted node IDs and sometimes reuse them in ForNode() so the successful lookup path is exercised more reliably. Replace no-op callbacks with lightweight CNode accessor calls to make ForEachNode() and ForNode() cover previously unreached callback code paths.

This addresses feedback from #34830 (comment) where it was noted that the callbacks had "neither the return type checked nor its side-effect”.

Coverage reports from the connman fuzz corpus, before and after the change:

Before
After

diff cov_show_before.txt cov_show_after.txt filtered to ForNode/ForEachNode/IsFullOutboundConn/ConnectionTypeAsString:

IsFullOutboundConn — net.h:786-788

-  786|      0|    bool IsFullOutboundConn() const {
-  787|      0|        return m_conn_type == ConnectionType::OUTBOUND_FULL_RELAY;
-  788|      0|    }
+  786|  1.13M|    bool IsFullOutboundConn() const {
+  787|  1.13M|        return m_conn_type == ConnectionType::OUTBOUND_FULL_RELAY;
+  788|  1.13M|    }

ConnectionTypeAsString — net.h:967

-  967|      0|    std::string ConnectionTypeAsString() const { return ::ConnectionTypeAsString(m_conn_type); }
+  967|  1.11M|    std::string ConnectionTypeAsString() const { return ::ConnectionTypeAsString(m_conn_type); }

ForNode — net.cpp:4126-4131

-  4126|  1.08k|        if(pnode->GetId() == id) {
-    |  Branch (4126:12): [True: 0, False: 1.08k]
-  4127|      0|            found = pnode;
-  4131|     39|    return found != nullptr && NodeFullyConnected(found) && func(found);
-                                              ^0                          ^0
+  4126|    602|        if(pnode->GetId() == id) {
+    |  Branch (4126:12): [True: 1, False: 601]
+  4127|      1|            found = pnode;
+  4131|     28|    return found != nullptr && NodeFullyConnected(found) && func(found);
+                                              ^1                          ^1

ForEachNode — net.h:1270-1271

-  1270|  1.13M|            if (NodeFullyConnected(node))
-    |  Branch (1270:17): [True: 0, False: 1.13M]
-  1271|      0|                func(node);
+  1270|  1.11M|            if (NodeFullyConnected(node))
+    |  Branch (1270:17): [True: 1.11M, False: 0]
+  1271|  1.11M|                func(node);

Two previously uncovered functions (IsFullOutboundConn, ConnectionTypeAsString) are now exercised through the iteration callbacks. ForNode finds matching nodes.

DrahtBot added the label Fuzzing on Mar 27, 2026

DrahtBot commented at 9:01 AM on March 27, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	enirox001
Stale ACK	maflcko

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

maflcko commented at 1:41 PM on March 27, 2026: member

lgtm ACK b6d846147bbeb38833bd768829d1dadf904527e5

Seems fine

in src/test/fuzz/connman.cpp:161 in b6d846147b

 158 | +                    : PickValue(fuzzed_data_provider, node_ids);
 159 | +                (void)connman.ForNode(id, [&](CNode* pnode) {
 160 | +                    (void)pnode->GetId();
 161 | +                    (void)pnode->IsInboundConn();
 162 | +                    (void)pnode->IsFullOutboundConn();
 163 | +                    return fuzzed_data_provider.ConsumeBool();

brunoerg commented at 2:01 PM on April 15, 2026:

Why does it need to consume from the input if you're not using it? Couldn't you just return true?

frankomosh commented at 7:10 AM on April 16, 2026:

Thanks for taking a look. Yes, true seems simpler and clearer here

fuzz: exercise ForNode/ForEachNode callbacks in connman fuzz harness

Track inserted node IDs and select from them when calling ForNode, so the ID-match branch is reliably hit. Replace no-op callbacks with CNode accessor calls to exercise previously uncovered code
paths (IsFullOutboundConn, ConnectionTypeAsString) through the iteration.

5e06a80d7a

in src/test/fuzz/connman.cpp:146 in b6d846147b outdated

 142 | @@ -141,10 +143,23 @@ FUZZ_TARGET(connman, .init = initialize_connman)
 143 |                  connman.DisconnectNode(random_subnet);
 144 |              },
 145 |              [&] {
 146 | -                connman.ForEachNode([](auto) {});
 147 | +                connman.ForEachNode([](CNode* pnode) {

brunoerg commented at 2:03 PM on April 15, 2026:

Not sure about it because in the worst scenario it would call ForEachNode for the same nodes 10'000x. Wouldn't be better to move it to out of the loop?

frankomosh commented at 7:38 AM on April 16, 2026:

ForEachNode runs ~35x (per my corpus files input measurement), on average (119k calls across 3391 inputs). I'd like to think LIMITED_WHILE rarely hits the 10000 cap, see ConsumeBool() exits early.

I kept it inside CallOneOf to let it interleave with DisconnectNode (which fires 42k times across the corpus), exercising NodeFullyConnected's false branch on partially disconnected node sets. The false branch shows [True: 7.48M, False: 3.22M], the 3.22M false hits come from this interleaving.

frankomosh force-pushed on Apr 16, 2026

DrahtBot added the label CI failed on Apr 16, 2026

DrahtBot removed the label CI failed on Apr 16, 2026

enirox001 commented at 11:53 AM on April 21, 2026: contributor

Concept ACK 5e06a80

Not very familiar with this part of the codebase, but the change seems narrow and coherent: it records IDs for fuzz-created test nodes, uses those IDs to make the CConnman::ForNode() hit path reachable more often, and replaces no-op callbacks with harmless CNode accessors so the callback bodies actually exercise code.

I do have a nit though and it’s about the fuzz harness doing less than the commit message claims rather than a runtime crash

in src/test/fuzz/connman.cpp:105 in 5e06a80d7a

  98 | @@ -99,12 +99,14 @@ FUZZ_TARGET(connman, .init = initialize_connman)
  99 |      CNode random_node = ConsumeNode(fuzzed_data_provider);
 100 |      CSubNet random_subnet;
 101 |      std::string random_string;
 102 | +    std::vector<NodeId> node_ids;
 103 |  
 104 |      LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100) {
 105 |          CNode& p2p_node{*ConsumeNodeAsUniquePtr(fuzzed_data_provider).release()};

enirox001 commented at 11:54 AM on April 21, 2026:

In commit "fuzz: exercise ForNode/ForEachNode callbacks in connman fuzz harness" (https://github.com/bitcoin/bitcoin/pull/34934/changes/5e06a80d7a4590c0be0c2888f5ac12dbf40951c5):

I think the new node_ids tracking makes the ForNode() callback path more likely, but not reliably reachable in the way the commit message suggests.

In this harness, the initial peers are still created with ConsumeNodeAsUniquePtr(fuzzed_data_provider), which means their NodeIds come from fuzz input rather than a uniqueness source. If duplicate IDs are inserted into m_nodes, CConnman::ForNode() stops at the first GetId() == id match and then checks NodeFullyConnected() only for that one node; it does not continue scanning for a later live peer with the same ID. So PickValue(..., node_ids) can still miss the callback entirely even when another fully connected node with that same ID exists.

I verified this locally with a temporary deterministic repro: two nodes sharing one NodeId, first marked disconnected, second left live. ForEachNode() still saw one live node for that ID, while ForNode(id, ...) returned false and never invoked the callback.

Would it make sense to assign unique IDs explicitly when constructing the fuzz peers, e.g. by passing a deterministic unique NodeId into ConsumeNodeAsUniquePtr(...)? That would make the coverage improvement deterministic and align better with the stated goal.

frankomosh commented at 12:41 PM on April 21, 2026:

Thanks for reviewing. the IDs come from fuzz input by design, and duplicate IDs are a valid scenario worth exploring for the fuzzer. Making them deterministically unique would remove that coverage. The goal here is to make the hit path more likely, not guaranteed. That said I think you are right that the commit message could be softened to something like "more reliably" or "more frequently" instead of "reliably".