Some recently introduced Coverity warnings (WRAPPER_ESCAPE, UNINIT and FORWARD_NULL) #19372

DISCLAIMER: These results have not been thoroughly confirmed, and may be improbable or flat out invalid, but it's worth having a list of these somewhere.

Disclaimer stolen from kallewoof's issue #9573 ("6696b46: Clang Static Analyzer Report") :)

Below are some recently introduced Coverity warnings of type WRAPPER_ESCAPE (Memory - illegal accesses), UNINIT (Uninitialized variables) and FORWARD_NULL (Null pointer dereferences) that might be investigating. I've excluded warnings in GUI code.

Note that a static analyzer warning does not necessarily imply a defect: it is just a warning from an automated tool that something might be worth investigating manually. Don't panic :)

*** CID 360095:  Memory - illegal accesses  (WRAPPER_ESCAPE)
/src/test/pow_tests.cpp: 121 in pow_tests::GetBlockProofEquivalentTime_test::test_method()()
115     
116     BOOST_AUTO_TEST_CASE(GetBlockProofEquivalentTime_test)
117     {
118         const auto chainParams = CreateChainParams(CBaseChainParams::MAIN);
119         std::vector<CBlockIndex> blocks(10000);
120         for (int i = 0; i < 10000; i++) {
>>>     CID 360095:  Memory - illegal accesses  (WRAPPER_ESCAPE)
>>>     The internal representation of local "blocks" escapes into "blocks[i].pprev", but is destroyed when it exits scope.
121             blocks[i].pprev = i ? &blocks[i - 1] : nullptr;
122             blocks[i].nHeight = i;
123             blocks[i].nTime = 1269211443 + i * chainParams->GetConsensus().nPowTargetSpacing;
124             blocks[i].nBits = 0x207fffff; /* target 0x7fffff000... */
125             blocks[i].nChainWork = i ? blocks[i - 1].nChainWork + GetBlockProof(blocks[i - 1]) : arith_uint256(0);
126         }

*** CID 360094:  Memory - illegal accesses  (WRAPPER_ESCAPE)
/src/test/skiplist_tests.cpp: 66 in skiplist_tests::getlocator_test::test_method()()
60         // Build a branch that splits off at block 49999, 50000 blocks long.
61         std::vector<uint256> vHashSide(50000);
62         std::vector<CBlockIndex> vBlocksSide(50000);
63         for (unsigned int i=0; i<vBlocksSide.size(); i++) {
64             vHashSide[i] = ArithToUint256(i + 50000 + (arith_uint256(1) << 128)); // Add 1<<128 to the hashes, so GetLow64() still returns the height.
65             vBlocksSide[i].nHeight = i + 50000;
>>>     CID 360094:  Memory - illegal accesses  (WRAPPER_ESCAPE)
>>>     The internal representation of local "vBlocksSide" escapes into "vBlocksSide[i].pprev", but is destroyed when it exits scope.
66             vBlocksSide[i].pprev = i ? &vBlocksSide[i - 1] : (vBlocksMain.data()+49999);
67             vBlocksSide[i].phashBlock = &vHashSide[i];
68             vBlocksSide[i].BuildSkip();
69             BOOST_CHECK_EQUAL((int)UintToArith256(vBlocksSide[i].GetBlockHash()).GetLow64(), vBlocksSide[i].nHeight);
70             BOOST_CHECK(vBlocksSide[i].pprev == nullptr || vBlocksSide[i].nHeight == vBlocksSide[i].pprev->nHeight + 1);
71         }

*** CID 360089:  Uninitialized variables  (UNINIT)
/src/bench/hashpadding.cpp: 40 in RegularPadded(benchmark::State &)()
34     
35         // Setup the salted hasher
36         uint256 nonce = GetRandHash();
37         uint256 data = GetRandHash();
38         while (state.KeepRunning()) {
39             unsigned char out[32];
>>>     CID 360089:  Uninitialized variables  (UNINIT)
>>>     Using uninitialized value "hasher". Field "hasher.buf" is uninitialized.
40             CSHA256 h = hasher;
41             h.Write(nonce.begin(), 32);
42             h.Write(data.begin(), 32);
43             h.Finalize(out);
44         }
45     }
46     

