Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix uninitialized variable nMinerConfirmationWindow #17449

Merged
merged 1 commit into from Nov 14, 2019

Conversation

@bitcoinVBR
Copy link

bitcoinVBR commented Nov 12, 2019

It is used for the computation of BIP9WarningHeight, and by that time it isn't initialized.

@wodry

This comment has been minimized.

Copy link
Contributor

wodry commented Nov 12, 2019

I find it scary, that a bug like this can be introduced. I am wondering if there are not any tools that check the code for references to uninitialized variables?

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 12, 2019

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

Conflicts

No conflicts as of last run.

@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 12, 2019

Fixes what? Both are already initialized..

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 12, 2019

Fixes what? Both are already initialized..

It's used for the computation of BIP9WarningHeight, and by that time it isn't initialized (I missed it first, too!).

I find it scary, that a bug like this can be introduced. I am wondering if there are not any tools that check the code for references to uninitialized variables?

The tests are being run in various sanitizers, and valgrind can do this IIRC. I'm surprised it wasn't caught.

ACK 6fcd798

Should fix #16697.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 12, 2019

@promag It looks like consensus.nMinerConfirmationWindow was indeed being read uninitialised on L74 prior to the change suggested by this PR?

consensus.MinBIP9WarningHeight = consensus.SegwitHeight + consensus.nMinerConfirmationWindow;

This code was introduced in fdb3e8f in PR #16713 which was merged in to master on September 27, 2019.

Really fascinating that this 1.) wasn't caught automatically by our static and dynamic analysis, 2.) wasn't caught by our review process and 3.) was unnoticed in master for over a month. Room for improvement! :)

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 12, 2019

@bitcoinVBR

Thanks a lot for reporting this! What an excellent first time contribution! Hope to see more great contributions from you.

Sorry about the initial misunderstanding in your original PR #17433 :)

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 12, 2019

Really fascinating that this 1.) wasn't caught automatically by our static and dynamic analysis,

Port to rust when? 😄

@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 12, 2019

Oh!

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 12, 2019

Worth noting is that the read of nMinerConfirmationWindow takes place on mainnet and testnet but not on regtest:

$ grep -E '(nMinerConfirmationWindow|CBaseChainParams|MinBIP9WarningHeight)' \
      src/chainparams.cpp | grep -A2 " = *CBaseChainParams"
        strNetworkID = CBaseChainParams::MAIN;
        consensus.MinBIP9WarningHeight = consensus.SegwitHeight + consensus.nMinerConfirmationWindow;
        consensus.nMinerConfirmationWindow = 2016; // nPowTargetTimespan / nPowTargetSpacing
        strNetworkID = CBaseChainParams::TESTNET;
        consensus.MinBIP9WarningHeight = consensus.SegwitHeight + consensus.nMinerConfirmationWindow;
        consensus.nMinerConfirmationWindow = 2016; // nPowTargetTimespan / nPowTargetSpacing
        strNetworkID =  CBaseChainParams::REGTEST;
        consensus.MinBIP9WarningHeight = 0;
        consensus.nMinerConfirmationWindow = 144; // Faster than normal for regtest (144 instead of 2016)
@theStack

This comment has been minimized.

Copy link
Contributor

theStack commented Nov 12, 2019

ACK 6fcd798
Really surprising that apparently no compiler spit out a warning about this uninitialized access...

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 12, 2019

Worth noting is that the read of nMinerConfirmationWindow takes place on mainnet and testnet but not on regtest:

Good catch. If there is no unit test that instantiates the main / testnet chain, that'd explain why sanitizer CI checks don't notice it. But it seems that quite a few do, for example:

pow_tests.cpp:    const auto consensus = CreateChainParams(CBaseChainParams::MAIN)->GetConsensus();
@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 12, 2019

@laanwj I think there might be a more subtle issue here too - note that MinBIPWarningHeight appears to have the correct value (481824 + 2016 == 483840) when reaching Condition(…) on mainnet: #16713 (comment).

I think we might be chasing two issues here where the uninitialised read is the first one :)

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 12, 2019

Good catch. If there is no unit test that instantiates the main / testnet chain, that'd explain why sanitizer CI checks don't notice it. But it seems that quite a few do, for example:

We have functional tests that run on the testnet chain. So this should have been caught by a sanitizer (assuming one was enabled)

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 12, 2019

