Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks by luke-jr · Pull Request #19502 · bitcoin/bitcoin

luke-jr · 2020-07-13T03:05:49Z

Previously, an exception would be thrown, which could kill the node in some circumstances.

Includes test changes to cause failure.

Review with ?w=1

jonatack

ACK bca838d38c54f3df3c85da54e23857f06f07c6bb

test/functional/wallet_multiwallet.py

DrahtBot · 2020-07-13T05:52:52Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: List all wallets in non-SQLite and non-BDB builds #20275 (wallet: List SQLite wallets in non-SQLite builds by ryanofsky)

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.

jonatack · 2020-07-14T04:05:54Z

ACK 4f0cbc4

meshcollider

Code review ACK 4f0cbc4

hebasto

I've reviewed 4f0cbc4 and ready to ACK after rebasing.

luke-jr · 2020-10-12T15:53:29Z

Rebased

hebasto

ACK 1e77a8d5a4faa0757e53e0284662b249eebc3ef7, I have reviewed the code and it looks OK, I agree it can be merged.

jonatack

re-ACK 1e77a8d5a4faa0757e53e0284662b249eebc3ef7

In the new test, the new exception prints:

[httpworker.1] ListWalletDir: Error scanning /tmp/bitcoin_func_test_ql5k4c0f/node0/regtest/wallets/self_walletdat_symlink: boost::filesystem::status: Too many levels of symbolic links: "/tmp/bitcoin_func_test_ql5k4c0f/node0/regtest/wallets/self_walletdat_symlink/wallet.dat"

