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:
0./configure --with-sanitizers=undefined && make check && ./test/functional/test_runner.py
2441@@ -2442,17 +2442,23 @@ bool CChainState::ConnectTip(CValidationState& state, const CChainParams& chainp
2442 return error("%s: ConnectBlock %s failed, %s", __func__, pindexNew->GetBlockHash().ToString(), FormatStateMessage(state));
2443 }
2444 nTime3 = GetTimeMicros(); nTimeConnectTotal += nTime3 - nTime2;
2445- LogPrint(BCLog::BENCH, " - Connect total: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime3 - nTime2) * MILLI, nTimeConnectTotal * MICRO, nTimeConnectTotal * MILLI / nBlocksTotal);
2446+ if (nBlocksTotal != 0) {
nBlocksTotal can be zero? So at this point we could just assert(nBlocksTotal > 0) and ignore these remaining changes.
nBlocksTotal can be zero: this UB is triggered when running the functional tests.
As said in the PR description:
UBSAN in Travis running the test suite would have catched all three of these.
:-)
381- passBucket.withinTarget, passBucket.totalConfirmed, passBucket.inMempool, passBucket.leftMempool,
382- failBucket.start, failBucket.end,
383- 100 * failBucket.withinTarget / (failBucket.totalConfirmed + failBucket.inMempool + failBucket.leftMempool),
384- failBucket.withinTarget, failBucket.totalConfirmed, failBucket.inMempool, failBucket.leftMempool);
385-
386+ if (passBucket.totalConfirmed + passBucket.inMempool + passBucket.leftMempool != 0 && failBucket.totalConfirmed + failBucket.inMempool + failBucket.leftMempool != 0) {
Yes they can be zero: this UB is triggered when running the functional tests.
As said in the PR description:
UBSAN in Travis running the test suite would have catched all three of these.
:-)
I have 2 suggestions:
> instead of !=?So:
0auto pass_bucket_sum = passBucket.totalConfirmed + passBucket.inMempool + passBucket.leftMempool;
1if (pass_bucket_sum > 0 && ...) {
2 ...
3 100 * passBucket.withinTarget / pass_bucket_sum,
4 ...
5}
@fanquake Is refactoring really the correct label for this PR?
Definitions:
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.
@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 :-)
Added commit 8ed2af7ca980345995ed4115570e4677ed0b2f47 which remove the UBSan suppressions for the fixed files.
Please re-review :-)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
if (pass_bucket_sum > 0 && fail_bucket_sum > 0), how about something like: pass_bucket_sum ? 100 * passBucket.withinTarget / pass_bucket_sum : -1 or similar with a string value result such as “N/A”.?
utACK 914553d.
But I wonder if logs with -1 matter at all. If not then it could be avoided by incrementing the denominator, like sum = a + b + 1 and then the ternary can be removed..
But I wonder if logs with -1 matter at all. If not then it could be avoided by incrementing the denominator, like
sum = a + b + 1and then the ternary can be removed..
Agree, I’m not sure logging -1 is so great here. The meaning of this would be unclear to the user.
-1, e.g. "N/A"?
%d cannot be used for "N/A". I think the current version is simpler and therefore preferred :-)
373@@ -374,14 +374,18 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal,
374 failBucket.leftMempool = failNum;
375 }
376
377- LogPrint(BCLog::ESTIMATEFEE, "FeeEst: %d %s%.0f%% decay %.5f: feerate: %g from (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",
378+ auto pass_bucket_sum = passBucket.totalConfirmed + passBucket.inMempool + passBucket.leftMempool;
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.
1824@@ -1825,6 +1825,8 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
1825 uint256 hashPrevBlock = pindex->pprev == nullptr ? uint256() : pindex->pprev->GetBlockHash();
1826 assert(hashPrevBlock == view.GetBestBlock());
1827
1828+ nBlocksTotal++;
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.
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.
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:
This change appears worthwhile and doesn’t seem harmful to me.
@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 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.
@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.
@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.
@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.
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.