I don't think we have the memory sanitizer enabled anywhere

@bitcoinVBR

This comment has been minimized.

Copy link
Author

bitcoinVBR commented Nov 12, 2019

@bitcoinVBR

Thanks a lot for reporting this! What an excellent first time contribution! Hope to see more great contributions from you.

Sorry about the initial misunderstanding in your original PR #17433 :)

Thanks! I'm happy to help. I actually only found this bug because of my work on BitcoinV. I'll continue to keep an eye out for anything fishy.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 12, 2019

Compiler writers get a lot of heat for exploiting UB to the fullest for optimisation purposes, but in this case it seems like we got really lucky -- they "fixed" our code (assuming Clang -O2) 😉

Look at the Clang -O2 case where we get the 483840 (481824 + 2016 == 483840) we intended (!) :)

$ clang++-8 -O0 -o repro repro.cpp && ./repro
consensus.MinBIP9WarningHeight[1] == 481824
consensus.MinBIP9WarningHeight[2] == 4678000
consensus.MinBIP9WarningHeight[3] == 481824
$ clang++-8 -O1 -o repro repro.cpp && ./repro
consensus.MinBIP9WarningHeight[1] == 586474128
consensus.MinBIP9WarningHeight[2] == 4678896
consensus.MinBIP9WarningHeight[3] == -1100057664
$ clang++-8 -O2 -o repro repro.cpp && ./repro
consensus.MinBIP9WarningHeight[1] == 483840
consensus.MinBIP9WarningHeight[2] == 483840
consensus.MinBIP9WarningHeight[3] == 483840
$ g++-8 -O0 -o repro repro.cpp && ./repro
consensus.MinBIP9WarningHeight[1] == 503884
consensus.MinBIP9WarningHeight[2] == 481824
consensus.MinBIP9WarningHeight[3] == 503884
$ g++-8 -O1 -o repro repro.cpp && ./repro
consensus.MinBIP9WarningHeight[1] == 481824
consensus.MinBIP9WarningHeight[2] == 481824
consensus.MinBIP9WarningHeight[3] == 481824
$ g++-8 -O2 -o repro repro.cpp && ./repro
consensus.MinBIP9WarningHeight[1] == 481824
consensus.MinBIP9WarningHeight[2] == 481824
consensus.MinBIP9WarningHeight[3] == 481824
$ cat repro.cpp
#include <cstdint>
#include <iostream>

namespace Consensus {
struct Params {
  int SegwitHeight;
  int MinBIP9WarningHeight;
  uint32_t nMinerConfirmationWindow;
};
} // namespace Consensus

class CChainParams {
public:
  const Consensus::Params &GetConsensus() const { return consensus; }

protected:
  CChainParams() {}

  Consensus::Params consensus;
};

class CMainParams : public CChainParams {
public:
  CMainParams() {
    consensus.SegwitHeight = 481824;
    consensus.MinBIP9WarningHeight =
        consensus.SegwitHeight + consensus.nMinerConfirmationWindow;
    consensus.nMinerConfirmationWindow = 2016;
  }
};

int main() {
  CMainParams main_params_1;
  std::cout << "consensus.MinBIP9WarningHeight[1] == "
            << main_params_1.GetConsensus().MinBIP9WarningHeight << std::endl;

  CMainParams main_params_2;
  std::cout << "consensus.MinBIP9WarningHeight[2] == "
            << main_params_2.GetConsensus().MinBIP9WarningHeight << std::endl;

  CMainParams main_params_3;
  std::cout << "consensus.MinBIP9WarningHeight[3] == "
            << main_params_3.GetConsensus().MinBIP9WarningHeight << std::endl;
}

:)

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 12, 2019

@jonatack The output you posted from the testing of PR #16713 in #16713 (comment) ...

2019-09-20T11:26:36Z SegwitHeight 481824
2019-09-20T11:26:36Z nMinerConfirmationWindow 2016
2019-09-20T11:26:36Z MinBIPWarningHeight 483840
2019-09-20T11:26:36Z pindex->nHeight 164836
2019-09-20T11:26:36Z pindex->nVersion 1
2019-09-20T11:26:36Z ComputeBlockVersion 536870912

... could it be the case that you happened to test with Clang with the project default optimisation level -O2? :) If so, could you retry using GCC (any optimisation level), or Clang with -O0 or -O1 and see if you get the same results?

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 12, 2019

