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
@fanquake Refactoring? :-)
Concept ACK. Could enable the functional tests in travis in this commit?
@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 :-)
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) {
I don't think 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) {
Can any of this sums be zero? I guess no otherwise this was raised before. Just assert? Same below in wallet.cpp.
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:
auto pass_bucket_sum = passBucket.totalConfirmed + passBucket.inMempool + passBucket.leftMempool;
if (pass_bucket_sum > 0 && ...) {
...
100 * passBucket.withinTarget / pass_bucket_sum,
...
}
Rebased! :-)
@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 Ping? :-)
@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 :-)
Could remove the suppression from the file in contrib?
Added commit 8ed2af7ca980345995ed4115570e4677ed0b2f47 which remove the UBSan suppressions for the fixed files.
Please re-review :-)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
Rebased!
@promag Good points! Now addressed. Please re-review :-)
@MarcoFalke Could this one get a release milestone? :-)
Rather than 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".?
@Empact Fixed. Please re-review :-)
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.
How about keeping the log message in those cases and adapting it to be more clear than -1, e.g. "N/A"?
Now reverted to the original version. Please re-review.
@Empact That will make the diff unnecessarily large since %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;
nit, could specify type instead of auto.
utACK c4a88550, nit, also could be squashed.
@promag Feedback addressed. Please re-review.
utACK 2ce49a16, thanks.
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++;
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.
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.
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.
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.
NAK
@gmaxwell Why?
@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.
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.
@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.
@practicalswift I don't see any incorrect claim or refutation of that. I only see a disagreement about what language requirements to assume.
@sipa That was the point I made yesterday :-)
@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.
@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).