*** CID 360085:    (WRAPPER_ESCAPE)
/src/wallet/wallet.cpp: 4350 in CWallet::SetupLegacyScriptPubKeyMan()()
4344             return;
4345         }
4346     
4347         auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new LegacyScriptPubKeyMan(*this));
4348         for (const auto& type : OUTPUT_TYPES) {
4349             m_internal_spk_managers[type] = spk_manager.get();
>>>     CID 360085:    (WRAPPER_ESCAPE)
>>>     The internal representation of local "spk_manager" escapes into "this->m_external_spk_managers[type]", but is destroyed when it exits scope.
4350             m_external_spk_managers[type] = spk_manager.get();
4351         }
4352         m_spk_managers[spk_manager->GetID()] = std::move(spk_manager);
4353     }
4354     
4355     const CKeyingMaterial& CWallet::GetEncryptionKey() const
/src/wallet/wallet.cpp: 4349 in CWallet::SetupLegacyScriptPubKeyMan()()
4343         if (!m_internal_spk_managers.empty() || !m_external_spk_managers.empty() || !m_spk_managers.empty() || IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
4344             return;
4345         }
4346     
4347         auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new LegacyScriptPubKeyMan(*this));
4348         for (const auto& type : OUTPUT_TYPES) {
>>>     CID 360085:    (WRAPPER_ESCAPE)
>>>     The internal representation of local "spk_manager" escapes into "this->m_internal_spk_managers[type]", but is destroyed when it exits scope.
4349             m_internal_spk_managers[type] = spk_manager.get();
4350             m_external_spk_managers[type] = spk_manager.get();
4351         }
4352         m_spk_managers[spk_manager->GetID()] = std::move(spk_manager);
4353     }
4354     

