wallet: Do not iterate a directory if having an error while accessing it #21907

hebasto · 2021-05-10T18:56:32Z

On Windows when ListDatabases tries to iterate any system folder, e.g., "System Volume Information", it falls into an infinite loop.

This PR fixes this bug. Now the debug.log contains:

2021-05-12T09:07:53Z ListDatabases: Access is denied D:/System Volume Information -- skipping.

An easy way to reproduce the bug and test this PR is to pass the -walletdir=D:\ command-line option, and run the listwalletdir RPC, or File -> Open Wallet in the GUI menu.

Fixes #20081.
Fixes #21136.
Fixes #21904.

Also https://bitcoin.stackexchange.com/questions/99243/listwalletdir-access-is-denied-d-system-volume-information

laanwj · 2021-05-11T11:20:59Z

src/wallet/db.cpp

@@ -19,6 +23,12 @@ std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
    for (auto it = fs::recursive_directory_iterator(wallet_dir, ec); it != fs::recursive_directory_iterator(); it.increment(ec)) {
        if (ec) {
            LogPrintf("%s: %s %s\n", __func__, ec.message(), it->path().string());
+#ifdef WIN32
+            if (ec.value() == boost::system::windows_error::access_denied) {


Is this specific to windows? Or could this happen with directory "access denied" errors on other OSes as well?

Is this specific to windows?

The error code is specific to windows.

Or could this happen with directory "access denied" errors on other OSes as well?

Yes, it could. But on other OSes the reason of "access denied" errors is a bad setup that a user can/should fix. On windows this situation seems common and not always fixable by a user.

Maybe it should avoid recursion regardless of the error and platform? Is there any case where this approach is not desirable?

Maybe it should avoid recursion regardless of the error and platform? Is there any case where this approach is not desirable?

To be sure I understand you correctly, you are suggesting do not iterate a directory if we have any error while accessing it, right?

Yes, that was my suggestion. Is there any downside of that approach?

@laanwj @promag

Thanks! Updated.

sipsorcery · 2021-05-11T12:24:13Z

Started with:

bitcoin-qt.exe -walletdir=c:\

Is this error message expected? The app continues as normal once OK is clicked.

On master the message is a warning:

hebasto · 2021-05-11T16:09:46Z

@sipsorcery

Started with:

bitcoin-qt.exe -walletdir=c:\

Is this error message expected? The app continues as normal once OK is clicked.

I cannot reproduce neither error message with this PR nor warning message on master. Could you share other relevant non-private options from the bitcoin.conf, or the debug.log from the beginning (where options are logged)?

In any case, -walletdir=c:\ on windows is a bad practice :)

sipsorcery · 2021-05-11T16:25:03Z

I cannot reproduce neither error message with this PR nor warning message on master. Could you share other relevant non-private options from the bitcoin.conf, or the debug.log from the beginning (where options are logged)?

I checked my bitcoin.conf at the time and made sure it was empty. I've attempted to repeat the steps with a fresh configuration and have managed to delete my whole bitcoin directory. Using the same command line I now don't get the error or warning prompt with either bitcoin-qt version. I'll play around a bit more to try and find what generates the previous messages.

In any case, -walletdir=c:\ on windows is a bad practice :)

Sure I was only attempting to test the fix in this PR by getting the wallet logic to scan the root drive. I don't have a D:\ drive.

When using bitcoin-qt from master and bitcoin-qt.exe -regtest -walletdir=c:\ I don't get an error about attempting to access c:\System Volume Information. I'll retry with the symbolic link mentioned in #20081 although to be honest symbolic linking a root drive from within the Bitcoin app data directory seems like asking for trouble.

hebasto · 2021-05-11T16:49:15Z

In any case, -walletdir=c:\ on windows is a bad practice :)

Sure I was only attempting to test the fix in this PR by getting the wallet logic to scan the root drive. I don't have a D:\ drive.

With this PR it is possible to scan C:\ without hanging the client, but it takes really long time :)

hebasto · 2021-05-11T16:50:56Z

I don't have a D:\ drive.

A USB flash-drive could work, no?

ghost

