signet: Fix uninitialized read in validation #20035

MarcoFalke commented at 12:37 PM on September 29, 2020: member

No description provided.

MarcoFalke commented at 12:41 PM on September 29, 2020: member

Too sad that teaching the compiler to warn about this would make the code massively verbose, but maybe it is worth a shot now that this is the second time this year?

<!-- ==2844273== Thread 24 b-msghand: ==2844273== Conditional jump or move depends on uninitialised value(s) ==2844273== at 0x5335DD: WarningBitsConditionChecker::Condition(CBlockIndex const*, Consensus::Params const&) const (validation.cpp:1860) ==2844273== by 0x54A4B1: AbstractThresholdConditionChecker::GetStateFor(CBlockIndex const*, Consensus::Params const&, std::map<CBlockIndex const*, ThresholdState, std::less<CBlockIndex const*>, std::allocator<std::pair<CBlockIndex const* const, ThresholdState> > >&) const (versionbits.cpp:70) ==2844273== by 0x50CE03: UpdateTip(CTxMemPool&, CBlockIndex const*, CChainParams const&) (validation.cpp:2458) ==2844273== by 0x50E39C: CChainState::ConnectTip(BlockValidationState&, CChainParams const&, CBlockIndex*, std::shared_ptr<CBlock const> const&, ConnectTrace&, DisconnectedBlockTransactions&) (validation.cpp:2647) ==2844273== by 0x50F82A: CChainState::ActivateBestChainStep(BlockValidationState&, CChainParams const&, CBlockIndex*, std::shared_ptr<CBlock const> const&, bool&, ConnectTrace&) (validation.cpp:2779) ==2844273== by 0x511B03: CChainState::ActivateBestChain(BlockValidationState&, CChainParams const&, std::shared_ptr<CBlock const>) (validation.cpp:2906) ==2844273== by 0x51C13E: ChainstateManager::ProcessNewBlock(CChainParams const&, std::shared_ptr<CBlock const>, bool, bool*) (validation.cpp:3862) ==2844273== by 0x3241C4: PeerManager::ProcessMessage(CNode&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CDataStream&, std::chrono::duration<long, std::ratio<1l, 1000000l> >, std::atomic<bool> const&) (net_processing.cpp:3506) ==2844273== by 0x3324E4: PeerManager::ProcessMessages(CNode*, std::atomic<bool>&) (net_processing.cpp:3894) ==2844273== by 0x333C7A: non-virtual thunk to PeerManager::ProcessMessages(CNode*, std::atomic<bool>&) (list.tcc:0) ==2844273== by 0x2F5A43: CConnman::ThreadMessageHandler() (net.cpp:2125) ==2844273== by 0x30B97D: __invoke_impl<void, void (CConnman::*&)(), CConnman *&> (invoke.h:73) ==2844273== by 0x30B97D: __invoke<void (CConnman::*&)(), CConnman *&> (invoke.h:95) ==2844273== by 0x30B97D: __call<void, 0> (functional:416) ==2844273== by 0x30B97D: operator()<, void> (functional:499) ==2844273== by 0x30B97D: __invoke_impl<void, std::_Bind<void (CConnman::*(CConnman *))()> &> (invoke.h:60) ==2844273== by 0x30B97D: __invoke_r<void, std::_Bind<void (CConnman::*(CConnman *))()> &> (invoke.h:110) ==2844273== by 0x30B97D: std::_Function_handler<void (), std::_Bind<void (CConnman::*(CConnman*))()> >::_M_invoke(std::_Any_data const&) (std_function.h:291) ==2844273==

theStack commented at 12:54 PM on September 29, 2020: member

Concept ACK

The parameters BIP16Exception and BIP34Hash are also not initialized for Signet?

MarcoFalke commented at 1:01 PM on September 29, 2020: member

unit256 are default initialized to all-zeros

theStack commented at 1:24 PM on September 29, 2020: member

unit256 are default initialized to all-zeros

Right, in those case there is no uninitialized read.

Still, looking at mainnet, testnet and regtest, without exception all parameters (i.e. all 22 fields of struct Params) are explicitely initialized -- hence I think for signet and all future *nets the same should be done. Both for consistency and do avoid such uninitialized read errors in the feature. The following params are not set for signet, i.e. uninitialized or default initialized:

