signet: Fix uninitialized read in validation by maflcko · Pull Request #20035 · bitcoin/bitcoin

maflcko · 2020-09-29T12:37:42Z

No description provided.

maflcko · 2020-09-29T12:41:36Z

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?

theStack · 2020-09-29T12:54:57Z

Concept ACK

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

maflcko · 2020-09-29T13:01:28Z

unit256 are default initialized to all-zeros

theStack · 2020-09-29T13:24:31Z

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

practicalswift · 2020-09-29T19:47:12Z

@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 · 2020-09-30T04:32:51Z

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 · 2020-09-30T04:39:02Z

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

utACK

maflcko · 2020-09-30T12:53:03Z

Pushed some non-bugfix changes

DrahtBot · 2020-09-30T19:24:23Z

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

Conflicts

No conflicts as of last run.

kallewoof

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

practicalswift · 2020-10-01T06:37:55Z

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

kallewoof · 2020-10-01T07:03:09Z

@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.

maflcko · 2020-10-01T07:08:27Z

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

theStack

ACK fa82143e41c067e7d16d612c775701be94974d54 🩹

jonatack

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{};

src/chainparams.cpp

jonatack · 2020-10-01T08:27:59Z

Great find -- LGTM modulo the two comments

promag

Code review ACK fa82143e41c067e7d16d612c775701be94974d54.

src/chainparams.cpp

jonatack · 2020-10-01T11:28:15Z

Code review ACK fa82143e41c067e7d16d612c775701be94974d54

practicalswift · 2020-10-01T16:22:08Z

@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)

maflcko · 2020-10-01T16:26:17Z

It was found by reading/reviewing the code

michaelfolkson · 2020-10-06T12:52:23Z

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 · 2020-10-06T18:29:04Z

ACK fa82143e41c067e7d16d612c775701be94974d54: diff looks correct

laanwj · 2020-10-15T08:57:26Z

Sorry, needs rebase (probably with taproot changes)

maflcko · 2020-10-15T09:30:31Z

Rebased

kallewoof

Typo in 3rd commit message (unit256).

…citly

maflcko · 2020-10-16T04:32:54Z

Fixed typo in commit msg

practicalswift · 2020-10-16T06:07:47Z

re-ACK fa723e3: patch still looks correct

kallewoof

ReACK fa723e3

theStack

re-ACK fa723e3 🍐

fa723e3 Initialize default-initialized uint256 consensus params to zero explicitly (MarcoFalke) fa729cd doc: Move assumed-values doxygen comments to header (MarcoFalke) fa64892 signet: Fix uninitialized read in validation (MarcoFalke) Pull request description: ACKs for top commit: practicalswift: re-ACK fa723e3: patch still looks correct kallewoof: ReACK fa723e3 theStack: re-ACK fa723e3 🍐 Tree-SHA512: db562bcc15af23bbcbf485f0bbf7564c64c144a4368230fd7682e8861d9500f6f5240351e31c560140df43b2e8456eafd9d27d1e8dd682b20afcc279a39dc329

DrahtBot added the Validation label Sep 29, 2020

kallewoof approved these changes Sep 30, 2020

View reviewed changes

maflcko force-pushed the 2009-signetUninitRead branch from faff676 to fa82143 Compare September 30, 2020 12:52

This was referenced Sep 30, 2020

History for Taproot PR #19953 #19997

Closed

Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #19953

Merged

kallewoof reviewed Oct 1, 2020

View reviewed changes

theStack approved these changes Oct 1, 2020

View reviewed changes

jonatack reviewed Oct 1, 2020

View reviewed changes

src/chainparams.cpp Outdated Show resolved Hide resolved

promag reviewed Oct 1, 2020

View reviewed changes

src/chainparams.cpp Outdated Show resolved Hide resolved

maflcko added this to the 0.21.0 milestone Oct 9, 2020

MarcoFalke added 2 commits October 15, 2020 11:27

signet: Fix uninitialized read in validation

fa64892

doc: Move assumed-values doxygen comments to header

fa729cd

maflcko force-pushed the 2009-signetUninitRead branch from fa82143 to fa60f0d Compare October 15, 2020 09:30

kallewoof reviewed Oct 16, 2020

View reviewed changes

Initialize default-initialized uint256 consensus params to zero expli…

fa723e3

…citly

maflcko force-pushed the 2009-signetUninitRead branch from fa60f0d to fa723e3 Compare October 16, 2020 04:32

kallewoof reviewed Oct 16, 2020

View reviewed changes

theStack approved these changes Oct 16, 2020

View reviewed changes

laanwj merged commit 2947ae6 into bitcoin:master Oct 16, 2020

maflcko deleted the 2009-signetUninitRead branch October 16, 2020 10:39

practicalswift mentioned this pull request Nov 11, 2020

ci: Remove redundant valgrind fuzz task #20368

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

maflcko commented Sep 29, 2020

Uh oh!

maflcko commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theStack commented Sep 29, 2020

Uh oh!

maflcko commented Sep 29, 2020

Uh oh!

theStack commented Sep 29, 2020

Uh oh!

practicalswift commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Sep 30, 2020

Uh oh!

practicalswift commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 30, 2020

Uh oh!

DrahtBot commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Oct 1, 2020

Uh oh!

kallewoof commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Oct 1, 2020

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Oct 1, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Oct 1, 2020

Uh oh!

practicalswift commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Oct 1, 2020

Uh oh!

michaelfolkson commented Oct 6, 2020

Uh oh!

practicalswift commented Oct 6, 2020

Uh oh!

laanwj commented Oct 15, 2020

Uh oh!

maflcko commented Oct 15, 2020

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 16, 2020

Uh oh!

practicalswift commented Oct 16, 2020

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 29, 2020 •

edited

Loading

practicalswift commented Sep 29, 2020 •

edited

Loading

practicalswift commented Sep 30, 2020 •

edited

Loading

DrahtBot commented Sep 30, 2020 •

edited

Loading

kallewoof commented Oct 1, 2020 •

edited

Loading

practicalswift commented Oct 1, 2020 •

edited

Loading