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

Avoid dividing by zero in EstimateMedianVal (policy)/ConnectTip (validation)/CreateTransaction (wallet) #14239

Conversation

Projects
None yet
10 participants
@practicalswift
Copy link
Member

commented Sep 17, 2018

Avoid diving by zero in:

  • EstimateMedianVal (policy)
  • ConnectTip (validation)
  • CreateTransaction (wallet)

These are real nontheoretical cases in the sense that they are easily reachable.

Steps to reproduce:

./configure --with-sanitizers=undefined && make check && ./test/functional/test_runner.py
@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2018

@fanquake Refactoring? :-)

@practicalswift practicalswift changed the title Avoid undefined behaviour in EstimateMedianVal (policy)/ConnectTip (validation)/CreateTransaction (wallet) Avoid UB (divide by zero) in EstimateMedianVal (policy)/ConnectTip (validation)/CreateTransaction (wallet) Sep 17, 2018

@practicalswift practicalswift changed the title Avoid UB (divide by zero) in EstimateMedianVal (policy)/ConnectTip (validation)/CreateTransaction (wallet) Avoid dividing by zero (undefined behaviour) in EstimateMedianVal (policy)/ConnectTip (validation)/CreateTransaction (wallet) Sep 17, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 17, 2018

Concept ACK. Could enable the functional tests in travis in this commit?

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2018

@MarcoFalke Unfortunately it turned out that these were not the only undefined behaviours we're triggering in the functional tests. I'll submit a separate PR that enables the functional tests with suppressions for everything we're triggering :-)

Show resolved Hide resolved src/validation.cpp Outdated
Show resolved Hide resolved src/policy/fees.cpp Outdated
@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2018

Rebased! :-)

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

@fanquake Is refactoring really the correct label for this PR?

Definitions:

  1. Code refactoring is defined as the process of restructuring existing computer code without changing its external behaviour.
  2. Triggering undefined behaviour is defined as executing computer code whose behaviour is not prescribed by the language specification to which the code adheres, for the current state of the program.

Since undefined behaviour is a superset of external behaviour it follows that a PR fixing UB cannot be considered refactoring, no? :-)

I suggest labelling all UB fixes with the bug label so that they get the attention they deserve.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2018

@fanquake Ping? :-)

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

@fanquake I'm afraid the "refactoring" label is causing the UB:s (this and others) to go under the radar and thus remain unfixed.

Please consider giving the UB PR:s a more appropriate label :-) Please at least talk to me my fellow contributor – I'm your friend :-)

@sipa sipa removed the Refactoring label Oct 20, 2018

MarcoFalke added a commit that referenced this pull request Nov 5, 2018

Merge #14252: build: Run functional tests and benchmarks under the un…
…defined behaviour sanitizer (UBSan)

9f49db7 Enable functional tests in UBSAN job. Enable -fsanitize=integer (part of UBSAN). Merge UBSAN Travis job with no depends. (practicalswift)

Pull request description:

  Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan).

  This will make Travis automatically detect issues such as:
  * #14242: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)`
  * #14239: Avoid dividing by zero (undefined behaviour) in `EstimateMedianVal` (policy)/`ConnectTip` (validation)/`CreateTransaction` (wallet)
  * #13546: wallet: Avoid potential use of uninitialized value `bnb_used` in `CWallet::CreateTransaction(...)`

  Addresses issue #14059.

Tree-SHA512: 285e1542b36c582516c47938ce8d999fd89ba6c867bc0976e7306e7c949b8b84ffbfa43dbc679dd97ae639b086092e7d799d8e1c903c66a37d529ce61d5c64b4

@bitcoin bitcoin deleted a comment from DrahtBot Nov 5, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

Could remove the suppression from the file in contrib?

@practicalswift practicalswift force-pushed the practicalswift:undefined-behaviour-division-by-zero branch Nov 6, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2018

Added commit 8ed2af7ca980345995ed4115570e4677ed0b2f47 which remove the UBSan suppressions for the fixed files.

Please re-review :-)

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

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

Conflicts

No conflicts as of last run.

@practicalswift practicalswift force-pushed the practicalswift:undefined-behaviour-division-by-zero branch Nov 23, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2018

Rebased!

Show resolved Hide resolved src/policy/fees.cpp Outdated

