Test-only UB in fuzz tests #24373

This line:

    ConnmanTestMsg& connman = *static_cast<ConnmanTestMsg*>(g_setup->m_node.connman.get());

in src/test/fuzz/process_message.cpp:37, is constructing a reference to a ConnmanTestMsg, which actually refers to an object of type Connman. Even though ConnmanTestMsg inherits from Connman, and adds no fields, I am pretty sure this is undefined behavior.

It isn't detected by the sanitizer because they're not polymorphic types for which runtime type information is tracked, but if you make Connman::~Connman() virtual, it does get detected:

test/fuzz/util.cpp:265:23: runtime error: member call on address 0x619000034380 which does not point to an object of type 'ConnmanTestMsg'
0x619000034380: note: object is of type 'CConnman'
 00 00 00 00  90 8e a0 29 19 56 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  01 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'CConnman'

I don't know how to quickly solve this myself, as I'm unfamiliar with this part of the code, so I'm opening an issue to discuss it.

@MarcoFalke

My understanding is that this should be fixed if the derived constructor is called. As there shouldn't be any risk in always calling the derived test-constructor, the following diff might fix the problem:

diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index c968e4d124..c74f26df6d 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -15,10 +15,10 @@
 #include <interfaces/chain.h>
 #include <net.h>
 #include <net_processing.h>
-#include <node/miner.h>
-#include <noui.h>
 #include <node/blockstorage.h>
 #include <node/chainstate.h>
+#include <node/miner.h>
+#include <noui.h>
 #include <policy/fees.h>
 #include <pow.h>
 #include <rpc/blockchain.h>
@@ -28,6 +28,7 @@
 #include <script/sigcache.h>
 #include <shutdown.h>
 #include <streams.h>
+#include <test/util/net.h>
 #include <txdb.h>
 #include <util/strencodings.h>
 #include <util/string.h>
@@ -227,7 +228,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
                                                /*deterministic=*/false,
                                                m_node.args->GetIntArg("-checkaddrman", 0));
     m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
-    m_node.connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests.
+    m_node.connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests.
     m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman,
                                        m_node.banman.get(), *m_node.chainman,
                                        *m_node.mempool, false);

That helped on the work that discovered this bug. Thanks, @MarcoFalke