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

MarcoFalke commented at 9:33 PM on September 6, 2018: member

Steps to reproduce on current master adf27b531a7fcd7066ab6649e8073bd1895a823a:

./configure --with-sanitizers=address CC=clang CXX=clang++ && make -j 16
ASAN_OPTIONS=detect_leaks=0 ./test/functional/wallet_multiwallet.py
# Look in the leftover tempdir: cat ./node0/stderr/tmp*
# Addresses can be converted with addr2line -e ...

MarcoFalke added the label Wallet on Sep 6, 2018

practicalswift commented at 11:40 AM on September 7, 2018: contributor

Oh, very nice find!

We urgently need an ASAN Travis job :-)

ken2812221 commented at 10:49 AM on September 10, 2018: contributor

By applying

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index e41f829f02..602c0df088 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -548,7 +548,9 @@ bool CWallet::HasWalletSpend(const uint256& txid) const

 void CWallet::Flush(bool shutdown)
 {
+    WalletLogPrintf("CWallet::Flush Start\n");
     database->Flush(shutdown);
+    WalletLogPrintf("CWallet::Flush Finish\n");
 }

 void CWallet::SyncMetaData(std::pair<TxSpends::iterator, TxSpends::iterator> range)

2018-09-10T10:39:47.214696Z [w8] CWallet::Flush Start
2018-09-10T10:39:47.214768Z BerkeleyEnvironment::Flush: Flush(false)
2018-09-10T10:39:47.214831Z BerkeleyEnvironment::Flush: Flushing w8 (refcount = 0)...
2018-09-10T10:39:47.214964Z BerkeleyEnvironment::Flush: w8 checkpoint
2018-09-10T10:39:47.218020Z BerkeleyEnvironment::Flush: w8 detach
2018-09-10T10:39:47.220806Z BerkeleyEnvironment::Flush: w8 closed
2018-09-10T10:39:47.220943Z BerkeleyEnvironment::Flush: Flushing wallet.dat (refcount = 0)...
2018-09-10T10:39:47.222169Z BerkeleyEnvironment::Flush: wallet.dat checkpoint
2018-09-10T10:39:47.226511Z BerkeleyEnvironment::Flush: wallet.dat detach
2018-09-10T10:39:47.228803Z BerkeleyEnvironment::Flush: wallet.dat closed
2018-09-10T10:39:47.228939Z BerkeleyEnvironment::Flush: Flush(false) took              14ms
2018-09-10T10:39:47.229057Z [w8] CWallet::Flush Finish
2018-09-10T10:39:47.229146Z [default wallet] CWallet::Flush Start
2018-09-10T10:39:47.229217Z BerkeleyEnvironment::Flush: Flush(false)
2018-09-10T10:39:47.229398Z BerkeleyEnvironment::Flush: Flush(false) took               0ms
2018-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?

MarcoFalke commented at 9:18 PM on September 11, 2018: member

Steps to reproduce (with valgrind):

./src/bitcoind -regtest -datadir=/tmp -printtoconsole
mv /tmp/regtest/wallets/wallet.dat /tmp/regtest/wallets/wallet_moved_no_dat
valgrind ./src/bitcoind -regtest -datadir=/tmp -printtoconsole -wallet="" -wallet=wallet_moved_no_dat

