wallet: heap-use-after-free on multiwallet shutdown #14163

issue MarcoFalke openend this issue on September 6, 2018
  1. MarcoFalke commented at 9:33 pm on September 6, 2018: member

    Steps to reproduce on current master adf27b531a7fcd7066ab6649e8073bd1895a823a:

    0./configure --with-sanitizers=address CC=clang CXX=clang++ && make -j 16
1ASAN_OPTIONS=detect_leaks=0 ./test/functional/wallet_multiwallet.py
2# Look in the leftover tempdir: cat ./node0/stderr/tmp*
3# Addresses can be converted with addr2line -e ...
  2. MarcoFalke added the label Wallet on Sep 6, 2018
  3. practicalswift commented at 11:40 am on September 7, 2018: contributor

    Oh, very nice find!

    We urgently need an ASAN Travis job :-)

  4. ken2812221 commented at 10:49 am on September 10, 2018: contributor

    By applying

     0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 1index e41f829f02..602c0df088 100644
 2--- a/src/wallet/wallet.cpp
 3+++ b/src/wallet/wallet.cpp
 4@@ -548,7 +548,9 @@ bool CWallet::HasWalletSpend(const uint256& txid) const
 5
 6 void CWallet::Flush(bool shutdown)
 7 {
 8+    WalletLogPrintf("CWallet::Flush Start\n");
 9     database->Flush(shutdown);
10+    WalletLogPrintf("CWallet::Flush Finish\n");
11 }
12
13 void CWallet::SyncMetaData(std::pair<TxSpends::iterator, TxSpends::iterator> range)

     02018-09-10T10:39:47.214696Z [w8] CWallet::Flush Start
 12018-09-10T10:39:47.214768Z BerkeleyEnvironment::Flush: Flush(false)
 22018-09-10T10:39:47.214831Z BerkeleyEnvironment::Flush: Flushing w8 (refcount = 0)...
 32018-09-10T10:39:47.214964Z BerkeleyEnvironment::Flush: w8 checkpoint
 42018-09-10T10:39:47.218020Z BerkeleyEnvironment::Flush: w8 detach
 52018-09-10T10:39:47.220806Z BerkeleyEnvironment::Flush: w8 closed
 62018-09-10T10:39:47.220943Z BerkeleyEnvironment::Flush: Flushing wallet.dat (refcount = 0)...
 72018-09-10T10:39:47.222169Z BerkeleyEnvironment::Flush: wallet.dat checkpoint
 82018-09-10T10:39:47.226511Z BerkeleyEnvironment::Flush: wallet.dat detach
 92018-09-10T10:39:47.228803Z BerkeleyEnvironment::Flush: wallet.dat closed
102018-09-10T10:39:47.228939Z BerkeleyEnvironment::Flush: Flush(false) took              14ms
112018-09-10T10:39:47.229057Z [w8] CWallet::Flush Finish
122018-09-10T10:39:47.229146Z [default wallet] CWallet::Flush Start
132018-09-10T10:39:47.229217Z BerkeleyEnvironment::Flush: Flush(false)
142018-09-10T10:39:47.229398Z BerkeleyEnvironment::Flush: Flush(false) took               0ms
152018-09-10T10:39:47.229482Z [default wallet] CWallet::Flush Finish

    w8 and [default wallet] files are in the same directory. That flush order is weird.

    They might use the same database?

  5. MarcoFalke commented at 9:18 pm on September 11, 2018: member

    Steps to reproduce (with valgrind):

     0./src/bitcoind -regtest -datadir=/tmp -printtoconsole
 1mv /tmp/regtest/wallets/wallet.dat /tmp/regtest/wallets/wallet_moved_no_dat
 2valgrind ./src/bitcoind -regtest -datadir=/tmp -printtoconsole -wallet="" -wallet=wallet_moved_no_dat
 3
 42018-09-11T21:16:07Z [default wallet] CWallet::Flush Finish
 52018-09-11T21:16:07Z [wallet_moved_no_dat] CWallet::Flush Start
 62018-09-11T21:16:07Z [wallet_moved_no_dat] CWallet::Flush Finish
 72018-09-11T21:16:07Z scheduler thread interrupt
 82018-09-11T21:16:07Z Dumped mempool: 0.002633s to copy, 0.003621s to dump
 92018-09-11T21:16:07Z [default wallet] CWallet::Flush Start
