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

luke-jr commented at 3:05 am on July 13, 2020: member

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

Includes test changes to cause failure.

Review with ?w=1

fanquake added the label Wallet on Jul 13, 2020

in test/functional/wallet_multiwallet.py:85 in bca838d38c outdated

49@@ -50,6 +50,11 @@ def wallet_file(name):
50         os.mkdir(wallet_dir('w7'))
51         os.symlink('w7', wallet_dir('w7_symlink'))
52 
53+        os.symlink('..', wallet_dir('recursive_dir_symlink'))
54+
55+        os.mkdir(wallet_dir('self_walletdat_symlink'))
56+        os.symlink('wallet.dat', wallet_dir('self_walletdat_symlink/wallet.dat'))

jonatack commented at 5:49 am on July 13, 2020:

Verified that this does cause the test to fail at line 79 without the changes in this PR.

Could also assert that the new logging is taking place with this at line 79 or just after:

0with self.nodes[0].assert_debug_log(expected_msgs=['Too many levels of symbolic links', 'Error scanning']):
1    self.nodes[0].listwalletdir()['wallets']

jonatack commented at 5:54 am on July 13, 2020:

(or combined with the existing line 79 to share the same listwalletdir call)

0-        assert_equal(sorted(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), ['', os.path.join('sub', 'w5'), 'w', 'w1', 'w2', 'w3', 'w7', 'w7_symlink', 'w8'])
1+        with self.nodes[0].assert_debug_log(expected_msgs=['Too many levels of symbolic links', 'Error scanning']):
2+            list = self.nodes[0].listwalletdir()['wallets']
3+        assert_equal(sorted(map(lambda w: w['name'], list)), ['', os.path.join('sub', 'w5'), 'w', 'w1', 'w2', 'w3', 'w7', 'w7_symlink', 'w8'])

jonatack commented at 5:50 am on July 13, 2020: member

ACK bca838d38c54f3df3c85da54e23857f06f07c6bb

DrahtBot commented at 5:52 am on July 13, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

luke-jr force-pushed on Jul 13, 2020

jonatack commented at 4:05 am on July 14, 2020: member

ACK 4f0cbc4

MarcoFalke deleted a comment on Jul 14, 2020

meshcollider commented at 11:52 am on July 31, 2020: contributor

Code review ACK 4f0cbc4bc74dd87bc187fb839218b1fa64cfb205

DrahtBot added the label Needs rebase on Oct 2, 2020

hebasto approved

hebasto commented at 2:00 pm on October 12, 2020: member

I’ve reviewed 4f0cbc4bc74dd87bc187fb839218b1fa64cfb205 and ready to ACK after rebasing.

luke-jr force-pushed on Oct 12, 2020

luke-jr commented at 3:53 pm on October 12, 2020: member

Rebased

DrahtBot removed the label Needs rebase on Oct 12, 2020

hebasto approved

hebasto commented at 4:07 pm on October 12, 2020: member

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

jonatack commented at 7:31 pm on October 12, 2020: member

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 commented at 8:14 am on October 13, 2020: contributor

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 commented at 12:11 pm on October 13, 2020: member

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

Saibato commented at 6:07 pm on October 13, 2020: contributor

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

luke-jr referenced this in commit 254d712064 on Oct 13, 2020

luke-jr referenced this in commit 5d8ab56548 on Oct 13, 2020

meshcollider commented at 10:39 am on October 14, 2020: contributor

re-utACK 1e77a8d5a4faa0757e53e0284662b249eebc3ef7

Saibato commented at 1:02 pm on October 14, 2020: contributor

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

DrahtBot added the label Needs rebase on Oct 15, 2020

luke-jr force-pushed on Oct 15, 2020

luke-jr requested review from hebasto on Oct 15, 2020

luke-jr requested review from meshcollider on Oct 15, 2020

luke-jr requested review from jonatack on Oct 15, 2020

luke-jr commented at 7:33 pm on October 15, 2020: member

Rebased again

DrahtBot removed the label Needs rebase on Oct 15, 2020

