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

validation: Increase robustness when loading malformed mempool.dat files (LoadMempool) #20089

Closed

Conversation

practicalswift
Copy link
Contributor

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
Copy link
Contributor

DrahtBot commented Oct 5, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, change looks good.

src/validation.cpp Outdated Show resolved Hide resolved
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, seems fine to add checks against disk corruption

src/validation.cpp Outdated Show resolved Hide resolved
@practicalswift practicalswift force-pushed the load-mempool-robustness branch 2 times, most recently from 6166a9f to 5693d5a Compare October 8, 2020 19:52
@maflcko
Copy link
Member

maflcko commented Oct 9, 2020

review ACK 5693d5a

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review ACK 5693d5a 🩺

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
Copy link
Contributor Author

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

src/validation.cpp Outdated Show resolved Hide resolved
@practicalswift practicalswift force-pushed the load-mempool-robustness branch from 89ab5c2 to 875a2c3 Compare October 12, 2020 20:36
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review re-ACK 875a2c3

@maflcko
Copy link
Member

maflcko commented Oct 13, 2020

review ACK 875a2c3

@practicalswift
Copy link
Contributor Author

Could someone restart Cirrus please? :)

* Like MoneyRange(...), but allows for negative money amounts.
*/
inline bool FeeDeltaRange(const CAmount fee_delta) {
return fee_delta >= -MAX_MONEY && fee_delta <= MAX_MONEY;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@practicalswift practicalswift force-pushed the load-mempool-robustness branch from f044456 to 8953ff9 Compare November 11, 2020 21:25
maflcko pushed a commit that referenced this pull request Nov 12, 2020
…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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 12, 2020
…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 practicalswift deleted the load-mempool-robustness branch April 10, 2021 19:42
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 17, 2023
…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
UdjinM6 pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 18, 2023
…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
@bitcoin bitcoin locked and limited conversation to collaborators Jul 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Three UBSan warnings when loading corrupt mempool.dat files
6 participants