log: Fix UB with bench on genesis block #15283

instagibbs · 2019-01-29T13:06:46Z

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

promag

Funny that DisconnectTip doesn't decrement nBlocksTotal 👀

ACK ec30a79, nBlocksTotal is only used in logging.

@fanquake I don't think this should have validation label.

promag · 2019-01-29T14:56:04Z

src/validation.cpp

            return error("%s: ConnectBlock %s failed, %s", __func__, pindexNew->GetBlockHash().ToString(), FormatStateMessage(state));
        }
        nTime3 = GetTimeMicros(); nTimeConnectTotal += nTime3 - nTime2;
+        assert(nBlocksTotal > 0);


I'd avoid this, I think it is noise as there is already a "divide by nBlocksTotal" which implies this assertion.

Not a fan of silently committing UB.

There are 11 occurrences of / nBlocksTotal.

DrahtBot · 2019-01-29T15:45:29Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Testschains: Many regtests with different genesis and default datadir #17037 (Testschains: Many regtests with different genesis and default datadir by jtimon)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2019-01-29T17:19:02Z

ACK

practicalswift · 2019-01-30T10:34:30Z

This is already fixed in #14239 which was submitted back in September and is scheduled for 0.18.0 :-)

@promag @MarcoFalke @instagibbs Would you mind reviewing #14239?

instagibbs · 2019-01-30T10:38:01Z

@practicalswift sure

practicalswift · 2019-01-31T08:23:07Z

Close in favour of #14239?

laanwj · 2019-01-31T12:05:22Z

Yes this exact change is included in #14239.
Unless the rest of that PR is controversial and this part really needs to be factored out, it's probably better to focus on getting that one merged.

instagibbs · 2019-01-31T12:22:06Z

SGTM. Closing.

practicalswift · 2019-01-31T12:39:31Z

@laanwj PR #14239 is highly uncontroversial so I don't think that should be a problem :-)

instagibbs · 2020-02-20T19:24:23Z

@practicalswift you ended up closing that PR ;P d'oh

practicalswift · 2020-02-20T19:51:34Z

@instagibbs Please re-open this PR :)

practicalswift · 2020-02-20T19:51:50Z

ACK ec30a79

jonatack · 2020-02-20T20:00:19Z

Concept ACK

jonatack · 2020-02-21T15:07:15Z

Ok, this PR doesn't resolve all the UBSan issues that #17208 does, only the one involving nBlocksTotal.

As has been noted several times previously, nBlocksTotal appears to be used solely for bench logging purposes and it seems to me the approach in #17208 of initializing it to 1 instead of 0 is more straightforward to review and robust against regression; this comment by @gmaxwell concurs with that.

practicalswift · 2020-02-21T15:55:20Z

@jonatack Feel free to attack the remaining ones -- I'd be glad to review :)

sipa · 2020-03-17T17:35:41Z

utACK ec30a79

practicalswift · 2020-03-17T18:41:56Z

Nice to see this one merged!

Anyone (perhaps fellow UBSan-fan @jonatack?) who would like to volunteer to get rid of the remaining UBSan suppressions? If so #17208 can be used as a starting point :)

ec30a79 Fix UB with bench on genesis block (Gregory Sanders) Pull request description: During the loading of the genesis block, the bench print lines in ConnectTip divide by zero due to early return in ConnectBlock. ACKs for top commit: practicalswift: ACK ec30a79 sipa: utACK ec30a79 promag: ACK ec30a79, `nBlocksTotal` is only used in logging. Tree-SHA512: b3bdbb58d10d002a2293d7f99196b227ed9f4ca8c6cd08981e95cc964be47efed98b91fad276ee6da5cf7e6684610998ace7ce9bace172dd6c51c386d985b83c

- doc: fix git add argument bitcoin#18513 - build: Fix libevent linking for bench_bitcoin binary bitcoin#18397 - script: fix SCRIPT_ERR_SIG_PUSHONLY error string bitcoin#18412 - doc: Comment fix merkle.cpp bitcoin#18379 - log: Fix UB with bench on genesis block bitcoin#15283 - test: Fix mining to an invalid target + ensure that a new block has the correct hash internally bitcoin#18350 - Fix missing header in sync.h bitcoin#18357 - refactor: Fix implicit value conversion in formatPingTime bitcoin#18260 - Fix .gitignore policy in build_msvc directory bitcoin#18108 - build: Fix behavior when ALLOW_HOST_PACKAGES unset bitcoin#18051 - test: Fix p2p_invalid_messages failing in Python 3.8 because of warning bitcoin#17931 - qa: Fix double-negative arg test bitcoin#17893 - build: Fix configure report about qr bitcoin#17547 - log: Fix log message for -par=1 bitcoin#17325 - bench: Fix negative values and zero for -evals flag bitcoin#17267 - depends: fix boost mac cross build with clang 9+ bitcoin#17231 - doc: Fix broken bitcoin-cli examples bitcoin#17119 - doc: fix Makefile target in benchmarking.md bitcoin#17081 - contrib: fix minor typos in makeseeds.py bitcoin#17042 - test: Fix Python Docstring to include all Args. bitcoin#17030 - doc: Fix some misspellings bitcoin#17351 - doc: Doxygen-friendly script/descriptor.h comments bitcoin#16947 - doc: Fix doxygen errors bitcoin#17945 - doc: Doxygen-friendly CuckooCache comments bitcoin#16986 - doc: Add to Doxygen documentation guidelines bitcoin#17873 - depends: Consistent use of package variable bitcoin#17928 - init: Replace URL_WEBSITE with PACKAGE_URL bitcoin#18503 - doc: Add missed copyright headers bitcoin#17691

