net: fix use-after-free with v2->v1 reconnection logic #33956

CConnman::Stop() resets semOutbound, yet m_reconnections is not cleared in Stop. Each ReconnectionInfo contains a grant member that points to the memory that semOutbound pointed to and ~CConnman will attempt to access the grant field (memory that was already freed) when destroying m_reconnections. Fix this by calling m_reconnections.clear() in CConnman::Stop() and add appropriate annotations.

I was able to reproduce the original issue #33615 with the following diff by randomly stopping my node while it was attempting to reconnect (and verified that this patch fixes the issue, at least in my ~40-50 runs):

<details> <summary> diff </summary>

diff --git a/src/net.cpp b/src/net.cpp
index ef1c63044a..9c1d161d8b 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1918,8 +1918,8 @@ void CConnman::DisconnectNodes()
     {
         LOCK(m_nodes_mutex);
 
-        const bool network_active{fNetworkActive};
-        if (!network_active) {
+//        const bool network_active{fNetworkActive};
+//        if (!network_active) {
             // Disconnect any connected nodes
             for (CNode* pnode : m_nodes) {
                 if (!pnode->fDisconnect) {
@@ -1927,7 +1927,7 @@ void CConnman::DisconnectNodes()
                     pnode->fDisconnect = true;
                 }
             }
-        }
+//        }
 
         // Disconnect unused nodes
         std::vector<CNode*> nodes_copy = m_nodes;
@@ -1941,7 +1941,7 @@ void CConnman::DisconnectNodes()
                 // Add to reconnection list if appropriate. We don't reconnect right here, because
                 // the creation of a connection is a blocking operation (up to several seconds),
                 // and we don't want to hold up the socket handler thread for that long.
-                if (network_active && pnode->m_transport->ShouldReconnectV1()) {
+                if (true) {
                     reconnections_to_add.push_back({
                         .addr_connect = pnode->addr,
                         .grant = std::move(pnode->grantOutbound),

</details>

I'm curious to see if others can reproduce as well.

<!--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/33956.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

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

Reviewers, this pull request conflicts with the following ones:

• #32394 (net: make m_nodes_mutex non-recursive by vasild)
• #32015 (net: replace manual reference counting of CNode with shared_ptr by vasild)
• #30988 (Split CConnman by vasild)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Code review ACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe

utACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe

ACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe

didn't test it, but but the explanation of the bug make sense and since all the network threads have been stopped at this point anyway I can't see a downside to clearing m_reconnections here.