Fix crashes and infinite loop in ListWalletDir()

uhliksk commented at 8:52 pm on February 7, 2020: none

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.

uhliksk requested review from promag on Feb 7, 2020

DrahtBot added the label Wallet on Feb 7, 2020

meshcollider commented at 9:23 am on February 9, 2020: contributor

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 commented at 3:30 pm on February 9, 2020: member

Concept ACK.

in src/wallet/walletutil.cpp:64 in b89cb2b599 outdated

59@@ -59,11 +60,16 @@ std::vector<fs::path> ListWalletDir()
60     const fs::path wallet_dir = GetWalletDir();
61     const size_t offset = wallet_dir.string().size() + 1;
62     std::vector<fs::path> paths;
63-    boost::system::error_code ec;
64+    boost::system::error_code ec, eci;

promag commented at 11:58 pm on February 9, 2020:

One error_code is enough?

uhliksk commented at 11:49 am on February 10, 2020:

Nice catch. I can add commits to this PR fixing all other instances with missing error_code. (exists(), is_directory(), etc.)

For iterator() and increment() you have to use separate error_code because you have to handle them differently. Using single one it’s hard to differentiate which one failed.

uhliksk commented at 8:43 pm on September 22, 2020:

Made PR #18189.

promag commented at 0:01 am on February 10, 2020: member

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

uhliksk commented at 11:51 am on February 10, 2020: none

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.

laanwj added the label Bug on Feb 10, 2020

uhliksk commented at 1:37 am on February 11, 2020: none

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

uhliksk force-pushed on Feb 11, 2020

uhliksk commented at 1:58 am on February 11, 2020: none

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

uhliksk force-pushed on Feb 11, 2020

uhliksk requested review from promag on Feb 11, 2020

DrahtBot commented at 6:00 am on February 11, 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.

uhliksk commented at 5:05 pm on February 11, 2020: none

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

uhliksk commented at 7:32 pm on February 11, 2020: none

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.

uhliksk force-pushed on Feb 12, 2020

hebasto commented at 10:07 am on February 13, 2020: member

@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 commented at 7:23 pm on February 14, 2020: none

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 commented at 8:02 pm on February 14, 2020: none

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

promag commented at 7:52 am on February 21, 2020: member

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 commented at 1:47 pm on February 21, 2020: none

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

uhliksk force-pushed on Feb 21, 2020

promag commented at 0:39 am on March 2, 2020: member

ACK 6307dfa87e31aa30e884489d36c67688527bf8ca.

laanwj commented at 4:23 pm on March 27, 2020: member

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 commented at 1:19 am on March 28, 2020: none

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 commented at 3:21 pm on April 2, 2020: member

@uhliksk on Linux Mint 19.3, master branch (b83565625e32b22395e28c1965b2e42fc17f04d7), cannot reproduce the following 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 works just fine for me.

achow101 commented at 7:41 pm on June 11, 2020: member

@uhliksk on Linux Mint 19.3, master branch (b835656), cannot reproduce the following crash:

I had to do sudo chmod 700 ~/.bitcoin/wallets/crash to reproduce.

I was not able to reproduce the infinite loop issue.

One problem I noticed was that with the test case wallet in my walletdir, the listing did not contain all of the wallets present. Several wallets that were previously listed were no longer there.

in src/wallet/walletutil.cpp:66 in 6307dfa87e outdated

