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.
22 | @@ -19,6 +23,12 @@ std::vector<fs::path> ListDatabases(const fs::path& wallet_dir) 23 | for (auto it = fs::recursive_directory_iterator(wallet_dir, ec); it != fs::recursive_directory_iterator(); it.increment(ec)) { 24 | if (ec) { 25 | LogPrintf("%s: %s %s\n", __func__, ec.message(), it->path().string()); 26 | +#ifdef WIN32 27 | + 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?
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:
Started with:
bitcoin-qt.exe -walletdir=c:\Is this error message expected? The app continues as normal once
OKis 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 :)
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.
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 :)
I don't have a
D:\drive.
A USB flash-drive could work, no?
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
Concept ACK.
This change prevents infinite looping for, for example, system folders
on Windows.
Updated bd13ec6d834d4c5f20a5ff9ab41e2e9e86f32d93 -> 29c9e2c2d2015ade47ed4497926363dea3f9c59b (pr21907.01 -> pr21907.02, diff):
UPDATE: After moving to std::filesystem we can drop this change in favor of skip_permission_denied.
Compiled branch for this PR
bitcoin-qt.exe -walletdir=E:\ I am able to see some names in File->Open Walletbitcoind.exe -walletdir=E:\ and bitcoin-cli.exe listwalletdir works fine2021-05-12T13:35:53Z ListDatabases: Access is denied E:/System Volume Information -- skipping.
I see few names returned by listwalletdir
ACK https://github.com/bitcoin/bitcoin/pull/21907/commits/29c9e2c2d2015ade47ed4497926363dea3f9c59b
Found some issues with weird wallet names and path but its not related to this PR IMO: #21510 (comment)
Code review ACK 29c9e2c2d2015ade47ed4497926363dea3f9c59b.
Code review ACK 29c9e2c2d2015ade47ed4497926363dea3f9c59b
