Three UBSan warnings when loading corrupt mempool.dat files #19278

issue practicalswift openend this issue on June 14, 2020
  1. practicalswift commented at 7:23 pm on June 14, 2020: contributor

    I noticed three UBSan warnings that are reachable when loading corrupt mempool.dat files.

    0$ xxd -p -r > mempool.dat-crashes-in-validation.cpp-5064 <<EOF
10100000000000000000000000004000000000000000000000000ffffffff
2ffffff7f00000000000000000000000000
3EOF
4$ cp mempool.dat-crashes-in-validation.cpp-5064 ~/.bitcoin/regtest/mempool.dat
5$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
6validation.cpp:5064:23: runtime error: signed integer overflow: 9223372036854775807 + 1209600 cannot be represented in type 'long'
7    [#0](/bitcoin-bitcoin/0/) 0x5573a5a07a8b in LoadMempool(CTxMemPool&, std::function<_IO_FILE* (boost::filesystem::path const&, char const*)>) src/validation.cpp:5064:23
8    [#1](/bitcoin-bitcoin/1/) 0x5573a52cce12 in ThreadImport(ChainstateManager&, std::vector<boost::filesystem::path, std::allocator<boost::filesystem::path> >) src/init.cpp:750:9
9    [#2](/bitcoin-bitcoin/2/) 0x5573a52cbef7 in AppInitMain(util::Ref const&, NodeContext&)::$_9::operator()() const src/init.cpp:1845:48

     0$ xxd -p -r > mempool.dat-crashes-in-util-moneystr.cpp-16a <<EOF
 1010000000000000000050900000000000509000000000000800000000000
 20000000000000000000000800000000000
 3EOF
 4$ cp mempool.dat-crashes-in-util-moneystr.cpp-16a ~/.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/) 0x5641ce3e0aec in FormatMoney[abi:cxx11](long const&) src/util/moneystr.cpp:16:34
 8    [#1](/bitcoin-bitcoin/1/) 0x5641cd98a001 in CTxMemPool::PrioritiseTransaction(uint256 const&, long const&) src/txmempool.cpp:850:77
 9    [#2](/bitcoin-bitcoin/2/) 0x5641cdb117b4 in LoadMempool(CTxMemPool&, std::function<_IO_FILE* (boost::filesystem::path const&, char const*)>) src/validation.cpp:5061:22
10    [#3](/bitcoin-bitcoin/3/) 0x5641cd3d6e12 in ThreadImport(ChainstateManager&, std::vector<boost::filesystem::path, std::allocator<boost::filesystem::path> >) src/init.cpp:750:9
11    [#4](/bitcoin-bitcoin/4/) 0x5641cd3d5ef7 in AppInitMain(util::Ref const&, NodeContext&)::$_9::operator()() const src/init.cpp:1845:48

     0$ xxd -p -r > mempool.dat-crashes-in-util-moneystr.cpp-16b <<EOF
 10100000000000000000000000000000001253d8c0000000000000000006b
 200000000f7000000ff00f7000000ff0000000000000000000000800000ff
 30000
 4EOF
 5$ cp mempool.dat-crashes-in-util-moneystr.cpp-16b ~/.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/) 0x55987d51daec in FormatMoney[abi:cxx11](long const&) src/util/moneystr.cpp:16:34
 9    [#1](/bitcoin-bitcoin/1/) 0x55987cac7001 in CTxMemPool::PrioritiseTransaction(uint256 const&, long const&) src/txmempool.cpp:850:77
10    [#2](/bitcoin-bitcoin/2/) 0x55987cc4ed6f in LoadMempool(CTxMemPool&, std::function<_IO_FILE* (boost::filesystem::path const&, char const*)>) src/validation.cpp:5092:18
11    [#3](/bitcoin-bitcoin/3/) 0x55987c513e12 in ThreadImport(ChainstateManager&, std::vector<boost::filesystem::path, std::allocator<boost::filesystem::path> >) src/init.cpp:750:9
12    [#4](/bitcoin-bitcoin/4/) 0x55987c512ef7 in AppInitMain(util::Ref const&, NodeContext&)::$_9::operator()() const src/init.cpp:1845:48

    The signed integer overflow can probably be avoided simply by re-arranging the terms in the conditional (nTime > nNow - nExpiryTimeout instead of nTime + nExpiryTimeout > nNow), and the invalid negation can likely be avoided by simply checking that the delta amount is valid before calling PrioritiseTransaction.

  2. practicalswift renamed this:
    Three UBSan warnings when loading corrpus mempool.dat files
    Three UBSan warnings when loading corrupt mempool.dat files
    on Jun 14, 2020
  3. MarcoFalke commented at 10:51 am on June 15, 2020: member

    The signed integer overflow can probably be avoided simply by re-arranging the terms in the conditional (nTime > nNow - nExpiryTimeout

    Seems reasonable. Mind creating a pr?

  4. practicalswift commented at 2:23 pm on June 15, 2020: contributor

    @MarcoFalke Actually I think this one is a good candidate for “good first issue” :)

    It is a good learning opportunity for someone who is interested in UBSan and fuzzing :) I’d be glad to coach any volunteer.

  5. MarcoFalke added the label good first issue on Jun 15, 2020
  6. MarcoFalke commented at 11:11 pm on June 15, 2020: member
    Ok, I’ve assigned good first issue to one of the fixes (https://github.com/bitcoin/bitcoin/issues/19278#issuecomment-644055262)
  7. MarcoFalke commented at 11:11 pm on June 15, 2020: member

    Want to work on this issue?

    For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

  8. MarcoFalke added the label Refactoring on Jun 15, 2020
  9. rajarshimaitra commented at 11:10 am on June 16, 2020: contributor
    Hi @practicalswift i have been meaning to learn fuzzing for some time. Have basic ideas, where do i start to understand it better for core? Willing to work on this issue.
  10. MarcoFalke commented at 11:20 am on June 16, 2020: member
    See doc/fuzzing.md
  11. practicalswift commented at 11:31 am on June 16, 2020: contributor
    @rajarshimaitra Great! I would suggest starting by trying to reproduce the three issues reported here by using the LoadMempool fuzzer from #19259 using the libFuzzer instructions in doc/fuzzing.md. Are you able to hit the three issues when fuzzing? :)
  12. jonatack commented at 11:33 am on June 16, 2020: member

    @rajarshimaitra You might find these informative. Marco hosted two fuzzing sessions. The third session discusses other techniques as well.

    https://bitcoincore.reviews/17860 https://bitcoincore.reviews/18521 https://bitcoincore.reviews/17639

  13. rajarshimaitra commented at 11:53 am on June 16, 2020: contributor
    Great, thanks for all the help guys. Will start looking into these.
  14. rajarshimaitra commented at 5:04 pm on June 19, 2020: contributor

    Hi, I have been able too reproduce the bug so far. Heres the libfuzzer log.

     0INFO: Seed: 3179298610
 1INFO: Loaded 1 modules   (521164 inline 8-bit counters): 521164 [0x3392d28, 0x34120f4), 
 2INFO: Loaded 1 PC tables (521164 PCs): 521164 [0x34120f8,0x3c05db8), 
 3INFO:     4602 files found in /home/raj/github-repo/qa-assets/fuzz_seed_corpus/validation_load_mempool/
 4INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
 5INFO: seed corpus: files: 4602 min: 1b max: 4096b total: 2882876b rss: 105Mb
 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
 7SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior util/moneystr.cpp:16:34 in 
 8validation.cpp:5064:23: runtime error: signed integer overflow: 9223372036854775806 + 1209600 cannot be represented in type 'long'
 9SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior validation.cpp:5064:23 in 
10txmempool.cpp:828:15: runtime error: signed integer overflow: 1224979098644774912 + 8969489304443158649 cannot be represented in type 'long'
11SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior txmempool.cpp:828:15 in 
12[#2048](/bitcoin-bitcoin/2048/)	pulse  cov: 7029 ft: 26938 corp: 471/63Kb exec/s: 1024 rss: 220Mb
13[#4096](/bitcoin-bitcoin/4096/)	pulse  cov: 7053 ft: 35505 corp: 789/272Kb exec/s: 682 rss: 306Mb

    Though it seems there’s one more signed integer overflow happening at txmempool (new one?). I need to look further into why they are occurinng. My question is how do I extract the artifact causing the crash investigate them further?

    Further clarification: It seems ubsan doesn’t automatically drops the artifact in working directory and stops execution unlike asan. Using UBSAN_OPTION=“halt_on_error=1” creates the artifact but only on the first crash. Is there any other ways to get all the three crashes?

  15. rajarshimaitra commented at 2:16 pm on June 25, 2020: contributor
    Hi. I have successfully reproduced the bugs and fixed the relevant locations. Should I create a fresh PR or should I push it to #19529?
  16. practicalswift commented at 2:37 pm on June 25, 2020: contributor
    @rajarshimaitra That’s excellent news! Please create a fresh PR. I’ll review it right away! :)
  17. rajarshimaitra referenced this in commit 8cc8548ace on Jun 25, 2020
  18. rajarshimaitra commented at 5:23 pm on June 25, 2020: contributor
    Done. :)
  19. rajarshimaitra referenced this in commit c58261d6f7 on Jul 1, 2020
  20. rajarshimaitra referenced this in commit f19dcd4e41 on Jul 3, 2020
  21. MarcoFalke referenced this in commit 027e51f715 on Nov 12, 2020
  22. sidhujag referenced this in commit b4151ac86c on Nov 12, 2020
  23. MarcoFalke commented at 10:39 am on May 10, 2021: member
    Duplicate of #20626
  24. MarcoFalke marked this as duplicate on May 10, 2021
  25. MarcoFalke closed this on May 10, 2021
  26. MarcoFalke removed the label good first issue on May 10, 2021
  27. MarcoFalke referenced this in commit dde7205c57 on Jun 27, 2022
  28. sidhujag referenced this in commit 758481e22d on Jun 27, 2022
  29. DrahtBot locked this on Aug 18, 2022


practicalswift MarcoFalke rajarshimaitra jonatack

Labels
Refactoring

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