@DrahtBot DrahtBot removed the Needs rebase label Nov 23, 2018

@practicalswift practicalswift force-pushed the practicalswift:undefined-behaviour-division-by-zero branch Nov 24, 2018

@practicalswift practicalswift force-pushed the practicalswift:undefined-behaviour-division-by-zero branch from c4a8855 to 2ce49a1 Feb 7, 2019

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

@promag Feedback addressed. Please re-review.

@promag
Copy link
Member

left a comment

utACK 2ce49a1, thanks.

@Empact

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

I’m not sure it’s better to not have undefined behavior while also not having a logging statement in those cases, than to propagate the available information, with undefined behavior.

In fairness I’m also not clear on what situations the undefined behavior is associated with and how valuable the information is in those cases.

@practicalswift practicalswift force-pushed the practicalswift:undefined-behaviour-division-by-zero branch from 2ce49a1 to e559c73 Feb 8, 2019

@DrahtBot DrahtBot removed the Needs rebase label Feb 8, 2019

@@ -1825,6 +1825,8 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
uint256 hashPrevBlock = pindex->pprev == nullptr ? uint256() : pindex->pprev->GetBlockHash();
assert(hashPrevBlock == view.GetBestBlock());

nBlocksTotal++;

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 10, 2019

Member

uuuuhm, what is this change? I don't think it's supposed to touch validation at all!
I think you're doing an accidental reversion here.

This comment has been minimized.

Copy link
@practicalswift

practicalswift Feb 10, 2019

Author Member

See #15283 for a description of the problem:

During the loading of the genesis block, the bench print lines in ConnectTip divide by zero due to early return in ConnectBlock.

Do you mean it has been fixed elsewhere since being discussed in #15283?

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Feb 10, 2019

Contributor

nBlocksTotal appears to be only referenced otherwise from bench output. And I assume what this is trying to do is avoid the genesis block early terminate from leaving the bench output with a zero. I believe this is a reasonable change, though it will make bench results slightly incomparable (with small numbers of blocks processed) across versions.

This change would have been better done-- easier to review for correctness, more stable against "regression"-- by simply changing the existing initialization constant for nBlocksTotal from 0 to 1.

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

Floating point division by zero is well defined (by IEEE 754, see the C standard Annex F). The justification given for this change is incorrect.

@practicalswift practicalswift changed the title Avoid dividing by zero (undefined behaviour) in EstimateMedianVal (policy)/ConnectTip (validation)/CreateTransaction (wallet) Avoid dividing by zero in EstimateMedianVal (policy)/ConnectTip (validation)/CreateTransaction (wallet) Feb 10, 2019

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2019

@gmaxwell

The value representation of floating-point types is implementation-defined.

For an IEEE 754 implementation (std::numeric_limits<double>::is_iec559 == true), floating point division by zero is well defined.

Otherwise it is undefined.

Please correct me if I'm wrong.

@moneyball

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

I am not familiar with the code base so take this with a grain of salt. I was curious about the change in validation.cpp though, and I thought another pair of eyes cannot hurt. From my observation:

  1. A search of the repo indicates nBlocksTotal is only referenced in validation.cpp.
  2. nBlocksTotal is only used in 2 functions, ConnectBlock and ConnectTip.
  3. nBlocksTotal is initialized to 0, is incremented on another line of code, and the other 11 instances it is a divisor in a LogPrint.
  4. The change made causes the nBlocksTotal counter to include the genesis block, so it will always be one higher than prior to this change. Since it is only used in logging statements, this should not affect functionality at all.
  5. The logging statements in ConnectBlock would not have been vulnerable to this division by zero risk as they would not have been executed for the genesis block due to the return statement for that special case.
  6. The logging statements in ConnectTip might be affected by division by zero as ConnectTip is called by ActivateBestChain, which is called from several places in the code base. I didn't study it further to see if it would ever be called prior to ConnectBlock() for the second time, but given the several instances of calling it sprinkled throughout the code base, it is a good idea to protect against division by zero. [as an aside, would division by zero in a logging statement terminate the process and kill the node? although in this particular example, it'd happen immediately after validating just the first couple of blocks so not really that critical]

This change appears worthwhile and doesn't seem harmful to me.

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

NAK

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

@gmaxwell Why?