*** CID 360079:  Memory - illegal accesses  (WRAPPER_ESCAPE)
/src/test/skiplist_tests.cpp: 152 in skiplist_tests::findearliestatleast_edge_test::test_method()()
146     {
147         std::list<CBlockIndex> blocks;
148         for (const unsigned int timeMax : {100, 100, 100, 200, 200, 200, 300, 300, 300}) {
149             CBlockIndex* prev = blocks.empty() ? nullptr : &blocks.back();
150             blocks.emplace_back();
151             blocks.back().nHeight = prev ? prev->nHeight + 1 : 0;
>>>     CID 360079:  Memory - illegal accesses  (WRAPPER_ESCAPE)
>>>     The internal representation of local "blocks" escapes into "blocks.back().pprev", but is destroyed when it exits scope.
152             blocks.back().pprev = prev;
153             blocks.back().BuildSkip();
154             blocks.back().nTimeMax = timeMax;
155         }
156     
157         CChain chain;

*** CID 360078:  Memory - illegal accesses  (WRAPPER_ESCAPE)
/src/test/skiplist_tests.cpp: 53 in skiplist_tests::getlocator_test::test_method()()
47         // Build a main chain 100000 blocks long.
48         std::vector<uint256> vHashMain(100000);
49         std::vector<CBlockIndex> vBlocksMain(100000);
50         for (unsigned int i=0; i<vBlocksMain.size(); i++) {
51             vHashMain[i] = ArithToUint256(i); // Set the hash equal to the height, so we can quickly check the distances.
52             vBlocksMain[i].nHeight = i;
>>>     CID 360078:  Memory - illegal accesses  (WRAPPER_ESCAPE)
>>>     The internal representation of local "vBlocksMain" escapes into "vBlocksMain[i].pprev", but is destroyed when it exits scope.
53             vBlocksMain[i].pprev = i ? &vBlocksMain[i - 1] : nullptr;
54             vBlocksMain[i].phashBlock = &vHashMain[i];
55             vBlocksMain[i].BuildSkip();
56             BOOST_CHECK_EQUAL((int)UintToArith256(vBlocksMain[i].GetBlockHash()).GetLow64(), vBlocksMain[i].nHeight);
57             BOOST_CHECK(vBlocksMain[i].pprev == nullptr || vBlocksMain[i].nHeight == vBlocksMain[i].pprev->nHeight + 1);
58         }

*** CID 360077:  Null pointer dereferences  (FORWARD_NULL)
/src/net_processing.cpp: 1862 in ProcessHeadersMessage(CNode &, CConnman *, ChainstateManager &, CTxMemPool &, const std::vector<CBlockHeader, std::allocator<CBlockHeader>> &, const CChainParams &, bool)()
1856                     pindexWalk = pindexWalk->pprev;
1857                 }
1858                 // If pindexWalk still isn't on our main chain, we're looking at a
1859                 // very large reorg at a time we think we're close to caught up to
1860                 // the main chain -- this shouldn't really happen.  Bail out on the
1861                 // direct fetch and rely on parallel download instead.
>>>     CID 360077:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "pindexWalk" to "Contains", which dereferences it.
1862                 if (!::ChainActive().Contains(pindexWalk)) {
1863                     LogPrint(BCLog::NET, "Large reorg, won't direct fetch to %s (%d)\n",
1864                             pindexLast->GetBlockHash().ToString(),
1865                             pindexLast->nHeight);
1866                 } else {
1867                     std::vector<CInv> vGetData;

Not sure what to do here. Except for the last one they seem all bugs in the static analyser. I recommend filing bugs upstream.

For the last one, it might be preferable to assert on the pindex to avoid forwarding a nullptr

Below are some recent Coverity warnings. I've excluded warnings in GUI code. Usual disclaimers apply :)

*** CID 360898:  Uninitialized members  (UNINIT_CTOR)
/src/rpc/util.cpp: 436 in RPCHelpMan::RPCHelpMan(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::vector<RPCArg, std::allocator<RPCArg>>, RPCResults, RPCExamples)()
430             }
431             return ret;
432         }
433     };
434     
435     RPCHelpMan::RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples)
>>>     CID 360898:  Uninitialized members  (UNINIT_CTOR)
>>>     Non-static class member field "m_results.m_results" is not initialized in this constructor nor in any functions that it calls.
436         : RPCHelpMan{std::move(name), std::move(description), std::move(args), std::move(results), std::move(examples), nullptr} {}
437     
438     RPCHelpMan::RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples, RPCMethodImpl fun)
439         : m_name{std::move(name)},
440           m_fun{std::move(fun)},
441           m_description{std::move(description)},

*** CID 361023:  Uninitialized members  (UNINIT_CTOR)
/src/net.cpp: 2275 in CConnman::CConnman(unsigned long, unsigned long, bool)()
2269     {
2270         SetTryNewOutboundPeer(false);
2271     
2272         Options connOptions;
2273         Init(connOptions);
2274         SetNetworkActive(network_active);
>>>     CID 361023:  Uninitialized members  (UNINIT_CTOR)
>>>     Non-static class member "fMsgProcWake" is not initialized in this constructor nor in any functions that it calls.
2275     }
2276     
2277     NodeId CConnman::GetNewNodeId()
2278     {
2279         return nLastNodeId.fetch_add(1, std::memory_order_relaxed);
2280     }

The first one is another false positive and the second one is never read uninitialized. Though, it could be initialized to the same value that it is set in Start()

@MarcoFalke I should probably add that these are unfiltered Coverity reports so false positives are unfortunately expected (as with most static analyzer tools).

My idea is to make this issue a tracking issue where new static analyser warnings are posted say once per month effectively mimicking how the static analyser part works in Intel's 0-DAY CI Kernel Test Service (used in the Linux kernel project). With the difference that this would be low volume and monthly (instead of the super high volume and immediate kbuild-all@lists.01.org list :)).

We would have caught some nasty bugs in our code base earlier if we'd gotten more eyes on the Coverity reports. Anecdotal evidence: I've tried to keep track of Coverity reports for the past three/four years, but despite that I've sometimes accidentally skipped some reports only to find out that an issue I later found using another method (say fuzzing, manual review, dynamic analysis, another static analysis tool, etc.) was reported in my inbox all along thanks to Coverity :)