60@@ -60,10 +61,16 @@ std::vector<fs::path> ListWalletDir()
61     const size_t offset = wallet_dir.string().size() + 1;
62     std::vector<fs::path> paths;
63     boost::system::error_code ec;
64+    boost::system::error_code eci;
65 
66-    for (auto it = fs::recursive_directory_iterator(wallet_dir, ec); it != fs::recursive_directory_iterator(); it.increment(ec)) {
67+    for (auto it = fs::recursive_directory_iterator(wallet_dir, ec); it != fs::recursive_directory_iterator(); it.increment(eci)) {

luke-jr commented at 6:34 pm on August 14, 2020:

I don’t see why we can’t reuse ec? The initialisation and increment should never run in the same iteration.

luke-jr commented at 7:00 pm on August 14, 2020:

Oh, it’s so the error message can be different… nm

in src/wallet/walletutil.cpp:71 in 6307dfa87e outdated

68         if (ec) {
69-            LogPrintf("%s: %s %s\n", __func__, ec.message(), it->path().string());
70+            LogPrintf("%s: iterator: %s %s\n", __func__, ec.message(), it->path().string());
71+            continue;
72+        }
73+        if (eci) {

luke-jr commented at 6:34 pm on August 14, 2020:

In fact, eci wouldn’t be initialised here the first iteration…

luke-jr commented at 7:01 pm on August 14, 2020:

Initialising eci above should be done

uhliksk commented at 9:28 pm on August 14, 2020:

You’re right, I’ll add eci initialisation. Thank you.

luke-jr commented at 6:31 am on August 15, 2020:

No, I’m wrong. Boost takes care of this, and won’t let us explicitly initialise…

in src/wallet/walletutil.cpp:73 in 6307dfa87e outdated

70+            LogPrintf("%s: iterator: %s %s\n", __func__, ec.message(), it->path().string());
71+            continue;
72+        }
73+        if (eci) {
74+            LogPrintf("%s: increment: %s %s\n", __func__, eci.message(), it->path().string());
75+            it.disable_recursion_pending();

luke-jr commented at 3:50 am on August 18, 2020:

disable_recursion_pending is a new name introduced in Boost 1.72. We support much older. Renaming this to no_push (the old name) should just work.

in src/wallet/walletutil.cpp:70 in 6307dfa87e outdated

67+    for (auto it = fs::recursive_directory_iterator(wallet_dir, ec); it != fs::recursive_directory_iterator(); it.increment(eci)) {
68         if (ec) {
69-            LogPrintf("%s: %s %s\n", __func__, ec.message(), it->path().string());
70+            LogPrintf("%s: iterator: %s %s\n", __func__, ec.message(), it->path().string());
71+            continue;
72+        }

hebasto commented at 4:52 pm on October 4, 2020:

6307dfa87e31aa30e884489d36c67688527bf8ca

If recursive_directory_iterator ctor sets an error code we should not iterate it. Mind replacing continue with break, or, even better, initializing it before the loop and early return in case of error?

uhliksk commented at 10:39 pm on October 4, 2020:

Yes, you are right. If it failed initializing it then there is nothing to iterate. Fixed.

uhliksk commented at 10:54 pm on October 4, 2020:

We can also use ec again as I moved initialization of it before the loop.

hebasto approved

hebasto commented at 4:55 pm on October 4, 2020: member

Approach ACK 24625c881a4cd9214f68710b03631634d3d64a93, 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:

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

While fixing it with the following patch:

 0--- a/src/wallet/walletutil.cpp
 1+++ b/src/wallet/walletutil.cpp
 2@@ -31,11 +31,12 @@ fs::path GetWalletDir()
 3 
 4 bool IsBerkeleyBtree(const fs::path& path)
 5 {
 6-    if (!fs::exists(path)) return false;
 7-
 8     // A Berkeley DB Btree file has at least 4K.
 9     // This check also prevents opening lock files.
10     boost::system::error_code ec;
11+
12+    if (!fs::exists(path, ec)) return false;
13+
14     auto size = fs::file_size(path, ec);
15     if (ec) LogPrintf("%s: %s %s\n", __func__, ec.message(), path.string());
16     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:

02020-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 :)

uhliksk force-pushed on Oct 4, 2020

in src/wallet/walletutil.cpp:71 in ece61bb0fb outdated

67+    if (ec) {
68+        LogPrintf("%s: iterator: %s %s\n", __func__, ec.message(), it->path().string());
69+        return paths;
70+    }
71+
72+    for (it; it != fs::recursive_directory_iterator(); it.increment(ec)) {

hebasto commented at 7:37 am on October 6, 2020:

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

in src/wallet/walletutil.cpp:46 in ece61bb0fb outdated

61@@ -61,9 +62,16 @@ std::vector<fs::path> ListWalletDir()
62     std::vector<fs::path> paths;
63     boost::system::error_code ec;
64 
65-    for (auto it = fs::recursive_directory_iterator(wallet_dir, ec); it != fs::recursive_directory_iterator(); it.increment(ec)) {
66+    auto it = fs::recursive_directory_iterator(wallet_dir, ec);
67+    if (ec) {
68+        LogPrintf("%s: iterator: %s %s\n", __func__, ec.message(), it->path().string());
69+        return paths;
70+    }

hebasto commented at 7:41 am on October 6, 2020:

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

hebasto changes_requested

meshcollider commented at 9:10 am on October 7, 2020: contributor

This looks good to me now, utACK ece61bb0fb795facb5ad780245e15c65a816c8b8

hebasto changes_requested

hebasto commented at 2:43 am on October 11, 2020: member

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

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

uhliksk commented at 10:11 pm on October 11, 2020: none

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

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

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

uhliksk force-pushed on Oct 11, 2020

hebasto approved

hebasto commented at 6:11 am on October 12, 2020: member

ACK 2f06e9d12bb467dcae8cb8307467d0c0c89141bc

DrahtBot added the label Needs rebase on Oct 15, 2020

uhliksk force-pushed on Oct 15, 2020

uhliksk commented at 2:54 pm on October 15, 2020: none

rebase

Rebased.

DrahtBot removed the label Needs rebase on Oct 15, 2020

hebasto commented at 4:10 pm on October 16, 2020: member

rebase

Rebased.

I don’t think the latest rebasing is correct, wrt the ac38a87225be0f1103ff9629d63980550d2f372b commit.

uhliksk commented at 8:25 pm on October 17, 2020: none

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 commented at 8:29 pm on October 17, 2020: member

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:

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

Are such changes not required now?

uhliksk commented at 8:43 pm on October 17, 2020: none

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:

 0--- a/src/wallet/walletutil.cpp
 1+++ b/src/wallet/walletutil.cpp
 2@@ -31,11 +31,12 @@ fs::path GetWalletDir()
 3 
 4 bool IsBerkeleyBtree(const fs::path& path)
 5 {
 6-    if (!fs::exists(path)) return false;
 7-
 8     // A Berkeley DB Btree file has at least 4K.
 9     // This check also prevents opening lock files.
10     boost::system::error_code ec;
11+
12+    if (!fs::exists(path, ec)) return false;
13+
14     auto size = fs::file_size(path, ec);
15     if (ec) LogPrintf("%s: %s %s\n", __func__, ec.message(), path.string());
16     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 commented at 9:08 pm on October 17, 2020: none

Are such changes not required now?

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

Fix crashes and infinite loop in ListWalletDir() 6904a30919

uhliksk force-pushed on Oct 17, 2020

uhliksk commented at 9:29 pm on October 17, 2020: none

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

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

@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 commented at 8:04 am on November 12, 2020: member

Closing now that #19502 has been merged.

fanquake closed this on Nov 12, 2020

DrahtBot locked this on Feb 15, 2022

Fix crashes and infinite loop in ListWalletDir() #18095

Conflicts