bitcoin-qt.exe -walletdir=E:\ Not Responding when I try to open wallets

bitcoind.exe -walletdir=E:\

Not sure why its using forward slash which is used in Linux

2021-05-12T08:05:49Z Using wallet directory E:/
2021-05-12T08:05:49Z init message: Verifying wallet(s)...
2021-05-12T08:05:49Z Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
2021-05-12T08:05:49Z Using wallet E:/wallet.dat

bitcoin-cli.exe listwalletdir returns nothing and cmd prompt running bitcoind starts printing error continuously until I close it.

Will compile the branch for this PR in some time and two more things that I noticed:

"E:" works when used with datadir but gives error for walletdir
https://bitcoin.stackexchange.com/questions/103295/unable-to-use-blocks-and-chainstate-from-external-drive

promag

Concept ACK.

promag · 2021-05-12T08:30:58Z

src/wallet/db.cpp

@@ -19,6 +23,12 @@ std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
    for (auto it = fs::recursive_directory_iterator(wallet_dir, ec); it != fs::recursive_directory_iterator(); it.increment(ec)) {
        if (ec) {
            LogPrintf("%s: %s %s\n", __func__, ec.message(), it->path().string());
+#ifdef WIN32
+            if (ec.value() == boost::system::windows_error::access_denied) {


Maybe it should avoid recursion regardless of the error and platform? Is there any case where this approach is not desirable?

This change prevents infinite looping for, for example, system folders on Windows.

hebasto · 2021-05-12T09:16:54Z

Updated bd13ec6 -> 29c9e2c (pr21907.01 -> pr21907.02, diff):

addressed @laanwj's comment and @promag's comment

UPDATE: After moving to std::filesystem we can drop this change in favor of skip_permission_denied.

ghost · 2021-05-12T14:10:20Z

Compiled branch for this PR

bitcoin-qt.exe -walletdir=E:\ I am able to see some names in File->Open Wallet
bitcoind.exe -walletdir=E:\ and bitcoin-cli.exe listwalletdir works fine

2021-05-12T13:35:53Z ListDatabases: Access is denied E:/System Volume Information -- skipping.

I see few names returned by listwalletdir

ACK 29c9e2c

Found some issues with weird wallet names and path but its not related to this PR IMO: #21510 (comment)

promag

Code review ACK 29c9e2c.

meshcollider

Code review ACK 29c9e2c

…error while accessing it 29c9e2c wallet: Do not iterate a directory if having an error while accessing it (Hennadii Stepanov) Pull request description: On Windows when `ListDatabases` tries to iterate any system folder, e.g., "System Volume Information", it falls into an infinite loop. This PR fixes this bug. Now the `debug.log` contains: ``` 2021-05-12T09:07:53Z ListDatabases: Access is denied D:/System Volume Information -- skipping. ``` An easy way to reproduce the bug and test this PR is to pass the `-walletdir=D:\` command-line option, and run the `listwalletdir` RPC, or File -> Open Wallet in the GUI menu. Fixes bitcoin#20081. Fixes bitcoin#21136. Fixes bitcoin#21904. Also https://bitcoin.stackexchange.com/questions/99243/listwalletdir-access-is-denied-d-system-volume-information ACKs for top commit: prayank23: ACK bitcoin@29c9e2c promag: Code review ACK 29c9e2c. meshcollider: Code review ACK 29c9e2c Tree-SHA512: b851c88e6d09626f4cb81acc2fa59a563b2aee64582963285715bf785c64b872e8bf738aa6b27bdbaf4c3e5c8565c2dc2c802135f9aa1f48b4b913435bc5d793

…oot directory d44a261 Fix issues when `walletdir` is root directory (unknown) Pull request description: + Remove one character less from wallet path + After testing lot of random strings with special chars in `wallet_name`, I found that the issue was not related to special characters in the name. Reviewing PR bitcoin/bitcoin#21907 helped me resolve the issue. **Real issue**: If the path mentioned in `walletdir` is a root directory, first character of the wallet name or path is removed **Solution**: `if` statement to check `walletdir` is a root directory Fixes: bitcoin/bitcoin#21510 bitcoin/bitcoin#21501 Related PR: bitcoin/bitcoin#20080 Consider the wallet directories `w1` and `w2` saved in `D:\`. Run `bitcoind.exe -walletdir=D:\`, Results for `bitcoin-cli.exe listwalletdir`: Before this PR: ``` { "wallets": [ { "name": "1" }, { "name": "2" } ] } ``` After this PR: ``` "wallets": [ { "name": "w1" }, { "name": "w2" } ] } ``` ACKs for top commit: ryanofsky: Code review ACK d44a261 meshcollider: utACK d44a261 Tree-SHA512: b09b00f727407e3771c8694861dae1bfd29d97a0d51ddcb5d9c0111dc618b3fff2f75829cbb4361c54457ee564e94fcefd9e2928262a1c918a2b6bbad724eb55

…ctory d44a261 Fix issues when `walletdir` is root directory (unknown) Pull request description: + Remove one character less from wallet path + After testing lot of random strings with special chars in `wallet_name`, I found that the issue was not related to special characters in the name. Reviewing PR bitcoin#21907 helped me resolve the issue. **Real issue**: If the path mentioned in `walletdir` is a root directory, first character of the wallet name or path is removed **Solution**: `if` statement to check `walletdir` is a root directory Fixes: bitcoin#21510 bitcoin#21501 Related PR: bitcoin#20080 Consider the wallet directories `w1` and `w2` saved in `D:\`. Run `bitcoind.exe -walletdir=D:\`, Results for `bitcoin-cli.exe listwalletdir`: Before this PR: ``` { "wallets": [ { "name": "1" }, { "name": "2" } ] } ``` After this PR: ``` "wallets": [ { "name": "w1" }, { "name": "w2" } ] } ``` ACKs for top commit: ryanofsky: Code review ACK d44a261 meshcollider: utACK d44a261 Tree-SHA512: b09b00f727407e3771c8694861dae1bfd29d97a0d51ddcb5d9c0111dc618b3fff2f75829cbb4361c54457ee564e94fcefd9e2928262a1c918a2b6bbad724eb55

This change prevents infinite looping for, for example, system folders on Windows. Github-Pull: bitcoin#21907 Rebased-From: 29c9e2c

fanquake · 2021-07-01T01:36:50Z

Backported to 0.21 in #22255.

…error while accessing it 7b0b201 wallet: Do not iterate a directory if having an error while accessing it (Hennadii Stepanov) Pull request description: This change prevents infinite looping for, for example, system folders on Windows. Github-Pull: #21907 Rebased-From: 29c9e2c Note: Trivial backport, but in a differently-named function in another file ACKs for top commit: hebasto: ACK 7b0b201, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: d41ab267250e8bfd9289cacf1fd804cc1a3bb20fc479dc9da5a69ebf26530b552b11b2ee6b11e17a1c146ca792ee65bd64eeb2269fa5e73a70361da8a2a09925

…error while accessing it 29c9e2c wallet: Do not iterate a directory if having an error while accessing it (Hennadii Stepanov) Pull request description: On Windows when `ListDatabases` tries to iterate any system folder, e.g., "System Volume Information", it falls into an infinite loop. This PR fixes this bug. Now the `debug.log` contains: ``` 2021-05-12T09:07:53Z ListDatabases: Access is denied D:/System Volume Information -- skipping. ``` An easy way to reproduce the bug and test this PR is to pass the `-walletdir=D:\` command-line option, and run the `listwalletdir` RPC, or File -> Open Wallet in the GUI menu. Fixes bitcoin#20081. Fixes bitcoin#21136. Fixes bitcoin#21904. Also https://bitcoin.stackexchange.com/questions/99243/listwalletdir-access-is-denied-d-system-volume-information ACKs for top commit: prayank23: ACK bitcoin@29c9e2c promag: Code review ACK 29c9e2c. meshcollider: Code review ACK 29c9e2c Tree-SHA512: b851c88e6d09626f4cb81acc2fa59a563b2aee64582963285715bf785c64b872e8bf738aa6b27bdbaf4c3e5c8565c2dc2c802135f9aa1f48b4b913435bc5d793

…ctory d44a261 Fix issues when `walletdir` is root directory (unknown) Pull request description: + Remove one character less from wallet path + After testing lot of random strings with special chars in `wallet_name`, I found that the issue was not related to special characters in the name. Reviewing PR bitcoin#21907 helped me resolve the issue. **Real issue**: If the path mentioned in `walletdir` is a root directory, first character of the wallet name or path is removed **Solution**: `if` statement to check `walletdir` is a root directory Fixes: bitcoin#21510 bitcoin#21501 Related PR: bitcoin#20080 Consider the wallet directories `w1` and `w2` saved in `D:\`. Run `bitcoind.exe -walletdir=D:\`, Results for `bitcoin-cli.exe listwalletdir`: Before this PR: ``` { "wallets": [ { "name": "1" }, { "name": "2" } ] } ``` After this PR: ``` "wallets": [ { "name": "w1" }, { "name": "w2" } ] } ``` ACKs for top commit: ryanofsky: Code review ACK d44a261 meshcollider: utACK d44a261 Tree-SHA512: b09b00f727407e3771c8694861dae1bfd29d97a0d51ddcb5d9c0111dc618b3fff2f75829cbb4361c54457ee564e94fcefd9e2928262a1c918a2b6bbad724eb55

hebasto added Bug Wallet Windows labels May 10, 2021

hebasto force-pushed the 210510-win branch from adf68db to bd13ec6 Compare May 10, 2021 19:04

This was referenced May 10, 2021

Crash when opening wallet #21904

Closed

UI Hangs when running as std user #21136

Closed

wallet: Can't Access System Volume Information #20081

Closed

laanwj reviewed May 11, 2021

View reviewed changes

ghost reviewed May 12, 2021

View reviewed changes

promag reviewed May 12, 2021

View reviewed changes

wallet: Do not iterate a directory if having an error while accessing it

29c9e2c

This change prevents infinite looping for, for example, system folders on Windows.

hebasto force-pushed the 210510-win branch from bd13ec6 to 29c9e2c Compare May 12, 2021 09:12

hebasto changed the title ~~wallet: Do not iterate system folders on Windows~~ wallet: Do not iterate a directory if having an error while accessing it May 12, 2021

hebasto removed the Windows label May 12, 2021

ghost mentioned this pull request May 12, 2021

Special characters in wallet_name #21510

Closed

promag reviewed May 12, 2021

View reviewed changes

meshcollider reviewed May 13, 2021

View reviewed changes

meshcollider merged commit a31a1ce into bitcoin:master May 13, 2021

hebasto deleted the 210510-win branch May 13, 2021 09:26

ghost mentioned this pull request May 13, 2021

wallet: Fix issues when walletdir is root directory #21944

Merged

luke-jr mentioned this pull request Jun 15, 2021

[0.21] wallet: Do not iterate a directory if having an error while accessing it #22255

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge a31a1ce into merged_master (Bitcoin PR bitcoin/bitcoin#21907)

d9d4910

bitcoin locked and limited conversation to collaborators Jun 17, 2023

wallet: Do not iterate a directory if having an error while accessing it #21907

wallet: Do not iterate a directory if having an error while accessing it #21907

Uh oh!

Conversation

hebasto commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj May 11, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto May 11, 2021

Choose a reason for hiding this comment

Uh oh!

promag May 12, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto May 12, 2021

Choose a reason for hiding this comment

Uh oh!

promag May 12, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto May 12, 2021

Choose a reason for hiding this comment

Uh oh!

sipsorcery commented May 11, 2021

Uh oh!

hebasto commented May 11, 2021

Uh oh!

sipsorcery commented May 11, 2021

Uh oh!

hebasto commented May 11, 2021

Uh oh!

hebasto commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost left a comment • edited by ghost 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 May 12, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 12, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Jul 1, 2021

Uh oh!

Uh oh!

hebasto commented May 10, 2021 •

edited

Loading

hebasto commented May 11, 2021 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

hebasto commented May 12, 2021 •

edited

Loading

ghost commented May 12, 2021 •

edited by ghost

Loading