validation: Increase robustness when loading malformed mempool.dat files (LoadMempool) by practicalswift · Pull Request #20089 · bitcoin/bitcoin

practicalswift · 2020-10-05T19:06:43Z

Increase robustness when loading malformed mempool.dat files.

Avoids the following three signed integer overflows when loading malformed mempool.dat files:

$ xxd -p -r > mempool.dat-crash-1 <<EOF
0100000000000000000000000004000000000000000000000000ffffffff
ffffff7f00000000000000000000000000
EOF
$ cp mempool.dat-crash-1 ~/.bitcoin/regtest/mempool.dat
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
validation.cpp:5079:23: runtime error: signed integer overflow: 9223372036854775807 + 1209600 cannot be represented in type 'long'
    #0 0x5618d335197f in LoadMempool(CTxMemPool&) src/validation.cpp:5079:23
    #1 0x5618d3350df3 in CChainState::LoadMempool(ArgsManager const&) src/validation.cpp:4217:9
    #2 0x5618d2b9345f in ThreadImport(ChainstateManager&, std::vector<boost::filesystem::path, std::allocator<boost::filesystem::path> >, ArgsManager const&) src/init.cpp:762:33
    #3 0x5618d2b92162 in AppInitMain(util::Ref const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_14::operator()() const src/init.cpp:1881:9

$ xxd -p -r > mempool.dat-crash-2 <<EOF
010000000000000000050900000000000509000000000000800000000000
0000000000000000000000800000000000
EOF
$ cp mempool.dat-crash-2 ~/.bitcoin/regtest/mempool.dat
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
util/moneystr.cpp:16:34: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount' (aka 'long'); cast to an unsigned type to negate this value to itself                                                                                  
    #0 0x5618a4a6366c in FormatMoney[abi:cxx11](long const&) src/util/moneystr.cpp:16:34
    #1 0x5618a406c2f1 in CTxMemPool::PrioritiseTransaction(uint256 const&, long const&) src/txmempool.cpp:861:77
    #2 0x5618a411064f in LoadMempool(CTxMemPool&) src/validation.cpp:5076:22
    #3 0x5618a410fdf3 in CChainState::LoadMempool(ArgsManager const&) src/validation.cpp:4217:9
    #4 0x5618a395245f in ThreadImport(ChainstateManager&, std::vector<boost::filesystem::path, std::allocator<boost::filesystem::path> >, ArgsManager const&) src/init.cpp:762:33
    #5 0x5618a3951162 in AppInitMain(util::Ref const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_14::operator()() const src/init.cpp:1881:9

$ xxd -p -r > mempool.dat-crash-3 <<EOF
0100000000000000000000000000000001253d8c0000000000000000006b
00000000f7000000ff00f7000000ff0000000000000000000000800000ff
0000
EOF
$ cp mempool.dat-crash-3 ~/.bitcoin/regtest/mempool.dat
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
util/moneystr.cpp:16:34: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount' (aka 'long'); cast to an unsigned type to negate this value to itself                                                                                  
    #0 0x5575f515e66c in FormatMoney[abi:cxx11](long const&) src/util/moneystr.cpp:16:34
    #1 0x5575f47672f1 in CTxMemPool::PrioritiseTransaction(uint256 const&, long const&) src/txmempool.cpp:861:77
    #2 0x5575f480bc82 in LoadMempool(CTxMemPool&) src/validation.cpp:5107:18
    #3 0x5575f480adf3 in CChainState::LoadMempool(ArgsManager const&) src/validation.cpp:4217:9
    #4 0x5575f404d45f in ThreadImport(ChainstateManager&, std::vector<boost::filesystem::path, std::allocator<boost::filesystem::path> >, ArgsManager const&) src/init.cpp:762:33
    #5 0x5575f404c162 in AppInitMain(util::Ref const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_14::operator()() const src/init.cpp:1881:9

Fixes #19278.

DrahtBot · 2020-10-05T21:07:41Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Fix UBSan warnings triggered when loading corrupt mempool.dat files #19381 (Fix UBSan warnings triggered when loading corrupt mempool.dat files by rajarshimaitra)

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.

promag

Concept ACK, change looks good.

src/validation.cpp

maflcko

ACK, seems fine to add checks against disk corruption

src/validation.cpp

maflcko · 2020-10-09T07:07:08Z

review ACK 5693d5ad4a1ea862b82b25a3194569bfb66bc67d

theStack

Code Review ACK 5693d5ad4a1ea862b82b25a3194569bfb66bc67d 🩺

As a potential follow-up, would it also make sense to notify the user in case the file is malformed (which is most likely caused by disk corruption), with a warning?

practicalswift · 2020-10-12T06:35:13Z

@theStack Now printing a log message. Please re-review :)

src/validation.cpp

theStack

Code Review re-ACK 875a2c3149f6f94b198c7c24230375d27410f422

maflcko · 2020-10-13T13:19:55Z

review ACK 875a2c3149f6f94b198c7c24230375d27410f422

practicalswift · 2020-10-19T20:15:02Z

Could someone restart Cirrus please? :)

luke-jr · 2020-10-24T18:16:57Z

src/amount.h

Fee deltas outside MAX_MONEY are valid and have at least a conceptual use case (consider that fee rate is divided by transaction size: a larger transaction would need >MAX_MONEY feerate to go before a MAX_MONEY smaller tx).

@luke-jr What is the valid fee delta range in your opinion?

I'm not sure this is actionable unless we can establish what the valid range should be.

This is not a feerate, but an absolute amount in satoshis

