Fix crashes and infinite loop in ListWalletDir() #18095

uhliksk · 2020-02-07T20:52:09Z

Crashes:
If wallet is checking existence of wallet.dat in directory where access is denied then fs::exists() without ec parameter will cause wallet crash.

Reproduction steps on Linux platform:

Type: mkdir ~/.bitcoin/crash
Type: sudo chown root:root ~/.bitcoin/crash
Run bitcoin-qt
Go to menu "File" -> "Open Wallet", wallet will crash immediately

It's fixed by adding "ec" parameter into fs::exists() function to handle any error correctly.

Infinite loop:
If iterator is not able to increment because of access denied to next level of recursive scan it will stay on the same position indefinitely causing wallet to stop responding.

Reproduction steps on Windows platform:

Create new folder into bitcoin data folder (for example: %Appdata%\Roaming\Bitcoin\Loop)
Deny access to newly created folder for current user
Run bitcoin-qt.exe
Go to menu "File" -> "Open Wallet", wallet will hang indefinitely

It's fixed by disabling recursion if iterator increment failed. This way iterator will continue to next record in current recursion level not trying to dive into inaccessible directory structure.

meshcollider · 2020-02-09T09:23:30Z

Nice catch and fix, thanks! Great first time contribution. Looks good to me, but it would be great to have a test for this too if possible :)

hebasto · 2020-02-09T15:30:39Z

Concept ACK.

promag

Concept ACK. Maybe the same check should be done in VerifyWallets?

src/wallet/walletutil.cpp

uhliksk · 2020-02-10T11:51:26Z

Nice catch and fix, thanks! Great first time contribution. Looks good to me, but it would be great to have a test for this too if possible :)

I'll try to create test for all functions where error_code is necessary. Thank you for advice.

uhliksk · 2020-02-11T01:37:38Z

I added test to check all boost filesystem functions and added error code handlers where necessary.

uhliksk · 2020-02-11T01:58:05Z

I squashed all relative commits together to make it much cleaner now.

DrahtBot · 2020-02-11T06:00:17Z

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.

uhliksk · 2020-02-11T17:05:14Z

There is an issue with wallet backup after changes in error handling. I'll check today.

uhliksk · 2020-02-11T19:32:40Z

Ok, there was an unnecessary error handler. Routine is just checking if provided path is file or directory. It should not fail regardless of result. Fixed.

hebasto · 2020-02-13T10:07:29Z

@uhliksk
I guess with the words "... it would be great to have a test for this too if possible" @meshcollider had in mind a new functional test, not a linter. The latter seems do more harm than good.

I think no need to force replacement existing boost filesystem library function calls that throw an exception with ones that set an error_code.

uhliksk · 2020-02-14T19:23:18Z

I think no need to force replacement existing boost filesystem library function calls that throw an exception with ones that set an error_code.

Ok, I can revert all changes back to ListWalletDir() fix only but I still think it's better to handle all function calls properly instead of random wallet crashes. With "try...catch" method you never know what actually caused exception - boost function or your own code. It's harder to debug those "try...catch" blocks because all exceptions are catched. With error_code handled you know if boost function failed you have error code returned and if your own code failed you have an exception.

uhliksk · 2020-02-14T20:02:16Z

@hebasto I'm little bit busy today but I can try to add functional test tomorrow. Thank you for suggestion.

promag

Maybe we should wrap these boost::filesystem?

Also, maybe extract that refactor to a different PR and revert this one to just fix the original issue?

uhliksk · 2020-02-21T13:47:37Z

Yes, I was also thinking about that today. I'll revert back this PR to original issue and create another one for other changes.

promag · 2020-03-02T00:39:56Z

ACK 6307dfa.

laanwj · 2020-03-27T16:23:02Z

Concept ACK but

It's fixed by adding "ec" parameter into fs::exists() function to handle any error correctly.