@sipa

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@gmaxwell In C++11, division by zero is undefined behavior. Of course all architectures we currently support very likely follow IEEE 754 which makes it well-defined, so calling this a fix for actual UB is an exaggeration.

Still, I think it's an improvement to the codebase in my opinion.

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

Because the behaviour isn't undefined. If you cannot manage to get the basis motivation for the change factually correct what confidence should we have that the change does not break consensus or somehow introduce a vulnerability? Given that the PR doesn't give a (valid) justification for an improvement to the software, it isn't clear to me that it's worth review efforts to determine that its actually free of harmful side-effects and certainly shouldn't be made without such review, it certainly isn't worth yet another worthless language wonking debate.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

@gmaxwell You didn't see my reply from yesterday? :-)

The value representation of floating-point types is implementation-defined.

For an IEEE 754 implementation (std::numeric_limits<double>::is_iec559 == true), floating point division by zero is well defined.

Otherwise it is undefined.

Please correct me if I'm wrong.

@sipa

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@practicalswift Please, don't turn a discussion about language semantics into an absolute what is UB and what not.

I am fairly confident that on all platforms we support, this is not UB, as they follow IEEE 754.

It may be desirable to strictly follow C++11 here, but that's a code quality discussion, not one about software bugs.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

@sipa I simply refuted @gmaxwell's technically incorrect claim. I'm allowed intellectual self-defence, right? :-)

@sipa

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@practicalswift I don't see any incorrect claim or refutation of that. I only see a disagreement about what language requirements to assume.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

@sipa That was the point I made yesterday :-)

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

@sipa In C++11, while allowing all possible implementations and ignoring language annexes, assuming an "int" can represent a value outside of [-32768, 32767] is undefined behaviour. Do you intend to accept PRs going through the codebase to fix all code relative to that?

With these change the codebase is still not unconditionally correct or safe on some hypothetical system which has arbitrary non-IEEE-754-like semantics (e.g. go look at the fee estimator calculations). Nor do I think it's plausible to make it so without just eliminating the floating point entirely. (and, as an aside, if we wanted to limit the codebase to the language without optional appendexes like annex f then we'd have to eliminate floating point regardless)

This is a silence-a-tool make-work change-- of low but non-zero value-- being disguised as a fix of a serious and concerning problem. The result of handling it that way is that it both gets inadequate review and diverts attention from more useful activities, causing an increased risk of actual bugs.

@sipa

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@practicalswift I understand you're on a crusade against anything that smells like UB under the strictest definitions of the language. I support improvements in that regard, but discussions like this only piss people off.

We're writing software for real systems, and even have the luxury of rather tight control over what architectures are supported. There are plenty of assumptions made that restrict correct operation to certain systems. A certain subset of those also mean that things detected by ubsan and others are spurious.

Sorry, but claiming that a fix is needed "because UB in C++" is crying wolf. Especially when not taking the reality into account, it is overly dramatic, distracts from far more important issues, and won't get you much goodwill from reviewers.

I think this is a meaningful code quality improvement, but you just justify it as "this lets us remove the silencing of divide-by-zero in ubsan, making it a more powerful check for future issues, and it's just a small change" - not as "UB! Must fix at all costs", and then turn it into an endless semantics discussion.

@MarcoFalke MarcoFalke removed this from the 0.18.0 milestone Feb 11, 2019

@sipa

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Alternative suggestion:

Add a static_assert(std::numeric_limits<double>::is_iec559 == true) to the code, and remove the explicit enabling of float division by zero in ubsan.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

@sipa Yes, that's a good idea -- I thought about taking that route too. As long as we're verifying the assumptions we're making I'm happy (obviously).

laanwj added a commit that referenced this pull request Feb 15, 2019

Merge #15391: Add compile time verification of assumptions we're curr…
…ently making implicitly/tacitly

7cee858 Add compile time verification of assumptions we're currently making implicitly/tacitly (practicalswift)

Pull request description:

  Add compile time verification of assumptions we're currently making implicitly/tacitly.

  As suggested by @sipa in #14239 (comment) and @MarcoFalke in #14479 (comment).

Tree-SHA512: e68fe51164dbd3eeb76aa8a7e83dfcd3b4d5a66037c0f1822bbbd189bbe3c280e03b3b10af870880ecc09b612e62fb3d9bcd6cf1e16cb7ba818c257db0712ce4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.