@practicalswift I tried gcc 9.2.1 and it happened to optimize out the bug for me as well. Though running the same binary in valgrind still jells at me, so I can't make any sense out of that.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 12, 2019

@MarcoFalke What is the output of ./repro for binaries compiled at the different optimisation levels?

@jonatack

This comment has been minimized.

Copy link
Member

jonatack commented Nov 12, 2019

could it be the case that you happened to test with Clang with the project default optimisation level -O2? :) If so, could you retry using GCC (any optimisation level), or Clang with -O0 or -O1 and see if you get the same results?

It was with gcc version 8.3.0 (Debian 8.3.0-6) with no optimisation flags.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 12, 2019

It was with gcc version 8.3.0 (Debian 8.3.0-6) with no optimisation flags.

The default is O2 for Bitcoin Core

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Nov 12, 2019

I'd prefer to just hard code the MinBIP9WarningHeight, eg:

        consensus.MinBIP9WarningHeight = 483840; // segwit activation height + miner confirmation window

Hard-coding the value means that even if the member initializations are moved around again, nMinerConfirmationWindow will not be accessed before initialization.

MinBIP9WarningHeight is currently the only member variable that is set from the value of a different member, except the hashGenesisBlock which is checked with an assert against a hard-coded value.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 12, 2019

I was about to suggest that the members should be marked const, so that initialization and initialization order is enforced by the compiler. Though, I believe that makes the code less readable as C++ does not have named parameters that can be passed into a constructor.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 12, 2019

@bitcoinVBR Could you please adjust the patch as suggested by @jnewbery and then squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Nov 14, 2019

Code review ACK edb6b76. Nit: commit description has duplicate text.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 14, 2019

FWIW, Valgrind warns about this when running a bitcoind binary compiled from current master using GCC with -O0:

==19043== Thread 25 b-msghand:
==19043== Conditional jump or move depends on uninitialised value(s)
==19043==    at 0x49E12D: WarningBitsConditionChecker::Condition(CBlockIndex const*, Consensus::Params const&) const (validation.cpp:1815)
==19043==    by 0x541AED: 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)
==19043==    by 0x484248: UpdateTip(CBlockIndex const*, CChainParams const&) (validation.cpp:2379)
==19043==    by 0x4854A1: CChainState::ConnectTip(BlockValidationState&, CChainParams const&, CBlockIndex*, std::shared_ptr<CBlock const> const&, ConnectTrace&, DisconnectedBlockTransactions&) (validation.cpp:2584)
==19043==    by 0x485FD8: CChainState::ActivateBestChainStep(BlockValidationState&, CChainParams const&, CBlockIndex*, std::shared_ptr<CBlock const> const&, bool&, ConnectTrace&) (validation.cpp:2715)
==19043==    by 0x4866B2: CChainState::ActivateBestChain(BlockValidationState&, CChainParams const&, std::shared_ptr<CBlock const>) (validation.cpp:2836)
==19043==    by 0x48CFAC: ProcessNewBlock(CChainParams const&, std::shared_ptr<CBlock const>, bool, bool*) (validation.cpp:3792)
==19043==    by 0x21AA69: ProcessMessage(CNode*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CDataStream&, long, CChainParams const&, CConnman*, BanMan*, std::atomic<bool> const&) (net_processing.cpp:2974)
==19043==    by 0x21D80F: PeerLogicValidation::ProcessMessages(CNode*, std::atomic<bool>&) (net_processing.cpp:3331)
==19043==    by 0x1BDD83: CConnman::ThreadMessageHandler() (net.cpp:2013)
==19043==    by 0x202E2F: void std::__invoke_impl<void, void (CConnman::*&)(), CConnman*&>(std::__invoke_memfun_deref, void (CConnman::*&)(), CConnman*&) (invoke.h:73)
==19043==    by 0x1FE55C: std::__invoke_result<void (CConnman::*&)(), CConnman*&>::type std::__invoke<void (CConnman::*&)(), CConnman*&>(void (CConnman::*&)(), CConnman*&) (invoke.h:95)

@bitcoinVBR Thanks again for reporting! May I ask how you noticed this issue? :)

@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 14, 2019

@practicalswift he said how above #17449 (comment)

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 14, 2019