It's not actually checking the ec output (calls file_size right after, overwriting it). I don't think this is correct. The normal boost::fs::exists will throw an exception in case of an error, the ec one will not raise an exception but return the error in the parameter. This means that it must not be ignored.

uhliksk · 2020-03-28T01:19:32Z

Concept ACK but

It's fixed by adding "ec" parameter into fs::exists() function to handle any error correctly.

It's not actually checking the ec output (calls file_size right after, overwriting it). I don't think this is correct. The normal boost::fs::exists will throw an exception in case of an error, the ec one will not raise an exception but return the error in the parameter. This means that it must not be ignored.

It's not important to check the ec output because boost::fs::exists return true if the given path or filestatus corresponds to an existing file or directory. If it fail for any reason it will return false. In our use case it's not important to know what actually caused an error.

hebasto

Approach ACK 24625c8, tested on Linux Mint 20 (x86_64).
Should be squashed into a single commit before merging.

I can reproduce the initial error with denied access to a directory:

$ ls -l ~/bitcoin-data-dir/wallets/
total 1448
drwx------ 3 hebasto hebasto    4096 Oct  3 11:16 20191109-main
drwx------ 2 root    root       4096 Oct  4 18:25 crash
-rw-r--r-- 1 hebasto hebasto       0 Mar  5  2020 db.log
-rw-r----- 1 hebasto hebasto 1474560 Sep 12 16:08 wallet.dat
$ src/bitcoin-cli listwalletdir
error code: -1
error message:
boost::filesystem::status: Permission denied: "/home/hebasto/.bitcoin/wallets/crash/wallet.dat"

While fixing it with the following patch:

--- a/src/wallet/walletutil.cpp
+++ b/src/wallet/walletutil.cpp
@@ -31,11 +31,12 @@ fs::path GetWalletDir()
 
 bool IsBerkeleyBtree(const fs::path& path)
 {
-    if (!fs::exists(path)) return false;
-
     // A Berkeley DB Btree file has at least 4K.
     // This check also prevents opening lock files.
     boost::system::error_code ec;
+
+    if (!fs::exists(path, ec)) return false;
+
     auto size = fs::file_size(path, ec);
     if (ec) LogPrintf("%s: %s %s\n", __func__, ec.message(), path.string());
     if (size < 4096) return false;

I encounter the infinite loop that is also fixed in this PR.

With this PR I can see in the log:

2020-10-04T16:08:35Z [httpworker.0] ListWalletDir: increment: Permission denied /home/hebasto/.bitcoin/wallets/crash

and src/bitcoin-cli listwalletdir returns correct output.

With "try...catch" method you never know what actually caused exception - boost function or your own code.

This is not true. My own code does not throw exceptions of filesystem_error type :)

src/wallet/walletutil.cpp

hebasto · 2020-10-06T07:37:57Z

src/wallet/walletutil.cpp