102018-09-11T21:16:07Z [default wallet] CWallet::Flush Finish
112018-09-11T21:16:07Z [wallet_moved_no_dat] CWallet::Flush Start
12==10214== Invalid read of size 1
13==10214==    at 0x427735: BerkeleyEnvironment::Flush(bool) (db.cpp:669)
14==10214==    by 0x427F0C: BerkeleyDatabase::Flush(bool) (db.cpp:798)
15==10214==    by 0x3DB5D8: CWallet::Flush(bool) (wallet.cpp:552)
16==10214==    by 0x3A906D: WalletInit::Stop() const (init.cpp:264)
17==10214==    by 0x1DA7BE: Shutdown() (init.cpp:241)
18==10214==    by 0x1CE7AA: AppInit(int, char**) (bitcoind.cpp:181)
19==10214==    by 0x1C3F4E: main (bitcoind.cpp:193)
20==10214==  Address 0x14990950 is 64 bytes inside a block of size 208 free'd
21==10214==    at 0x4C3029C: operator delete(void*) (vg_replace_malloc.c:576)
22==10214==    by 0x42C438: deallocate (new_allocator.h:125)
23==10214==    by 0x42C438: deallocate (alloc_traits.h:462)
24==10214==    by 0x42C438: _M_put_node (stl_tree.h:603)
25==10214==    by 0x42C438: _M_drop_node (stl_tree.h:670)
26==10214==    by 0x42C438: std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, BerkeleyEnvironment>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, BerkeleyEnvironment> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, BerkeleyEnvironment> > >::_M_erase(std::_Rb_tree_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, BerkeleyEnvironment> >*) (stl_tree.h:1874)
27==10214==    by 0x42C580: clear (stl_tree.h:1187)
28==10214==    by 0x42C580: _M_erase_aux (stl_tree.h:2504)
29==10214==    by 0x42C580: std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, BerkeleyEnvironment>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, BerkeleyEnvironment> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, BerkeleyEnvironment> > >::erase(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (stl_tree.h:2518)
30==10214==    by 0x427E19: erase (stl_map.h:1069)
31==10214==    by 0x427E19: BerkeleyEnvironment::Flush(bool) (db.cpp:700)
32==10214==    by 0x427F0C: BerkeleyDatabase::Flush(bool) (db.cpp:798)
33==10214==    by 0x3DB5D8: CWallet::Flush(bool) (wallet.cpp:552)
34==10214==    by 0x3A906D: WalletInit::Stop() const (init.cpp:264)
35==10214==    by 0x1DA7BE: Shutdown() (init.cpp:241)
36==10214==    by 0x1CE7AA: AppInit(int, char**) (bitcoind.cpp:181)
37==10214==    by 0x1C3F4E: main (bitcoind.cpp:193)
38==10214==  Block was alloc'd at
39==10214==    at 0x4C2F226: operator new(unsigned long) (vg_replace_malloc.c:334)
40==10214==    by 0x42C1D0: allocate (new_allocator.h:111)
41==10214==    by 0x42C1D0: allocate (alloc_traits.h:436)
42==10214==    by 0x42C1D0: _M_get_node (stl_tree.h:599)
43==10214==    by 0x42C1D0: _M_create_node<const std::piecewise_construct_t&, std::tuple<const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&>, std::tuple<boost::filesystem::path&> > (stl_tree.h:653)
44==10214==    by 0x42C1D0: std::pair<std::_Rb_tree_iterator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, BerkeleyEnvironment> >, bool> std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, BerkeleyEnvironment>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, BerkeleyEnvironment> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, BerkeleyEnvironment> > >::_M_emplace_unique<std::piecewise_construct_t const&, std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&>, std::tuple<boost::filesystem::path&> >(std::piecewise_construct_t const&, std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&>&&, std::tuple<boost::filesystem::path&>&&) (stl_tree.h:2367)
45==10214==    by 0x426D81: emplace<const std::piecewise_construct_t&, std::tuple<const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&>, std::tuple<boost::filesystem::path&> > (stl_map.h:575)
46==10214==    by 0x426D81: GetWalletEnv(boost::filesystem::path const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (db.cpp:79)
47==10214==    by 0x426EDC: BerkeleyBatch::VerifyEnvironment(boost::filesystem::path const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (db.cpp:322)
48==10214==    by 0x3DF0C7: CWallet::Verify(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (wallet.cpp:3850)
49==10214==    by 0x3ABB4A: WalletInit::Verify() const (init.cpp:217)
50==10214==    by 0x1DB8D0: AppInitMain() (init.cpp:1283)
51==10214==    by 0x1CE79C: AppInit(int, char**) (bitcoind.cpp:167)
52==10214==    by 0x1C3F4E: main (bitcoind.cpp:193)
53==10214== 
542018-09-11T21:16:07Z [wallet_moved_no_dat] CWallet::Flush Finish
552018-09-11T21:16:07Z [default wallet] Releasing wallet
562018-09-11T21:16:07Z [default wallet] CWallet::Flush Start
572018-09-11T21:16:07Z [default wallet] CWallet::Flush Finish
582018-09-11T21:16:07Z [wallet_moved_no_dat] Releasing wallet
592018-09-11T21:16:07Z [wallet_moved_no_dat] CWallet::Flush Start
602018-09-11T21:16:07Z [wallet_moved_no_dat] CWallet::Flush Finish
612018-09-11T21:16:08Z Shutdown: done
  6. MarcoFalke deleted a comment on Sep 11, 2018
  7. MarcoFalke commented at 9:26 pm on September 11, 2018: member
    Note that it only happens when moving the wallet.dat to a file without the .dat extension.
  8. ryanofsky commented at 7:33 pm on September 13, 2018: member
    Not sure in this context, but wallet name needs to either be folder containing a “wallet.dat” file or an arbitrary filename of an existing file in the root wallet dir (for backwards compatibility).
  9. MarcoFalke commented at 7:43 pm on September 13, 2018: member
    In this case ./wallets/wallet_moved_no_dat is “an arbitrary filename of an existing file in the root wallet dir” as well as ./wallets/wallet_moved.dat (with the extension). But the bug is only seen when the “.dat” is missing.
  10. MarcoFalke commented at 7:21 pm on September 17, 2018: member
    I think I was wrong that this only happens when the .dat extension is missing. At least now I can reproduce regardless of that.
  11. MarcoFalke commented at 7:27 pm on September 17, 2018: member
    Maybe the original filename is embedded somewhere in the wallet file and some pointers are freed based on that name as opposed to the current file name?
  12. ken2812221 commented at 11:14 am on September 23, 2018: contributor

    I think I was wrong that this only happens when the .dat extension is missing. At least now I can reproduce regardless of that.

    If the filename is ends with .dat, it can be reproduced by call bitcoin-cli loadwallet w8.dat twice. The first time would fail, and the second time would succeed.

  13. MarcoFalke commented at 11:20 pm on November 20, 2018: member
    #14320 was merged, but the issue still exists on master
  14. practicalswift referenced this in commit 592d5e5a0a on Nov 26, 2018
  15. practicalswift referenced this in commit 53daef6983 on Nov 26, 2018
  16. practicalswift referenced this in commit f370c153b7 on Nov 26, 2018
  17. practicalswift commented at 10:32 pm on November 27, 2018: contributor

    An ASAN suppression for this issue is now in master:

    https://github.com/bitcoin/bitcoin/blob/6541d59ddc2b1b475187e6cbe76a50ee6e85b421/.travis.yml#L111

  18. MarcoFalke added this to the milestone 0.18.0 on Nov 27, 2018
  19. MarcoFalke referenced this in commit b3a715301a on Feb 1, 2019
  20. MarcoFalke closed this on Feb 1, 2019
  21. jasonbcox referenced this in commit 8915b2f748 on Oct 25, 2019
  22. vijaydasmp referenced this in commit c495d30e3b on Sep 24, 2021
  23. vijaydasmp referenced this in commit be0e8051ac on Sep 24, 2021
  24. vijaydasmp referenced this in commit acc5f9887b on Sep 25, 2021
  25. vijaydasmp referenced this in commit 08872db34b on Sep 25, 2021
  26. vijaydasmp referenced this in commit 9a2126302e on Sep 27, 2021
  27. vijaydasmp referenced this in commit 2363b6465c on Sep 28, 2021
  28. vijaydasmp referenced this in commit c484d21ec4 on Sep 28, 2021
  29. vijaydasmp referenced this in commit 6c3e37d994 on Sep 30, 2021
  30. vijaydasmp referenced this in commit 439bbdfa0e on Oct 1, 2021
  31. MarcoFalke locked this on Dec 16, 2021


MarcoFalke practicalswift ken2812221 ryanofsky

Labels
Wallet

Milestone
0.18.0

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-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me