@promag Yes, I understood that it was found in the context of @bitcoinVBR's work on a fork, but I didn't get how it was found. More specifically if it was caught by manual source code review, static analysis or dynamic analysis :)

@bitcoinVBR

This comment has been minimized.

Copy link
Author

bitcoinVBR commented Nov 14, 2019

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 14, 2019

Are there any bounty rewards for these finds?

The Bitcoin Core project does not have any official bounty program AFAIK, but the is nothing stopping individuals from donating to other individuals who they think have done valuable security research.

(Aside: My personal view is that we could do a much better job recognising good security research: especially the non-glamorous kind of low-level security research. As an example: if we had a proper incentive structure in place I guess someone would be running bitcoind binaries compiled from master with -O{0,1,2} running under valgrind around the clock just waiting to report an issue like this. Incentives matter. Unfortunately I don't know the optimal incentive structure (the mix between public recognition vs official bug bounty vs donations vs just being polite and thanking researchers vs long-term funding, etc.), but I'm fairly certain we're not at the global optimum right now :))

Shameless plug: People interested in running bitcoind under valgrind might want to help out by reviewing #17455 :)

fanquake added a commit that referenced this pull request Nov 14, 2019
edb6b76 fix uninitialized variable nMinerConfirmationWindow (NullFunctor)

Pull request description:

  It is used for the computation of `BIP9WarningHeight`, and by that time it isn't initialized.

ACKs for top commit:
  jnewbery:
    utACK edb6b76
  promag:
    ACK edb6b76, commit description could be cleaned up though.
  MarcoFalke:
    ACK edb6b76, used python3 to do the addition locally 📍
  practicalswift:
    ACK edb6b76, used `clang++ -O2` on the previous version^W^W^W^W^W^W`bc` to verify the addition locally 🏓
  Sjors:
    Code review ACK  edb6b76. Nit: commit description has duplicate text.

Tree-SHA512: 6fa0be0ecfbfd5d537f2c5b4a9333c76530c1f3182f777330cc7939b0496e37b75d8f8810cdaf471a9bd3247b425f2e239578300dfa0d5a87cd14a6ccfafa619
@fanquake fanquake merged commit edb6b76 into bitcoin:master Nov 14, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Nov 14, 2019
fix uninitialized variable hard code the MinBIP9WarningHeight

fix uninitialized var hard code the MinBIP9WarningHeight instead

Github-Pull: #17449
Rebased-From: edb6b76
Tree-SHA512: 6192940e5e13ad1176aa380da9f3287ff1eb0c8c2a78571a6c45fe0e100417452c8503b9ffc5c8b2a89c4a5e8811b9d2bfec95366e1de00f3365ba06959e9a9a
laanwj added a commit that referenced this pull request Nov 14, 2019
Change version to 0.19.0.1.

Add the following two PRs:
- #17368 cli: fix -getinfo output when compiled with no wallet
- #17449 fix uninitialized variable nMinerConfirmationWindow

Add the following author:
- NullFunctor (bitcoinVBR on gh)

Tree-SHA512: f37b822555f2069c999721eede9156250d780e8906cd2e3294e7dfefc0225fff65ee3af4614fca081dcdccfab4a2a980192156621f005aa529bba00058da5c9a
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 15, 2019

Maybe you can get funding for such a thing from one of the exchanges or companies in the space, that's always how developer funding has worked, but the 'bitcoin core' project is a loose name for people contributing to an open source project, and does not manage funds, and should not manage funds, and will never have an "official incentive structure".

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 15, 2019

[…] the 'bitcoin core' project is a loose name for people contributing to an open source project, and does not manage funds, and should not manage funds, and will never have an "official incentive structure".

Agree 100% if we are talking monetary reward.

Note that incentive structure does not necessarily imply monetary reward: giving proper credit and treating researchers nicely goes a surprisingly long way when it comes to attracting security research :)

To bring this on-topic again (sorry, my fault!): I hope we can make this a good example by making sure @bitcoinVBR is properly credited for allowing us to ship 0.19.0 without an "optimisation unstable" UpdateTip(…) (due to the uninitialised read) :)

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 15, 2019

As all contributors to the release, they've been added to the authors list in 0.19.0.1 (as NullFunctor, as that's the name on their git mail address, @bitcoinVBR: if you want to be credited under a different name let me know) and this PR has been added to the changelog.