[httpworker.1] ListWalletDir: Error scanning /tmp/bitcoin_func_test_ql5k4c0f/node0/regtest/wallets/self_walletdat_symlink/wallet.dat: boost::filesystem::status: Too many levels of symbolic links: "/tmp/bitcoin_func_test_ql5k4c0f/node0/regtest/wallets/self_walletdat_symlink/wallet.dat"`

Saibato · 2020-10-13T08:14:21Z

nit: Before merge please note; this ( #19502 and ( #20080 or #19933 ) should merged together, since they vice versa would create new issues, like shorted wallet names.

If we have boost cononical best first #20080 and then this.

luke-jr · 2020-10-13T12:11:27Z

I don't see what new issues (aside from rebasing) would be created by merging them separately?

Saibato · 2020-10-13T18:07:28Z

I don't see what new issues (aside from rebasing) would be created by merging them separately?

iirc this a fix for the boost link problem and ( evil wallet ) and makes #18095 obsolete.

̶B̶u̶t̶ ̶i̶f̶ ̶t̶h̶i̶s̶ ̶g̶e̶t̶ ̶m̶e̶r̶g̶e̶d̶ ̶f̶i̶r̶s̶t̶,̶ ̶t̶h̶e̶n̶ ̶r̶e̶a̶l̶ ̶s̶h̶o̶r̶t̶ ̶f̶i̶l̶e̶n̶a̶m̶e̶s̶ ̶w̶a̶l̶l̶e̶t̶s̶ ̶f̶r̶o̶m̶ ̶t̶h̶e̶ ̶l̶a̶t̶e̶r̶ ̶f̶o̶u̶n̶d̶ ̶b̶u̶g̶ ̶c̶o̶u̶l̶d̶ ̶b̶e̶ ̶c̶r̶e̶a̶t̶e̶d̶ ̶f̶r̶o̶m̶ ̶a̶ ̶b̶o̶g̶u̶s̶ ̶d̶i̶r̶n̶a̶m̶e̶ ̶o̶n̶ ̶d̶i̶s̶k̶ ̶f̶o̶r̶ ̶r̶e̶a̶l̶.̶

Diff-minimised Github-Pull: bitcoin#19502 Rebased-From: 4e1ce04e02ad9276b60084e189824718a6f20738

…llet.dat loops are ignored Github-Pull: bitcoin#19502 Rebased-From: 1e77a8d5a4faa0757e53e0284662b249eebc3ef7

meshcollider

re-utACK 1e77a8d5a4faa0757e53e0284662b249eebc3ef7

Saibato · 2020-10-14T13:02:53Z

But if this get merged first, then real short filenames wallets from the later found bug could be created from a bogus dirname on disk for real.

@luke-jr
Rechecked complies from the sources: To open non exitsting files, is no longer the case with current master and knots so no merge sequence preferences here any longer. Was only a problem short after those errors where found and exits only if direct on top of last releases before/= 0.20

hebasto · 2020-10-20T22:01:52Z

I revoke my ACK as this change makes possible an infinity loop.
To reproduce the bug, one could create a directory without access permissions, e.g.:

$ ls -l /home/hebasto/bitcoin-data-dir/wallets
total 1456
drwx------ 3 hebasto hebasto    4096 Oct  3 11:16 20191109-main
drwx------ 2 root    root       4096 Oct 20 23:58 crash
-rw-r--r-- 1 hebasto hebasto       0 Mar  5  2020 db.log
drwx------ 3 hebasto hebasto    4096 Oct 20 23:56 w20201015.bdb
drwx------ 3 hebasto hebasto    4096 Oct 20 23:56 w20201015.desc
-rw-r----- 1 hebasto hebasto 1474560 Sep 12 16:08 wallet.dat

I suggest to combine into this PR the approach from #18095:

bitcoin/src/wallet/walletutil.cpp

Lines 42 to 53 in 6904a30

    
           auto it = fs::recursive_directory_iterator(wallet_dir, ec); 
        
           if (ec) { 
        
               LogPrintf("%s: iterator: %s %s\n", __func__, ec.message(), it->path().string()); 
        
               return paths; 
        
           } 
        
           for (; it != fs::recursive_directory_iterator(); it.increment(ec)) { 
        
               if (ec) { 
        
                   LogPrintf("%s: increment: %s %s\n", __func__, ec.message(), it->path().string()); 
        
                   it.no_push(); 
        
                   continue; 
        
               }

with credits to @uhliksk

luke-jr · 2020-10-29T19:04:28Z

@hebasto Couldn't reproduce, but it did cause listwallets to miss other wallets. Added a fix and test.

hebasto · 2020-10-29T20:31:20Z

Tested 49cc6919c4f9ef5a33ad5948a1b43496b454fab0, still able to reproduce an infinite loop:

cd <path-to-wallet-dir>
sudo mkdir crash
sudo chown 0700 crash

then start a node, and initiate a ListWalletDir call in any way.

hebasto

Approach ACK 44c0bc31f4fadc932877d9764402644c84f3be4c, tested on Linux Mint 20 (x86_64).

Instead of an infinite loop having a nice log message:

2020-10-29T20:41:26Z [main] ListWalletDir: Error scanning /home/hebasto/.bitcoin/testnet3/wallets/crash: boost::filesystem::status: Permission denied: "/home/hebasto/.bitcoin/testnet3/wallets/crash/wallet.dat"

Keeping reviewing the last commit.

test/functional/wallet_multiwallet.py

hebasto

ACK 44c0bc31f4fadc932877d9764402644c84f3be4c

…llet.dat loops are ignored

hebasto

re-ACK 24d2d33, rebased only since my previous review.

promag

Tested ACK 24d2d33, test change fails on master.

nit, could squash.

promag · 2020-11-07T12:09:13Z

src/wallet/walletutil.cpp

            }
+        } catch (const std::exception& e) {
+            LogPrintf("%s: Error scanning %s: %s\n", __func__, it->path().string(), e.what());
+            it.no_push();


meshcollider

utACK 24d2d33

…WalletD…

fanquake added the Wallet label Jul 13, 2020

jonatack reviewed Jul 13, 2020

View reviewed changes

test/functional/wallet_multiwallet.py Outdated Show resolved Hide resolved

This was referenced Jul 13, 2020

wallet: let Listwalletdir do not iterate through our blocksdata. #19419

Closed

Replace boost::filesystem with std::filesystem #19245

Closed

wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077

Merged

luke-jr force-pushed the bugfix_listwalletdir_errors branch from bca838d to 4f0cbc4 Compare July 13, 2020 20:11

bitcoin deleted a comment from Aonny112 Jul 14, 2020

meshcollider reviewed Jul 31, 2020

View reviewed changes

DrahtBot mentioned this pull request Sep 29, 2020

test: Get rid of default wallet hacks #20034

Merged

DrahtBot added the Needs rebase label Oct 2, 2020

hebasto approved these changes Oct 12, 2020

View reviewed changes

luke-jr force-pushed the bugfix_listwalletdir_errors branch from 4f0cbc4 to 1e77a8d Compare October 12, 2020 15:53

DrahtBot removed the Needs rebase label Oct 12, 2020

hebasto approved these changes Oct 12, 2020

View reviewed changes

DrahtBot mentioned this pull request Oct 12, 2020

rpc, wallet: Expose database format in getwalletinfo #20125

Merged

jonatack reviewed Oct 12, 2020

View reviewed changes

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Oct 13, 2020

Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks

254d712

Diff-minimised Github-Pull: bitcoin#19502 Rebased-From: 4e1ce04e02ad9276b60084e189824718a6f20738

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Oct 13, 2020

QA: wallet_multiwallet: Check that recursive symlink directory and wa…

5d8ab56

…llet.dat loops are ignored Github-Pull: bitcoin#19502 Rebased-From: 1e77a8d5a4faa0757e53e0284662b249eebc3ef7

meshcollider reviewed Oct 14, 2020

View reviewed changes

DrahtBot added the Needs rebase label Oct 15, 2020

luke-jr force-pushed the bugfix_listwalletdir_errors branch from 1e77a8d to 9f74b7b Compare October 15, 2020 19:32

luke-jr requested review from hebasto and meshcollider October 15, 2020 19:32

DrahtBot mentioned this pull request Oct 19, 2020

wallet: Make -wallet setting not create wallets #20186

Merged

hebasto mentioned this pull request Oct 20, 2020

Fix crashes and infinite loop in ListWalletDir() #18095

Closed

DrahtBot mentioned this pull request Oct 29, 2020

wallet: Create named SQLite wallet files instead of wallet directories #20260

Closed

DrahtBot added the Needs rebase label Oct 29, 2020

luke-jr force-pushed the bugfix_listwalletdir_errors branch from 9f74b7b to 49cc691 Compare October 29, 2020 18:08

DrahtBot removed the Needs rebase label Oct 29, 2020

luke-jr requested a review from hebasto October 29, 2020 19:04

luke-jr force-pushed the bugfix_listwalletdir_errors branch 2 times, most recently from 49cc691 to 44c0bc3 Compare October 29, 2020 20:32

hebasto reviewed Oct 29, 2020

View reviewed changes

test/functional/wallet_multiwallet.py Outdated Show resolved Hide resolved

This was referenced Oct 30, 2020

Disable and fix tests for when BDB is not compiled #20267

Merged

tests: Update more tests to work with descriptor wallets #18788

Merged

hebasto approved these changes Oct 30, 2020

View reviewed changes

DrahtBot mentioned this pull request Oct 30, 2020

wallet: List all wallets in non-SQLite and non-BDB builds #20275

Merged

DrahtBot added the Needs rebase label Nov 2, 2020

luke-jr added 2 commits November 6, 2020 04:17

Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks

69f59af

QA: wallet_multiwallet: Check that recursive symlink directory and wa…

24d2d33

…llet.dat loops are ignored

luke-jr force-pushed the bugfix_listwalletdir_errors branch from 44c0bc3 to 24d2d33 Compare November 6, 2020 04:36

DrahtBot removed the Needs rebase label Nov 6, 2020

hebasto approved these changes Nov 6, 2020

View reviewed changes

promag reviewed Nov 7, 2020

View reviewed changes

meshcollider reviewed Nov 11, 2020

View reviewed changes

meshcollider merged commit c2d8ba6 into bitcoin:master Nov 12, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 12, 2020

Merge bitcoin#19502: Bugfix: Wallet: Soft-fail exceptions within List…

bab8f8e

…WalletD…

promag mentioned this pull request Nov 15, 2020

Bitcoin core locks up when hovering over Open Wallet menu item #20397

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

luke-jr commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack commented Jul 14, 2020

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Oct 12, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Saibato commented Oct 13, 2020

Uh oh!

luke-jr commented Oct 13, 2020

Uh oh!

Saibato commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Saibato commented Oct 14, 2020

Uh oh!

hebasto commented Oct 20, 2020

Uh oh!

luke-jr commented Oct 29, 2020

Uh oh!

hebasto commented Oct 29, 2020

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Nov 7, 2020

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

luke-jr commented Jul 13, 2020 •

edited

Loading

DrahtBot commented Jul 13, 2020 •

edited

Loading

Saibato commented Oct 13, 2020 •

edited

Loading

hebasto left a comment •

edited

Loading

hebasto left a comment •

edited

Loading