laanwj added this to the milestone 0.21.0 on Oct 15, 2020

hebasto approved

hebasto commented at 10:00 am on October 16, 2020: member

~~re-ACK 9f74b7b3beba182d3b3118bf331515ef6e5ffcf2~~, only rebased since my previous review, verified with git range-diff master 1e77a8d5a 9f74b7b3b.

Revoked. See #19502 (comment)

hebasto commented at 10:01 pm on October 20, 2020: member

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

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

I suggest to combine into this PR the approach from #18095: https://github.com/bitcoin/bitcoin/blob/6904a309194c7ddf5a071d43e6c2bf892e8fe396/src/wallet/walletutil.cpp#L42-L53 with credits to @uhliksk

DrahtBot added the label Needs rebase on Oct 29, 2020

luke-jr force-pushed on Oct 29, 2020

DrahtBot removed the label Needs rebase on Oct 29, 2020

luke-jr commented at 7:04 pm on October 29, 2020: member

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

luke-jr requested review from hebasto on Oct 29, 2020

hebasto commented at 8:31 pm on October 29, 2020: member

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

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

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

luke-jr force-pushed on Oct 29, 2020

hebasto commented at 8:43 pm on October 29, 2020: member

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

Instead of an infinite loop having a nice log message:

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

in test/functional/wallet_multiwallet.py:128 in 44c0bc31f4 outdated

114@@ -109,7 +115,16 @@ def wallet_file(name):
115             self.nodes[0].createwallet(wallet_name, descriptors=False)
116         for wallet_name in wallet_names[-2:]:
117             self.nodes[0].loadwallet(wallet_name)
118-        assert_equal(sorted(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), [self.default_wallet_name, os.path.join('sub', 'w5'), 'w', 'w1', 'w2', 'w3', 'w7', 'w7_symlink', 'w8'])
119+
120+        os.mkdir(wallet_dir('no_access'))
121+        os.chmod(wallet_dir('no_access'), 0)

hebasto commented at 8:57 pm on October 29, 2020:

44c0bc31f4fadc932877d9764402644c84f3be4c

If this line is commented out, the test still passing.

luke-jr commented at 9:21 pm on October 29, 2020:

???

hebasto commented at 9:29 pm on October 29, 2020:

If initial state is changed, i.e., the no_access directory actually has access, the test should fail, right?

luke-jr commented at 1:21 am on October 30, 2020:

No, it’s just there to make sure it doesn’t break things. We’re testing bitcoind, not the OS…

(And in fact, chmod won’t work to deny read access on Windows)

hebasto approved

hebasto commented at 10:45 am on October 30, 2020: member

ACK 44c0bc31f4fadc932877d9764402644c84f3be4c

DrahtBot added the label Needs rebase on Nov 2, 2020

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

QA: wallet_multiwallet: Check that recursive symlink directory and wallet.dat loops are ignored 24d2d3341d

luke-jr force-pushed on Nov 6, 2020

DrahtBot removed the label Needs rebase on Nov 6, 2020

hebasto approved

hebasto commented at 9:15 am on November 6, 2020: member

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

in src/wallet/walletutil.cpp:76 in 24d2d3341d

88+                    paths.emplace_back(path);
89+                }
90             }
91+        } catch (const std::exception& e) {
92+            LogPrintf("%s: Error scanning %s: %s\n", __func__, it->path().string(), e.what());
93+            it.no_push();

promag commented at 12:09 pm on November 7, 2020:

nice, TIL

promag commented at 12:18 pm on November 7, 2020: member

Tested ACK 24d2d3341d07509ad3f37bb6f130446ad20ac807, test change fails on master.

nit, could squash.

meshcollider commented at 11:50 pm on November 11, 2020: contributor

utACK 24d2d3341d07509ad3f37bb6f130446ad20ac807

meshcollider merged this on Nov 12, 2020

meshcollider closed this on Nov 12, 2020

sidhujag referenced this in commit bab8f8e1bf on Nov 12, 2020

DrahtBot locked this on Feb 15, 2022

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

Conflicts