MinBIP9WarningHeight (fixed in this PR)
nMinimumChainWork
defaultAssumeValid
BIP16Exception
BIP34Hash

DrahtBot added the label Validation on Sep 29, 2020

practicalswift commented at 7:47 PM on September 29, 2020: contributor

@MarcoFalke Nice catch! How did you find this one?

It's always a bit sad to see unitialized reads enter our code base without being caught automatically at CI stage by either dynamic analysis (MSAN or Valgrind) or static analysis :(

practicalswift commented at 4:32 AM on September 30, 2020: contributor

The UUM takes place in WarningBitsConditionChecker::Condition which is executed in the message handler thread:

$ valgrind --exit-on-first-error=yes --track-origins=yes --suppressions=contrib/valgrind.supp src/bitcoind -signet
…
==9443== Thread 23 b-msghand:
==9443== Conditional jump or move depends on uninitialised value(s)
==9443==    at 0x6F3E65: WarningBitsConditionChecker::Condition(CBlockIndex const*, Consensus::Params const&) const (validation.cpp:1862)
==9443==    by 0x744269: AbstractThresholdConditionChecker::GetStateFor(CBlockIndex const*, Consensus::Params const&, std::map<CBlockIndex const*, ThresholdState, std::less<CBlockIndex const*>, std::allocator<std::pair<CBlockIndex const* const, ThresholdState> > >&) const (versionbits.cpp:70)
==9443==    by 0x6D77FA: UpdateTip(CTxMemPool&, CBlockIndex const*, CChainParams const&) (validation.cpp:2458)
==9443==    by 0x6D8B64: CChainState::ConnectTip(BlockValidationState&, CChainParams const&, CBlockIndex*, std::shared_ptr<CBlock const> const&, ConnectTrace&, DisconnectedBlockTransactions&) (validation.cpp:2649)
==9443==    by 0x6D9712: CChainState::ActivateBestChainStep(BlockValidationState&, CChainParams const&, CBlockIndex*, std::shared_ptr<CBlock const> const&, bool&, ConnectTrace&) (validation.cpp:2781)
==9443==    by 0x6D9E8F: CChainState::ActivateBestChain(BlockValidationState&, CChainParams const&, std::shared_ptr<CBlock const>) (validation.cpp:2908)
==9443==    by 0x6E089E: ChainstateManager::ProcessNewBlock(CChainParams const&, std::shared_ptr<CBlock const>, bool, bool*) (validation.cpp:3864)
==9443==    by 0x3FADD3: PeerManager::ProcessMessage(CNode&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CDataStream&, std::chrono::duration<long, std::ratio<1l, 1000000l> >, std::atomic<bool> const&) (net_processing.cpp:3506)
==9443==    by 0x3FDA75: PeerManager::ProcessMessages(CNode*, std::atomic<bool>&) (net_processing.cpp:3864)
==9443==    by 0x390065: CConnman::ThreadMessageHandler() (net.cpp:2155)
==9443==    by 0x3E018D: void std::__invoke_impl<void, void (CConnman::*&)(), CConnman*&>(std::__invoke_memfun_deref, void (CConnman::*&)(), CConnman*&) (invoke.h:73)
==9443==    by 0x3DB61C: std::__invoke_result<void (CConnman::*&)(), CConnman*&>::type std::__invoke<void (CConnman::*&)(), CConnman*&>(void (CConnman::*&)(), CConnman*&) (invoke.h:95)
==9443==  Uninitialised value was created by a heap allocation
==9443==    at 0x4C3052A: operator new(unsigned long) (vg_replace_malloc.c:342)
==9443==    by 0xA1C134: CreateChainParams(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (chainparams.cpp:495)
==9443==    by 0xA1C31B: SelectParams(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (chainparams.cpp:505)
==9443==    by 0x30ABB6: AppInit(int, char**) (bitcoind.cpp:82)
==9443==    by 0x30B53E: main (bitcoind.cpp:172)

practicalswift commented at 4:39 AM on September 30, 2020: contributor

Unfortunately it seems like this UUM is not triggered by the code paths exercised by feature_signet.py. Covering the triggering code path in the functional test would be nice as part of this fix :)

$ test/functional/test_runner.py --valgrind --timeout-factor=0 feature_signet

TEST              | STATUS    | DURATION

feature_signet.py | ✓ Passed  | 151 s

ALL               | ✓ Passed  | 151 s (accumulated) 
Runtime: 151 s

kallewoof approved

kallewoof commented at 7:05 AM on September 30, 2020: member

utACK

MarcoFalke force-pushed on Sep 30, 2020

MarcoFalke commented at 12:53 PM on September 30, 2020: member

Pushed some non-bugfix changes

DrahtBot commented at 7:24 PM on September 30, 2020: member

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

Conflicts

No conflicts as of last run.

kallewoof commented at 4:43 AM on October 1, 2020: member

re-ACK. Agree with moving chain work / assume valid comments.

practicalswift commented at 6:37 AM on October 1, 2020: contributor

@kallewoof @MarcoFalke Have someone investigated why this UUM appear not to be exercised in the functional tests?

kallewoof commented at 7:03 AM on October 1, 2020: member

@practicalswift @MarcoFalke I made a minimal test that fails on master: https://github.com/kallewoof/bitcoin/tree/202010-signet-uuv-min9-test

test/validation_block_tests.cpp:379: error: in "validation_block_tests/chainparameters": check chain->GetConsensus().MinBIP9WarningHeight == 0 has failed [809056355 != 0]
2020-10-01T06:51:08.729020Z [scheduler] scheduler thread exit
test/validation_block_tests.cpp:376: Leaving test case "chainparameters"; testing time: 79044us
test/validation_block_tests.cpp:33: Leaving test suite "validation_block_tests"; testing time: 13338900us
test/validation_chainstate_tests.cpp:16: Test suite "validation_chainstate_tests" is skipped because disabled
[...]
wallet/test/scriptpubkeyman_tests.cpp:13: Test suite "scriptpubkeyman_tests" is skipped because disabled
Leaving test module "Bitcoin Core Test Suite"; testing time: 13340245us

*** 1 failure is detected in the test module "Bitcoin Core Test Suite"

The test succeeds on top of this PR.

MarcoFalke commented at 7:08 AM on October 1, 2020: member

The versionbits condition isn't evaluated for the first retarget period after the genesis block, so no test hits it.

theStack approved

theStack commented at 7:11 AM on October 1, 2020: member

ACK fa82143e41c067e7d16d612c775701be94974d54 🩹

jonatack commented at 8:22 AM on October 1, 2020: member

fa82143e maybe also change

--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -365,7 +365,7 @@ public:
         consensus.nSubsidyHalvingInterval = 150;
         consensus.BIP16Exception = uint256();
         consensus.BIP34Height = 500; // BIP34 activated on regtest (Used in functional tests)
-        consensus.BIP34Hash = uint256();
+        consensus.BIP34Hash = uint256{};

in src/chainparams.cpp:327 in fa82143e41 outdated

 314 | @@ -317,6 +315,7 @@ class SigNetParams : public CChainParams {
 315 |          consensus.fPowNoRetargeting = false;
 316 |          consensus.nRuleChangeActivationThreshold = 1916;
 317 |          consensus.nMinerConfirmationWindow = 2016;
 318 | +        consensus.MinBIP9WarningHeight = 0;

jonatack commented at 8:27 AM on October 1, 2020:

fadb384e perhaps group MinBIP9WarningHeight with the other height settings as done in the other three chains

--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -309,13 +309,13 @@ public:
        consensus.BIP66Height = 1;
        consensus.CSVHeight = 1;
        consensus.SegwitHeight = 1;
+        consensus.MinBIP9WarningHeight = 0;
        consensus.nPowTargetTimespan = 14 * 24 * 60 * 60; // two weeks
        consensus.nPowTargetSpacing = 10 * 60;
        consensus.fPowAllowMinDifficultyBlocks = false;
        consensus.fPowNoRetargeting = false;
        consensus.nRuleChangeActivationThreshold = 1916;
        consensus.nMinerConfirmationWindow = 2016;
-        consensus.MinBIP9WarningHeight = 0;

MarcoFalke commented at 8:32 AM on October 1, 2020:

I've put it in the line right before powLimit, like in all other chain constructors

jonatack commented at 8:27 AM on October 1, 2020: member

Great find -- LGTM modulo the two comments

in src/chainparams.cpp:398 in fa82143e41 outdated

 381 | @@ -378,8 +382,8 @@ class CRegTestParams : public CChainParams {
 382 |          consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 0;
 383 |          consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
 384 |  
 385 | -        consensus.nMinimumChainWork = uint256S("0x00");

promag commented at 8:43 AM on October 1, 2020:

fa82143e41c067e7d16d612c775701be94974d54

Not sure what is the motivation of being explicit here, I'd just remove these cases.

MarcoFalke commented at 8:58 AM on October 1, 2020:

#20035 (comment)

promag commented at 8:44 AM on October 1, 2020: member

Code review ACK fa82143e41c067e7d16d612c775701be94974d54.

jonatack commented at 11:28 AM on October 1, 2020: member

Code review ACK fa82143e41c067e7d16d612c775701be94974d54

practicalswift commented at 4:22 PM on October 1, 2020: contributor

@MarcoFalke

Want to share how you found this one (great find!)? :) Was it found by static analysis or dynamic analysis? Automatic testing or manual testing?

I'm thinking about what we can learn from this UUM case. More specifically I'm thinking about potential ways to improve manual and/or automated processes to catch UUM:s pre-merge (in the spirit of #18288, #17633, #18159, #18166 and similar attempts to kill this bug class for good) :)

FWIW, some previous UUM cases if someone wants to analyse specific instances of this bug class more in-depth:

2020: Use of uninitialized memory in Erlay networking code - found pre-merge
2020: Use of uninitialized memory in BIP324 encrypted p2p transport de-/serializer code (truth in advertising: I haven't verified this one by writing a PoC) - found pre-merge
2020: util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return
2019: Use of uninitialized memory in networking code when receiving a transaction we already have - found post-merge
2019: wallet: Uninitialized read in bumpfee(…)
2018: wallet: Fix non-determinism in ParseHDKeypath(...). Avoid using an uninitialized variable in path calculation.
2018: wallet: Fix use of uninitialized value bnb_used in CWallet::CreateTransaction(...)
2017: [net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest&)
2017: [test] Avoid reading a potentially uninitialized variable in tx_invalid-test (transaction_tests.cpp)

MarcoFalke commented at 4:26 PM on October 1, 2020: member

It was found by reading/reviewing the code

michaelfolkson commented at 12:52 PM on October 6, 2020: contributor

Concept ACK, Approach ACK

Haven't looked into @kallewoof test but presumably that will be in a follow up PR.

Thanks for sharing resources too @practicalswift. Added to a StackExchange post.

practicalswift commented at 6:29 PM on October 6, 2020: contributor

ACK fa82143e41c067e7d16d612c775701be94974d54: diff looks correct

MarcoFalke added this to the milestone 0.21.0 on Oct 9, 2020

laanwj commented at 8:57 AM on October 15, 2020: member

Sorry, needs rebase (probably with taproot changes)

signet: Fix uninitialized read in validation fa64892b82

doc: Move assumed-values doxygen comments to header fa729cdb2c

MarcoFalke force-pushed on Oct 15, 2020

MarcoFalke commented at 9:30 AM on October 15, 2020: member

Rebased

kallewoof commented at 3:43 AM on October 16, 2020: member

Typo in 3rd commit message (unit256).

Initialize default-initialized uint256 consensus params to zero explicitly fa723e3d43

MarcoFalke force-pushed on Oct 16, 2020

MarcoFalke commented at 4:32 AM on October 16, 2020: member

Fixed typo in commit msg

practicalswift commented at 6:07 AM on October 16, 2020: contributor

re-ACK fa723e3d43e63e8424d97d21d8f2cc8136aba206: patch still looks correct

kallewoof commented at 6:47 AM on October 16, 2020: member

ReACK fa723e3d43e63e8424d97d21d8f2cc8136aba206

theStack approved

theStack commented at 8:15 AM on October 16, 2020: member

re-ACK fa723e3d43e63e8424d97d21d8f2cc8136aba206 🍐

laanwj merged this on Oct 16, 2020

laanwj closed this on Oct 16, 2020

MarcoFalke deleted the branch on Oct 16, 2020

sidhujag referenced this in commit 0b2a28fa79 on Oct 16, 2020

DrahtBot locked this on Feb 15, 2022