+        return paths;
+    }
+
+    for (it; it != fs::recursive_directory_iterator(); it.increment(ec)) {


Suggested change

for (it; it != fs::recursive_directory_iterator(); it.increment(ec)) {

for (; it != fs::recursive_directory_iterator(); it.increment(ec)) {

hebasto · 2020-10-06T07:41:14Z

src/wallet/walletutil.cpp

+    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;
+    }


Wondering what conditions could cause this error. I've read the relevant Boost docs, and tried different ill filesystem setups but without success.

meshcollider

This looks good to me now, utACK ece61bb

hebasto

Approach ACK ece61bb, the only suggestion I remind about, as currently clang emits warning:

wallet/walletutil.cpp:71:10: warning: expression result unused [-Wunused-value]
    for (it; it != fs::recursive_directory_iterator(); it.increment(ec)) {
         ^~
1 warning generated.

uhliksk · 2020-10-11T22:11:28Z

Approach ACK ece61bb, the only suggestion I remind about, as currently clang emits warning:
wallet/walletutil.cpp:71:10: warning: expression result unused [-Wunused-value]
    for (it; it != fs::recursive_directory_iterator(); it.increment(ec)) {
         ^~
1 warning generated.

Yes, sorry, my mistake. I forgot to clean it up.

hebasto

ACK 2f06e9d

uhliksk · 2020-10-15T14:54:43Z

rebase

Rebased.

hebasto · 2020-10-16T16:10:57Z

rebase

Rebased.

I don't think the latest rebasing is correct, wrt the ac38a87 commit.

uhliksk · 2020-10-17T20:25:35Z

rebase

Rebased.

I don't think the latest rebasing is correct, wrt the ac38a87 commit.

What exactly do you mean? I checked walletutil.cpp with current master branch and there is nothing different.

hebasto · 2020-10-17T20:29:16Z

rebase

Rebased.

I don't think the latest rebasing is correct, wrt the ac38a87 commit.

What exactly do you mean? I checked walletutil.cpp with current master branch and there is nothing different.

I mean this initial patch:

--- a/src/wallet/walletutil.cpp
+++ b/src/wallet/walletutil.cpp
@@ -31,11 +31,12 @@ fs::path GetWalletDir()
 
 bool IsBerkeleyBtree(const fs::path& path)
 {
-    if (!fs::exists(path)) return false;
-
     // A Berkeley DB Btree file has at least 4K.
     // This check also prevents opening lock files.
     boost::system::error_code ec;
+
+    if (!fs::exists(path, ec)) return false;
+
     auto size = fs::file_size(path, ec);
     if (ec) LogPrintf("%s: %s %s\n", __func__, ec.message(), path.string());
     if (size < 4096) return false;

Are such changes not required now?

uhliksk · 2020-10-17T20:43:52Z

rebase

Rebased.

I don't think the latest rebasing is correct, wrt the ac38a87 commit.

What exactly do you mean? I checked walletutil.cpp with current master branch and there is nothing different.

I mean this initial patch:

--- a/src/wallet/walletutil.cpp
+++ b/src/wallet/walletutil.cpp
@@ -31,11 +31,12 @@ fs::path GetWalletDir()
 
 bool IsBerkeleyBtree(const fs::path& path)
 {
-    if (!fs::exists(path)) return false;
-
     // A Berkeley DB Btree file has at least 4K.
     // This check also prevents opening lock files.
     boost::system::error_code ec;
+
+    if (!fs::exists(path, ec)) return false;
+
     auto size = fs::file_size(path, ec);
     if (ec) LogPrintf("%s: %s %s\n", __func__, ec.message(), path.string());
     if (size < 4096) return false;

Are such changes not required now?

Yes, thank you. I missed that one as IsBerkeleyBtree function has been removed from walletutil.cpp and rebase just show changes in this one file for me. I didn't noticed it was moved to bdb.cpp as IsBDBFile.

uhliksk · 2020-10-17T21:08:54Z

Are such changes not required now?

Bug has been introduced even into IsSQLiteFile. I fixed both right now. Thank you.

uhliksk · 2020-10-17T21:29:27Z

I should get back on #18189 where I have lint test to prevent this kind of bugs in future.

hebasto · 2020-10-20T22:07:42Z

@uhliksk

I suggest to combine your approach to prevent an infinity loop into #19502 (see #19502 (comment)).

@luke-jr's approach to handle file system errors/exceptions seems more preferable to me (with two types of wallet databases now).

fanquake · 2020-11-12T08:04:16Z

Closing now that #19502 has been merged.

uhliksk requested a review from promag February 7, 2020 21:12

DrahtBot added the Wallet label Feb 7, 2020

promag reviewed Feb 10, 2020

View reviewed changes

src/wallet/walletutil.cpp Outdated Show resolved Hide resolved

laanwj added the Bug label Feb 10, 2020

uhliksk force-pushed the master branch from 10a8439 to e9f90e4 Compare February 11, 2020 01:56

uhliksk force-pushed the master branch 2 times, most recently from e0eebb2 to 87a4de7 Compare February 11, 2020 03:05

uhliksk requested a review from promag February 11, 2020 03:07

This was referenced Feb 11, 2020

rpc: add extensive file checks for dumptxoutset and dumpwallet #17623

Closed

Testschains: Many regtests with different genesis and default datadir #17037

Closed

gui: Bilingual GUI error messages #16224

Merged

uhliksk force-pushed the master branch from b9701d7 to 8633d48 Compare February 12, 2020 08:12

promag reviewed Feb 21, 2020

View reviewed changes

uhliksk force-pushed the master branch from c46254a to 6307dfa Compare February 21, 2020 14:21

uhliksk mentioned this pull request Feb 21, 2020

Add error handling to all boost filesystem functions #18189

Closed

DrahtBot mentioned this pull request Sep 19, 2020

wallet: bugfix; if datadir has a trailing '/' listwalletdir would strip lead char of walletname #19933

Closed

hebasto approved these changes Oct 4, 2020

View reviewed changes

src/wallet/walletutil.cpp Outdated Show resolved Hide resolved

uhliksk force-pushed the master branch 2 times, most recently from 0f70f05 to ece61bb Compare October 4, 2020 22:52

hebasto requested changes Oct 6, 2020

View reviewed changes

meshcollider reviewed Oct 7, 2020

View reviewed changes

hebasto requested changes Oct 11, 2020

View reviewed changes

uhliksk force-pushed the master branch from ece61bb to 2f06e9d Compare October 11, 2020 22:16

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

Saibato mentioned this pull request Oct 13, 2020

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

Merged

DrahtBot added the Needs rebase label Oct 15, 2020

uhliksk force-pushed the master branch from 2f06e9d to f6f4a60 Compare October 15, 2020 14:53

DrahtBot removed the Needs rebase label Oct 15, 2020

Fix crashes and infinite loop in ListWalletDir()

6904a30

uhliksk force-pushed the master branch from f6f4a60 to 6904a30 Compare October 17, 2020 21:27

This was referenced Oct 29, 2020

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

Closed

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

Merged

fanquake closed this Nov 12, 2020

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

	for (it; it != fs::recursive_directory_iterator(); it.increment(ec)) {
	for (; it != fs::recursive_directory_iterator(); it.increment(ec)) {

Fix crashes and infinite loop in ListWalletDir() #18095

Fix crashes and infinite loop in ListWalletDir() #18095

Uh oh!

Conversation

uhliksk commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Feb 9, 2020

Uh oh!

hebasto commented Feb 9, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

uhliksk commented Feb 10, 2020

Uh oh!

uhliksk commented Feb 11, 2020

Uh oh!

uhliksk commented Feb 11, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

uhliksk commented Feb 11, 2020

Uh oh!

uhliksk commented Feb 11, 2020

Uh oh!

hebasto commented Feb 13, 2020

Uh oh!

uhliksk commented Feb 14, 2020

Uh oh!

uhliksk commented Feb 14, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

uhliksk commented Feb 21, 2020

Uh oh!

promag commented Mar 2, 2020

Uh oh!

laanwj commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uhliksk commented Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 6, 2020

Choose a reason for hiding this comment

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!

uhliksk commented Oct 11, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

uhliksk commented Oct 15, 2020

Uh oh!

hebasto commented Oct 16, 2020

Uh oh!

uhliksk commented Oct 17, 2020

Uh oh!

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

Uh oh!

Uh oh!

uhliksk commented Oct 17, 2020

Uh oh!

uhliksk commented Feb 7, 2020 •

edited

Loading

DrahtBot commented Feb 11, 2020 •

edited

Loading

laanwj commented Mar 27, 2020 •

edited

Loading

uhliksk commented Mar 28, 2020 •

edited

Loading

hebasto commented Oct 17, 2020 •

edited

Loading