2018-09-11T21:16:07Z [default wallet] CWallet::Flush Finish
2018-09-11T21:16:07Z [wallet_moved_no_dat] CWallet::Flush Start
2018-09-11T21:16:07Z [wallet_moved_no_dat] CWallet::Flush Finish
2018-09-11T21:16:07Z scheduler thread interrupt
2018-09-11T21:16:07Z Dumped mempool: 0.002633s to copy, 0.003621s to dump
2018-09-11T21:16:07Z [default wallet] CWallet::Flush Start
2018-09-11T21:16:07Z [default wallet] CWallet::Flush Finish
2018-09-11T21:16:07Z [wallet_moved_no_dat] CWallet::Flush Start
==10214== Invalid read of size 1
==10214==    at 0x427735: BerkeleyEnvironment::Flush(bool) (db.cpp:669)
==10214==    by 0x427F0C: BerkeleyDatabase::Flush(bool) (db.cpp:798)
==10214==    by 0x3DB5D8: CWallet::Flush(bool) (wallet.cpp:552)
==10214==    by 0x3A906D: WalletInit::Stop() const (init.cpp:264)
==10214==    by 0x1DA7BE: Shutdown() (init.cpp:241)
==10214==    by 0x1CE7AA: AppInit(int, char**) (bitcoind.cpp:181)
==10214==    by 0x1C3F4E: main (bitcoind.cpp:193)
==10214==  Address 0x14990950 is 64 bytes inside a block of size 208 free'd
==10214==    at 0x4C3029C: operator delete(void*) (vg_replace_malloc.c:576)
==10214==    by 0x42C438: deallocate (new_allocator.h:125)
==10214==    by 0x42C438: deallocate (alloc_traits.h:462)
==10214==    by 0x42C438: _M_put_node (stl_tree.h:603)
==10214==    by 0x42C438: _M_drop_node (stl_tree.h:670)
==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)
==10214==    by 0x42C580: clear (stl_tree.h:1187)
==10214==    by 0x42C580: _M_erase_aux (stl_tree.h:2504)
==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)
==10214==    by 0x427E19: erase (stl_map.h:1069)
==10214==    by 0x427E19: BerkeleyEnvironment::Flush(bool) (db.cpp:700)
==10214==    by 0x427F0C: BerkeleyDatabase::Flush(bool) (db.cpp:798)
==10214==    by 0x3DB5D8: CWallet::Flush(bool) (wallet.cpp:552)
==10214==    by 0x3A906D: WalletInit::Stop() const (init.cpp:264)
==10214==    by 0x1DA7BE: Shutdown() (init.cpp:241)
==10214==    by 0x1CE7AA: AppInit(int, char**) (bitcoind.cpp:181)
==10214==    by 0x1C3F4E: main (bitcoind.cpp:193)
==10214==  Block was alloc'd at
==10214==    at 0x4C2F226: operator new(unsigned long) (vg_replace_malloc.c:334)
==10214==    by 0x42C1D0: allocate (new_allocator.h:111)
==10214==    by 0x42C1D0: allocate (alloc_traits.h:436)
==10214==    by 0x42C1D0: _M_get_node (stl_tree.h:599)
==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)
==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)
==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)
==10214==    by 0x426D81: GetWalletEnv(boost::filesystem::path const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (db.cpp:79)
==10214==    by 0x426EDC: BerkeleyBatch::VerifyEnvironment(boost::filesystem::path const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (db.cpp:322)
==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)
==10214==    by 0x3ABB4A: WalletInit::Verify() const (init.cpp:217)
==10214==    by 0x1DB8D0: AppInitMain() (init.cpp:1283)
==10214==    by 0x1CE79C: AppInit(int, char**) (bitcoind.cpp:167)
==10214==    by 0x1C3F4E: main (bitcoind.cpp:193)
==10214== 
2018-09-11T21:16:07Z [wallet_moved_no_dat] CWallet::Flush Finish
2018-09-11T21:16:07Z [default wallet] Releasing wallet
2018-09-11T21:16:07Z [default wallet] CWallet::Flush Start
2018-09-11T21:16:07Z [default wallet] CWallet::Flush Finish
2018-09-11T21:16:07Z [wallet_moved_no_dat] Releasing wallet
2018-09-11T21:16:07Z [wallet_moved_no_dat] CWallet::Flush Start
2018-09-11T21:16:07Z [wallet_moved_no_dat] CWallet::Flush Finish
2018-09-11T21:16:08Z Shutdown: done

MarcoFalke deleted a comment on Sep 11, 2018

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.

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

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.

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.

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?

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.

MarcoFalke commented at 11:20 PM on November 20, 2018: member

#14320 was merged, but the issue still exists on master

practicalswift referenced this in commit 592d5e5a0a on Nov 26, 2018

practicalswift referenced this in commit 53daef6983 on Nov 26, 2018

practicalswift referenced this in commit f370c153b7 on Nov 26, 2018

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

MarcoFalke added this to the milestone 0.18.0 on Nov 27, 2018

MarcoFalke referenced this in commit b3a715301a on Feb 1, 2019

MarcoFalke closed this on Feb 1, 2019

jasonbcox referenced this in commit 8915b2f748 on Oct 25, 2019

vijaydasmp referenced this in commit c495d30e3b on Sep 24, 2021

vijaydasmp referenced this in commit be0e8051ac on Sep 24, 2021

vijaydasmp referenced this in commit acc5f9887b on Sep 25, 2021

vijaydasmp referenced this in commit 08872db34b on Sep 25, 2021

vijaydasmp referenced this in commit 9a2126302e on Sep 27, 2021

vijaydasmp referenced this in commit 2363b6465c on Sep 28, 2021

vijaydasmp referenced this in commit c484d21ec4 on Sep 28, 2021

vijaydasmp referenced this in commit 6c3e37d994 on Sep 30, 2021

vijaydasmp referenced this in commit 439bbdfa0e on Oct 1, 2021

MarcoFalke locked this on Dec 16, 2021