…t file with a malformed time field ee11a41 Avoid signed integer overflow when loading a mempool.dat file with a malformed time field (practicalswift) Pull request description: Avoid signed integer overflow when loading a `mempool.dat` file with a malformed time field. Avoid the following signed integer overflow: ``` $ xxd -p -r > mempool.dat-crash-1 <<EOF 0100000000000000000000000004000000000000000000000000ffffffff ffffff7f00000000000000000000000000 EOF $ cp mempool.dat-crash-1 ~/.bitcoin/regtest/mempool.dat $ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest validation.cpp:5079:23: runtime error: signed integer overflow: 9223372036854775807 + 1209600 cannot be represented in type 'long' #0 0x5618d335197f in LoadMempool(CTxMemPool&) src/validation.cpp:5079:23 #1 0x5618d3350df3 in CChainState::LoadMempool(ArgsManager const&) src/validation.cpp:4217:9 #2 0x5618d2b9345f in ThreadImport(ChainstateManager&, std::vector<boost::filesystem::path, std::allocator<boost::filesystem::path> >, ArgsManager const&) src/init.cpp:762:33 #3 0x5618d2b92162 in AppInitMain(util::Ref const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_14::operator()() const src/init.cpp:1881:9 ``` This PR was broken out from PR #20089. Hopefully this PR is trivial to review. Fixes a subset of #19278. ACKs for top commit: MarcoFalke: review ACK ee11a41 Crypt-iQ: crACK ee11a41 Tree-SHA512: 227ab95cd7d22f62f3191693b455eacfa8e36534961bee12c622fc9090957cfb29992eabafa74d806a336e03385aa8f98b7ce734f04b0b400e33aa187d353337

…pool.dat file with a malformed time field ee11a41 Avoid signed integer overflow when loading a mempool.dat file with a malformed time field (practicalswift) Pull request description: Avoid signed integer overflow when loading a `mempool.dat` file with a malformed time field. Avoid the following signed integer overflow: ``` $ xxd -p -r > mempool.dat-crash-1 <<EOF 0100000000000000000000000004000000000000000000000000ffffffff ffffff7f00000000000000000000000000 EOF $ cp mempool.dat-crash-1 ~/.bitcoin/regtest/mempool.dat $ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest validation.cpp:5079:23: runtime error: signed integer overflow: 9223372036854775807 + 1209600 cannot be represented in type 'long' #0 0x5618d335197f in LoadMempool(CTxMemPool&) src/validation.cpp:5079:23 #1 0x5618d3350df3 in CChainState::LoadMempool(ArgsManager const&) src/validation.cpp:4217:9 #2 0x5618d2b9345f in ThreadImport(ChainstateManager&, std::vector<boost::filesystem::path, std::allocator<boost::filesystem::path> >, ArgsManager const&) src/init.cpp:762:33 #3 0x5618d2b92162 in AppInitMain(util::Ref const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_14::operator()() const src/init.cpp:1881:9 ``` This PR was broken out from PR bitcoin#20089. Hopefully this PR is trivial to review. Fixes a subset of bitcoin#19278. ACKs for top commit: MarcoFalke: review ACK ee11a41 Crypt-iQ: crACK ee11a41 Tree-SHA512: 227ab95cd7d22f62f3191693b455eacfa8e36534961bee12c622fc9090957cfb29992eabafa74d806a336e03385aa8f98b7ce734f04b0b400e33aa187d353337

practicalswift mentioned this pull request Oct 5, 2020

fuzz: Add fuzzing harness for LoadMempool(...) and DumpMempool(...) #19259

Merged

DrahtBot added the Validation label Oct 5, 2020

DrahtBot mentioned this pull request Oct 5, 2020

Fix UBSan warnings triggered when loading corrupt mempool.dat files #19381

Closed

promag reviewed Oct 7, 2020

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

maflcko approved these changes Oct 8, 2020

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the load-mempool-robustness branch 2 times, most recently from 6166a9f to 5693d5a Compare October 8, 2020 19:52

theStack approved these changes Oct 11, 2020

View reviewed changes

theStack reviewed Oct 12, 2020

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the load-mempool-robustness branch from 89ab5c2 to 875a2c3 Compare October 12, 2020 20:36

theStack approved these changes Oct 13, 2020

View reviewed changes

luke-jr suggested changes Oct 24, 2020

View reviewed changes

practicalswift force-pushed the load-mempool-robustness branch from 875a2c3 to f044456 Compare November 7, 2020 19:45

practicalswift mentioned this pull request Nov 11, 2020

Avoid signed integer overflow when loading a mempool.dat file with a malformed time field #20372

Merged

practicalswift added 2 commits November 11, 2020 21:24

validation: Increase robustness when loading malformed mempool.dat files

3f30ab8

Print log message in case of a corrupt mempool.dat file

8953ff9

practicalswift force-pushed the load-mempool-robustness branch from f044456 to 8953ff9 Compare November 11, 2020 21:25

practicalswift closed this Nov 13, 2020

practicalswift reopened this Jan 14, 2021

practicalswift closed this Jan 14, 2021

practicalswift mentioned this pull request Feb 28, 2021

Avoid signed integer overflow and invalid integer negation when loading malformed mempool.dat files #20383

Closed

practicalswift deleted the load-mempool-robustness branch April 10, 2021 19:42

bitcoin locked and limited conversation to collaborators Jul 27, 2023

Conversation

practicalswift commented Oct 5, 2020

Uh oh!

DrahtBot commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Oct 9, 2020

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Oct 12, 2020

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 13, 2020

Uh oh!

practicalswift commented Oct 19, 2020

Uh oh!

luke-jr Oct 24, 2020

Choose a reason for hiding this comment

Uh oh!

practicalswift Oct 24, 2020

Choose a reason for hiding this comment

Uh oh!

practicalswift Nov 7, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Oct 5, 2020 •

edited

Loading