This is not a security issue so I'm not sure why you bring that up. It doesn't matter either, bugs are bugs. All people that contribute deserve credit.

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Nov 15, 2019

Backported to 0.19 in 6ec0dc1.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Nov 15, 2019

ACK on the rebase 6ec0dc1.

MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 17, 2019
@jonatack jonatack mentioned this pull request Nov 18, 2019
1 of 1 task complete
domob1812 added a commit to domob1812/namecore that referenced this pull request Nov 18, 2019
Updated the BIP9 warning height in chain params for Namecoin's segwit
activation height (see bitcoin/bitcoin#17449).
domob1812 added a commit to domob1812/namecore that referenced this pull request Nov 18, 2019
Updated the BIP9 warning height in chain params for Namecoin's segwit
activation height (see bitcoin/bitcoin#17449).
domob1812 added a commit to xaya/xaya that referenced this pull request Nov 18, 2019
Updated the BIP9 warning height in chain params for Xaya's segwit
activation height (see bitcoin/bitcoin#17449).
domob1812 added a commit to xaya/xaya that referenced this pull request Nov 18, 2019
Updated the BIP9 warning height in chain params for Xaya's segwit
activation height (see bitcoin/bitcoin#17449).

Fixes #101 through the upstream patch
in 50cd27e.
fxtc added a commit to fxtc/fxtc that referenced this pull request Nov 25, 2019
fix uninitialized variable hard code the MinBIP9WarningHeight

fix uninitialized var hard code the MinBIP9WarningHeight instead

Github-Pull: bitcoin#17449
Rebased-From: edb6b76
Tree-SHA512: 6192940e5e13ad1176aa380da9f3287ff1eb0c8c2a78571a6c45fe0e100417452c8503b9ffc5c8b2a89c4a5e8811b9d2bfec95366e1de00f3365ba06959e9a9a
fxtc added a commit to fxtc/fxtc that referenced this pull request Nov 25, 2019
Change version to 0.19.0.1.

Add the following two PRs:
- bitcoin#17368 cli: fix -getinfo output when compiled with no wallet
- bitcoin#17449 fix uninitialized variable nMinerConfirmationWindow

Add the following author:
- NullFunctor (bitcoinVBR on gh)

Tree-SHA512: f37b822555f2069c999721eede9156250d780e8906cd2e3294e7dfefc0225fff65ee3af4614fca081dcdccfab4a2a980192156621f005aa529bba00058da5c9a
fxtc added a commit to fxtc/fxtc that referenced this pull request Nov 25, 2019
fix uninitialized variable hard code the MinBIP9WarningHeight

fix uninitialized var hard code the MinBIP9WarningHeight instead

Github-Pull: bitcoin#17449
Rebased-From: edb6b76
Tree-SHA512: 6192940e5e13ad1176aa380da9f3287ff1eb0c8c2a78571a6c45fe0e100417452c8503b9ffc5c8b2a89c4a5e8811b9d2bfec95366e1de00f3365ba06959e9a9a
fxtc added a commit to fxtc/fxtc that referenced this pull request Nov 25, 2019
Change version to 0.19.0.1.

Add the following two PRs:
- bitcoin#17368 cli: fix -getinfo output when compiled with no wallet
- bitcoin#17449 fix uninitialized variable nMinerConfirmationWindow

Add the following author:
- NullFunctor (bitcoinVBR on gh)

Tree-SHA512: f37b822555f2069c999721eede9156250d780e8906cd2e3294e7dfefc0225fff65ee3af4614fca081dcdccfab4a2a980192156621f005aa529bba00058da5c9a
@ajtowns

This comment has been minimized.

Copy link
Contributor

ajtowns commented Nov 26, 2019

consensus.MinBIP9WarningHeight = consensus.SegwitHeight + consensus.nMinerConfirmationWindow;

Really fascinating that this 1.) wasn't caught automatically by our static and dynamic analysis,

Looks like -Werror=uninitialized is able to catch this, but only if the initialisation is done in a Consensus::Params constructor via a member initializer list rather than a code block (and -O3 is specified if using g++). (I thought I was getting g++ to catch it in code blocks as well, but can't duplicate that now)

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 26, 2019

@ajtowns Worth mentioning from a dynamic analysis perspective is that both Valgrind and MemorySanitizer (-fsanitize=memory) catch this too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.