Mini miner AncestorFeerateComparator Signed Integer Overflow #30284

issue ismaelsadeeq openend this issue on June 13, 2024
  1. ismaelsadeeq commented at 7:40 pm on June 13, 2024: member

    While working on #30079, I noticed that using multiplication to compare fee rates results in a signed integer overflow. https://github.com/bitcoin/bitcoin/blob/fcc3b653dc2bd5683193a836556de07bea4d1b11/src/node/mini_miner.cpp#L191-L195

    The policy estimator fuzz tests generate transaction fee and sigops values using ConsumeTxMemPoolEntry. https://github.com/bitcoin/bitcoin/blob/fcc3b653dc2bd5683193a836556de07bea4d1b11/src/test/fuzz/util/mempool.cpp#L17-L31

    Here https://github.com/bitcoin/bitcoin/blob/fcc3b653dc2bd5683193a836556de07bea4d1b11/src/test/fuzz/policy_estimator.cpp#L47

    We fuzz test CBlockPolicyEstimator::processBlock using these randomly generated transaction fee and size. https://github.com/bitcoin/bitcoin/blob/fcc3b653dc2bd5683193a836556de07bea4d1b11/src/test/fuzz/policy_estimator.cpp#L71-L79

    #30079 uses mini miner to linearize received block transactions in CBlockPolicyEstimator::processBlock

    This enables the fuzzed values to reach the mini miner, revealing the overflow.

    A possible is to compare the fee rates using the CFeeRate operator<=, as I did here https://github.com/bitcoin/bitcoin/pull/30079/commits/ba565be6fae897d43698dfa47b7c90aba6cf88c6 to fix the C.I failure in #30079.

    However, @murchandamus mentioned that the reason for originally using multiplication for the comparison was due to precision loss.

    https://cirrus-ci.com/task/5125969122426880?logs=ci#L3239

     0INFO: Seed: 1181225158
     1INFO: Loaded 1 modules   (585279 inline 8-bit counters): 585279 [0x560b0df0fac8, 0x560b0df9e907), 
     2INFO: Loaded 1 PC tables (585279 PCs): 585279 [0x560b0df9e908,0x560b0e88ccf8), 
     3INFO:      973 files found in /ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/policy_estimator
     4INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1047554 bytes
     5INFO: seed corpus: files: 973 min: 1b max: 1047554b total: 47876409b rss: 149Mb
     6[#256](/bitcoin-bitcoin/256/)	pulse  cov: 3067 ft: 7190 corp: 194/19Kb exec/s: 85 rss: 464Mb
     7node/mini_miner.cpp:196:33: runtime error: signed integer overflow: 67333229723135 * 285030 cannot be represented in type 'int64_t' (aka 'long')
     8    [#0](/bitcoin-bitcoin/0/) 0x560b0b9fa461 in bool node::AncestorFeerateComparator::operator()<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>(std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>> const&, std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>> const&) const::'lambda'(node::MiniMinerMempoolEntry const&)::operator()(node::MiniMinerMempoolEntry const&) const src/node/mini_miner.cpp:196:33
     9    [#1](/bitcoin-bitcoin/1/) 0x560b0b9f9dbe in bool node::AncestorFeerateComparator::operator()<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>(std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>> const&, std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>> const&) const src/node/mini_miner.cpp:200:28
    10    [#2](/bitcoin-bitcoin/2/) 0x560b0b9fb0ff in bool __gnu_cxx::__ops::_Iter_comp_iter<node::AncestorFeerateComparator>::operator()<__gnu_cxx::__normal_iterator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>*, std::vector<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>, std::allocator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>>>, __gnu_cxx::__normal_iterator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>*, std::vector<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>, std::allocator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>>>>(__gnu_cxx::__normal_iterator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>*, std::vector<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>, std::allocator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>>>, __gnu_cxx::__normal_iterator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>*, std::vector<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>, std::allocator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>>>) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/predefined_ops.h:158:23
    11    [#3](/bitcoin-bitcoin/3/) 0x560b0b9fb0ff in void std::__insertion_sort<__gnu_cxx::__normal_iterator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>*, std::vector<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>, std::allocator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>>>, __gnu_cxx::__ops::_Iter_comp_iter<node::AncestorFeerateComparator>>(__gnu_cxx::__normal_iterator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>*, std::vector<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>, std::allocator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>>>, __gnu_cxx::__normal_iterator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>*, std::vector<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>, std::allocator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>>>, __gnu_cxx::__ops::_Iter_comp_iter<node::AncestorFeerateComparator>) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_algo.h:1819:8
    12    [#4](/bitcoin-bitcoin/4/) 0x560b0b9f074f in void std::__sort<__gnu_cxx::__normal_iterator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>*, std::vector<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>, std::allocator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>>>, __gnu_cxx::__ops::_Iter_comp_iter<node::AncestorFeerateComparator>>(__gnu_cxx::__normal_iterator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>*, std::vector<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>, std::allocator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>>>, __gnu_cxx::__normal_iterator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>*, std::vector<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>, std::allocator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>>>, __gnu_cxx::__ops::_Iter_comp_iter<node::AncestorFeerateComparator>) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_algo.h:1950:4
    13    [#5](/bitcoin-bitcoin/5/) 0x560b0b9f074f in void std::sort<__gnu_cxx::__normal_iterator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>*, std::vector<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>, std::allocator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>>>, node::AncestorFeerateComparator>(__gnu_cxx::__normal_iterator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>*, std::vector<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>, std::allocator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>>>, __gnu_cxx::__normal_iterator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>*, std::vector<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>, std::allocator<std::_Rb_tree_iterator<std::pair<uint256 const, node::MiniMinerMempoolEntry>>>>>, node::AncestorFeerateComparator) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_algo.h:4894:7
    14    [#6](/bitcoin-bitcoin/6/) 0x560b0b9f074f in node::MiniMiner::BuildMockTemplate(std::optional<CFeeRate>) src/node/mini_miner.cpp:263:9
    15    [#7](/bitcoin-bitcoin/7/) 0x560b0b9f24d2 in node::MiniMiner::Linearize() src/node/mini_miner.cpp:320:5
    16    [#8](/bitcoin-bitcoin/8/) 0x560b0c0fcecd in LinearizeTransactions(std::vector<RemovedMempoolTransactionInfo, std::allocator<RemovedMempoolTransactionInfo>> const&) src/policy/fees_util.cpp:66:61
    17    [#9](/bitcoin-bitcoin/9/) 0x560b0ba50884 in CBlockPolicyEstimator::removeCPFPdParentTxs(std::vector<RemovedMempoolTransactionInfo, std::allocator<RemovedMempoolTransactionInfo>> const&) src/policy/fees.cpp:668:41
    18    [#10](/bitcoin-bitcoin/10/) 0x560b0ba4d92b in CBlockPolicyEstimator::processBlock(std::vector<RemovedMempoolTransactionInfo, std::allocator<RemovedMempoolTransactionInfo>> const&, unsigned int) src/policy/fees.cpp:711:5
    19    [#11](/bitcoin-bitcoin/11/) 0x560b0b29d292 in policy_estimator_fuzz_target(Span<unsigned char const>)::$_1::operator()() const src/test/fuzz/policy_estimator.cpp:78:40
    20    [#12](/bitcoin-bitcoin/12/) 0x560b0b29d292 in unsigned long CallOneOf<policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3>(FuzzedDataProvider&, policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3) src/./test/fuzz/util.h:42:27
    21    [#13](/bitcoin-bitcoin/13/) 0x560b0b29d292 in policy_estimator_fuzz_target(Span<unsigned char const>) src/test/fuzz/policy_estimator.cpp:38:9
    22    [#14](/bitcoin-bitcoin/14/) 0x560b0b41be8d in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
    23    [#15](/bitcoin-bitcoin/15/) 0x560b0b41be8d in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:201:5
    24    [#16](/bitcoin-bitcoin/16/) 0x560b0ad42044 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1abb044) (BuildId: 38c5692016408502c695f631cd9bdad2a9744d28)
    25    [#17](/bitcoin-bitcoin/17/) 0x560b0ad41739 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1aba739) (BuildId: 38c5692016408502c695f631cd9bdad2a9744d28)
    26    [#18](/bitcoin-bitcoin/18/) 0x560b0ad43356 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1abc356) (BuildId: 38c5692016408502c695f631cd9bdad2a9744d28)
    27    [#19](/bitcoin-bitcoin/19/) 0x560b0ad43867 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1abc867) (BuildId: 38c5692016408502c695f631cd9bdad2a9744d28)
    28    [#20](/bitcoin-bitcoin/20/) 0x560b0ad30d5f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1aa9d5f) (BuildId: 38c5692016408502c695f631cd9bdad2a9744d28)
    29    [#21](/bitcoin-bitcoin/21/) 0x560b0ad5b3e6 in main (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1ad43e6) (BuildId: 38c5692016408502c695f631cd9bdad2a9744d28)
    30    [#22](/bitcoin-bitcoin/22/) 0x7f432cb5e1c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)
    31    [#23](/bitcoin-bitcoin/23/) 0x7f432cb5e28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)
    32    [#24](/bitcoin-bitcoin/24/) 0x560b0ad25d44 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1a9ed44) (BuildId: 38c5692016408502c695f631cd9bdad2a9744d28)
    33SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow node/mini_miner.cpp:196:33 
    34MS: 0 ; base unit: 0000000000000000000000000000000000000000
    35artifact_prefix='./'; Test unit written to ./crash-2315c89458602360eb93362328da5c0ef21f9864
    36Traceback (most recent call last):
    37  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 411, in <module>
    38    main()
    39  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 199, in main
    40    run_once(
    41  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 376, in run_once
    42    assert len(done_stat) == 1
    43           ^^^^^^^^^^^^^^^^^^^
    44AssertionError
    
  2. glozow added the label Tests on Jun 19, 2024
  3. glozow commented at 3:08 pm on July 9, 2024: member
    Ah, I think #30412 might fix this since FeeFrac::Mul handles the overflow? ~I’m not sure how to get the input from that CI task so I can check.~ Edit: nvm, got it Edit: yay it does fix it! See https://github.com/bitcoin/bitcoin/pull/30079/commits/b1b28a21e185dfbc0c75f92ac55401a9ba8a5b08#r1670728220
  4. fanquake closed this on Jul 15, 2024


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-11-21 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me