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

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:load-mempool-robustness changing 2 files +19 −3
  1. practicalswift commented at 7:06 pm on October 5, 2020: contributor

    Increase robustness when loading malformed mempool.dat files.

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

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

     0$ xxd -p -r > mempool.dat-crash-2 <<EOF
 1010000000000000000050900000000000509000000000000800000000000
 20000000000000000000000800000000000
 3EOF
 4$ cp mempool.dat-crash-2 ~/.bitcoin/regtest/mempool.dat
 5$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
 6util/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                                                                                  
 7    [#0](/bitcoin-bitcoin/0/) 0x5618a4a6366c in FormatMoney[abi:cxx11](long const&) src/util/moneystr.cpp:16:34
 8    [#1](/bitcoin-bitcoin/1/) 0x5618a406c2f1 in CTxMemPool::PrioritiseTransaction(uint256 const&, long const&) src/txmempool.cpp:861:77
 9    [#2](/bitcoin-bitcoin/2/) 0x5618a411064f in LoadMempool(CTxMemPool&) src/validation.cpp:5076:22
10    [#3](/bitcoin-bitcoin/3/) 0x5618a410fdf3 in CChainState::LoadMempool(ArgsManager const&) src/validation.cpp:4217:9
11    [#4](/bitcoin-bitcoin/4/) 0x5618a395245f in ThreadImport(ChainstateManager&, std::vector<boost::filesystem::path, std::allocator<boost::filesystem::path> >, ArgsManager const&) src/init.cpp:762:33
12    [#5](/bitcoin-bitcoin/5/) 0x5618a3951162 in AppInitMain(util::Ref const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_14::operator()() const src/init.cpp:1881:9

     0$ xxd -p -r > mempool.dat-crash-3 <<EOF
 10100000000000000000000000000000001253d8c0000000000000000006b
 200000000f7000000ff00f7000000ff0000000000000000000000800000ff
 30000
 4EOF
 5$ cp mempool.dat-crash-3 ~/.bitcoin/regtest/mempool.dat
 6$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
 7util/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                                                                                  
 8    [#0](/bitcoin-bitcoin/0/) 0x5575f515e66c in FormatMoney[abi:cxx11](long const&) src/util/moneystr.cpp:16:34
 9    [#1](/bitcoin-bitcoin/1/) 0x5575f47672f1 in CTxMemPool::PrioritiseTransaction(uint256 const&, long const&) src/txmempool.cpp:861:77
10    [#2](/bitcoin-bitcoin/2/) 0x5575f480bc82 in LoadMempool(CTxMemPool&) src/validation.cpp:5107:18
11    [#3](/bitcoin-bitcoin/3/) 0x5575f480adf3 in CChainState::LoadMempool(ArgsManager const&) src/validation.cpp:4217:9
12    [#4](/bitcoin-bitcoin/4/) 0x5575f404d45f in ThreadImport(ChainstateManager&, std::vector<boost::filesystem::path, std::allocator<boost::filesystem::path> >, ArgsManager const&) src/init.cpp:762:33
13    [#5](/bitcoin-bitcoin/5/) 0x5575f404c162 in AppInitMain(util::Ref const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_14::operator()() const src/init.cpp:1881:9

    Fixes #19278.

  2. DrahtBot added the label Validation on Oct 5, 2020
  3. DrahtBot commented at 9:07 pm on October 5, 2020: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #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.

  4. in src/validation.cpp:5046 in 6d3ffd566a outdated 
    5037@@ -5037,6 +5038,14 @@ int VersionBitsTipStateSinceHeight(const Consensus::Params& params, Consensus::D
5038 
5039 static const uint64_t MEMPOOL_DUMP_VERSION = 1;
5040 
5041+// Check if an amount is a valid fee delta.
5042+//
5043+// Like MoneyRange(...), but allows for negative money amounts.
5044+static bool FeeDeltaRange(const CAmount fee_delta)
5045+{
5046+    return fee_delta >= -MAX_MONEY && fee_delta <= MAX_MONEY;
    promag commented at 7:59 am on October 7, 2020:
    nit, MoneyRange(abs(fee_delta))?
    practicalswift commented at 8:52 am on October 7, 2020:

    No that would be very dangerous! :)

    I’m intentionally avoiding abs here.

    FeeDeltaRange(fee_delta) is not equivalent to MoneyRange(abs(fee_delta)) :)

    Live demo:

    At first sight they might appear to be equivalent:

     0$ cling
 1[cling]$ #include <cstdint>
 2[cling]$ using CAmount = int64_t;
 3[cling]$ static const CAmount COIN = 100000000;
 4[cling]$ static const CAmount MAX_MONEY = 21000000 * COIN;
 5[cling]$ bool FeeDeltaRange(const CAmount fee_delta) { return fee_delta >= -MAX_MONEY && fee_delta <= MAX_MONEY; }
 6[cling]$ bool MoneyRange(const CAmount& nValue) { return (nValue >= 0 && nValue <= MAX_MONEY); }
 7[cling]$ FeeDeltaRange(-12345)
 8(bool) true
 9[cling]$ MoneyRange(abs(-12345))
10(bool) true

    But let’s test with the amount -9223372036854775808 (std::numeric_limits<int64_t>::min()):

    0[cling]$ #include <limits>
1[cling]$ CAmount counterexample = std::numeric_limits<int64_t>::min();
2[cling]$ counterexample
3(long) -9223372036854775808
4[cling]$ FeeDeltaRange(counterexample)
5(bool) false
6[cling]$ MoneyRange(abs(counterexample))
7(bool) true

    Note that the behaviour of abs is undefined if the result cannot be represented by the return type.

    The same goes for std::abs which is not equivalent to abs :)

    Another std::abs/abs gotcha:

    0[cling]$ abs(std::numeric_limits<int64_t>::min())
1(int) 0
2[cling]$ std::abs(std::numeric_limits<int64_t>::min())
3(long) -9223372036854775808

    Note also that the result of std::abs is not guaranteed to be positive as shown in the latter case :)

  5. promag commented at 8:09 am on October 7, 2020: member
    Concept ACK, change looks good.
  6. in src/validation.cpp:5044 in 6d3ffd566a outdated 
    5037@@ -5037,6 +5038,14 @@ int VersionBitsTipStateSinceHeight(const Consensus::Params& params, Consensus::D
5038 
5039 static const uint64_t MEMPOOL_DUMP_VERSION = 1;
5040 
5041+// Check if an amount is a valid fee delta.
5042+//
5043+// Like MoneyRange(...), but allows for negative money amounts.
5044+static bool FeeDeltaRange(const CAmount fee_delta)
    MarcoFalke commented at 12:29 pm on October 8, 2020:
    This helper could be moved to the amount module?
    practicalswift commented at 7:51 pm on October 8, 2020:
    Good idea! Done! :)
  7. MarcoFalke approved
  8. MarcoFalke commented at 12:30 pm on October 8, 2020: member
    ACK, seems fine to add checks against disk corruption
  9. practicalswift force-pushed on Oct 8, 2020
  10. practicalswift force-pushed on Oct 8, 2020
  11. MarcoFalke commented at 7:07 am on October 9, 2020: member
    review ACK 5693d5ad4a1ea862b82b25a3194569bfb66bc67d
  12. theStack approved
  13. theStack commented at 5:31 pm on October 11, 2020: contributor

    Code Review ACK 5693d5ad4a1ea862b82b25a3194569bfb66bc67d :stethoscope:

    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?

  14. practicalswift commented at 6:35 am on October 12, 2020: contributor
    @theStack Now printing a log message. Please re-review :)
  15. in src/validation.cpp:5114 in 89ab5c2458 outdated 
    5109@@ -5106,6 +5110,10 @@ bool LoadMempool(CTxMemPool& pool)
5110         for (const auto& i : mapDeltas) {
5111             if (FeeDeltaRange(i.second)) {
5112                 pool.PrioritiseTransaction(i.first, i.second);
5113+            } else {
5114+                if (i.second != 0) {
    theStack commented at 5:31 pm on October 12, 2020:
    This condition is always true, isn’t it? 0 is a valid fee delta, hence the else-branch wouldn’t be taken in the case that i.second is zero.
    practicalswift commented at 8:36 pm on October 12, 2020:

    You’re absolutely right: the conditional is redundant. Thanks for noticing! Redundancy removed :)

    Please re-review! :)

  16. practicalswift force-pushed on Oct 12, 2020
  17. theStack approved
  18. theStack commented at 12:04 pm on October 13, 2020: contributor
    Code Review re-ACK 875a2c3149f6f94b198c7c24230375d27410f422
  19. MarcoFalke commented at 1:19 pm on October 13, 2020: member
    review ACK 875a2c3149f6f94b198c7c24230375d27410f422
  20. practicalswift commented at 8:15 pm on October 19, 2020: contributor
    Could someone restart Cirrus please? :)
  21. in src/amount.h:33 in 875a2c3149 outdated 
    24@@ -25,4 +25,12 @@ static const CAmount COIN = 100000000;
25 static const CAmount MAX_MONEY = 21000000 * COIN;
26 inline bool MoneyRange(const CAmount& nValue) { return (nValue >= 0 && nValue <= MAX_MONEY); }
27 
28+/** Check if an amount is a valid fee delta.
29+ *
30+ * Like MoneyRange(...), but allows for negative money amounts.
31+ */
32+inline bool FeeDeltaRange(const CAmount fee_delta) {
33+    return fee_delta >= -MAX_MONEY && fee_delta <= MAX_MONEY;
    luke-jr commented at 6:16 pm on October 24, 2020:
    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).
    practicalswift commented at 10:13 pm on October 24, 2020:
    @luke-jr What is the valid fee delta range in your opinion?
    practicalswift commented at 7:54 pm on November 7, 2020:
    I’m not sure this is actionable unless we can establish what the valid range should be.
    MarcoFalke commented at 9:21 am on January 14, 2021:
    This is not a feerate, but an absolute amount in satoshis
  22. luke-jr changes_requested
  23. practicalswift force-pushed on Nov 7, 2020
  24. validation: Increase robustness when loading malformed mempool.dat files 3f30ab8a77
  25. Print log message in case of a corrupt mempool.dat file 8953ff9658
  26. practicalswift force-pushed on Nov 11, 2020
  27. MarcoFalke referenced this in commit 027e51f715 on Nov 12, 2020
  28. sidhujag referenced this in commit b4151ac86c on Nov 12, 2020
  29. practicalswift closed this on Nov 13, 2020
  30. practicalswift reopened this on Jan 14, 2021
  31. practicalswift closed this on Jan 14, 2021
  32. practicalswift deleted the branch on Apr 10, 2021
  33. PastaPastaPasta referenced this in commit 3721768696 on Apr 17, 2023
  34. UdjinM6 referenced this in commit 8869d634f0 on Apr 18, 2023
  35. bitcoin locked this on Jul 27, 2023


practicalswift DrahtBot promag MarcoFalke theStack luke-jr

Labels
Validation

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 13:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me