Test-only UB in fuzz tests

sipa commented at 8:49 pm on February 17, 2022: member

This line:

0    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:

0test/fuzz/util.cpp:265:23: runtime error: member call on address 0x619000034380 which does not point to an object of type 'ConnmanTestMsg'
10x619000034380: note: object is of type 'CConnman'
2 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
3              ^~~~~~~~~~~~~~~~~~~~~~~
4              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.

sipa added the label Bug on Feb 17, 2022

sipa added the label Tests on Feb 17, 2022

sipa commented at 8:52 pm on February 17, 2022: member

@MarcoFalke

MarcoFalke commented at 7:09 am on February 18, 2022: member

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:

 0diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
 1index c968e4d124..c74f26df6d 100644
 2--- a/src/test/util/setup_common.cpp
 3+++ b/src/test/util/setup_common.cpp
 4@@ -15,10 +15,10 @@
 5 #include <interfaces/chain.h>
 6 #include <net.h>
 7 #include <net_processing.h>
 8-#include <node/miner.h>
 9-#include <noui.h>
10 #include <node/blockstorage.h>
11 #include <node/chainstate.h>
12+#include <node/miner.h>
13+#include <noui.h>
14 #include <policy/fees.h>
15 #include <pow.h>
16 #include <rpc/blockchain.h>
17@@ -28,6 +28,7 @@
18 #include <script/sigcache.h>
19 #include <shutdown.h>
20 #include <streams.h>
21+#include <test/util/net.h>
22 #include <txdb.h>
23 #include <util/strencodings.h>
24 #include <util/string.h>
25@@ -227,7 +228,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
26                                                /*deterministic=*/false,
27                                                m_node.args->GetIntArg("-checkaddrman", 0));
28     m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
29-    m_node.connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests.
30+    m_node.connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests.
31     m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman,
32                                        m_node.banman.get(), *m_node.chainman,
33                                        *m_node.mempool, false);

dhruv commented at 5:12 pm on February 18, 2022: member

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

MarcoFalke referenced this in commit 6be319beb8 on Apr 16, 2022

MarcoFalke closed this on Apr 16, 2022

DrahtBot locked this on Apr 16, 2023

Test-only UB in fuzz tests #24373