ec30a79 Fix UB with bench on genesis block (Gregory Sanders) Pull request description: During the loading of the genesis block, the bench print lines in ConnectTip divide by zero due to early return in ConnectBlock. ACKs for top commit: practicalswift: ACK ec30a79 sipa: utACK ec30a79 promag: ACK ec30a79, `nBlocksTotal` is only used in logging. Tree-SHA512: b3bdbb58d10d002a2293d7f99196b227ed9f4ca8c6cd08981e95cc964be47efed98b91fad276ee6da5cf7e6684610998ace7ce9bace172dd6c51c386d985b83c

… divide-by-zero in validation.cpp) 0ccb3ad tests: Remove no longer needed UBSan suppression (float-divide-by-zero in validation.cpp) (practicalswift) Pull request description: Remove no longer needed UBSan suppression. The float divide-by-zero in `validation.cpp` was fixed by instagibbs in ec30a79 (#15283). ACKs for top commit: MarcoFalke: ACK 0ccb3ad Tree-SHA512: 89a4f4b7371fa5725d9f801cee7ebbd17523f66017c9acfa813657dcb8d837f42209eff44ce9e5d48296a630bab9599d75f10024a0c7da7defb228f4eae3392a

… (float divide-by-zero in validation.cpp) 0ccb3ad tests: Remove no longer needed UBSan suppression (float-divide-by-zero in validation.cpp) (practicalswift) Pull request description: Remove no longer needed UBSan suppression. The float divide-by-zero in `validation.cpp` was fixed by instagibbs in ec30a79 (bitcoin#15283). ACKs for top commit: MarcoFalke: ACK 0ccb3ad Tree-SHA512: 89a4f4b7371fa5725d9f801cee7ebbd17523f66017c9acfa813657dcb8d837f42209eff44ce9e5d48296a630bab9599d75f10024a0c7da7defb228f4eae3392a

Summary: > During the loading of the genesis block, the bench print lines in ConnectTip divide by zero due to early return in ConnectBlock. Increment `nBlocksTotal` before returning in ConnectBlock This is a backport of Core [[bitcoin/bitcoin#15283 | PR15283]] Test Plan: `ninja && ./src/bitcoind -debug=bench | grep "Connect total:"` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8857

Fix UB with bench on genesis block

ec30a79

fanquake added the Validation label Jan 29, 2019

promag reviewed Jan 29, 2019

View reviewed changes

maflcko changed the title ~~Fix UB with bench on genesis block~~ log: Fix UB with bench on genesis block Jan 29, 2019

maflcko added the Utils/log/libs label Jan 29, 2019

maflcko added this to the 0.18.0 milestone Jan 30, 2019

instagibbs closed this Jan 31, 2019

practicalswift mentioned this pull request Feb 10, 2019

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

Closed

instagibbs reopened this Feb 20, 2020

jonatack mentioned this pull request Feb 20, 2020

Make all tests pass UBSan without using any UBSan suppressions #17208

Closed

DrahtBot mentioned this pull request Feb 20, 2020

Testschains: Many regtests with different genesis and default datadir #17037

Closed

maflcko merged commit 39497d1 into bitcoin:master Mar 17, 2020

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

This was referenced Nov 12, 2020

wallet: fix potential division by 0 in WalletLogPrintf #20378

Merged

tests: Remove no longer needed UBSan suppression (float divide-by-zero in validation.cpp) #20379

Merged

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

log: Fix UB with bench on genesis block #15283

log: Fix UB with bench on genesis block #15283

Uh oh!

Conversation

instagibbs commented Jan 29, 2019

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

promag Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Jan 29, 2019

Uh oh!

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

Uh oh!

Uh oh!

instagibbs commented Jan 30, 2019

Uh oh!

practicalswift commented Jan 31, 2019

Uh oh!

laanwj commented Jan 31, 2019

Uh oh!

instagibbs commented Jan 31, 2019

Uh oh!

practicalswift commented Jan 31, 2019

Uh oh!

instagibbs commented Feb 20, 2020

Uh oh!

practicalswift commented Feb 20, 2020

Uh oh!

practicalswift commented Feb 20, 2020

Uh oh!

jonatack commented Feb 20, 2020

Uh oh!

jonatack commented Feb 21, 2020

Uh oh!

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

Uh oh!

Uh oh!

sipa commented Mar 17, 2020

Uh oh!

practicalswift commented Mar 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

promag left a comment •

edited

Loading

DrahtBot commented Jan 29, 2019 •

edited

Loading

practicalswift commented Jan 30, 2019 •

edited

Loading

practicalswift commented Feb 21, 2020 •

edited

Loading