Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: Log and ignore errors in ListWalletDir and IsBerkeleyBtree #15583

Merged
merged 1 commit into from Mar 14, 2019

Conversation

Projects
None yet
8 participants
@promag
Copy link
Member

commented Mar 11, 2019

Use the noexcept members of boost::filesystem::recursive_directory_iterator in order to ignore boost::filesystem::directory_iterator::construct: Permission denied errors. The errors are logged though.

Steps to reproduce the issue:

# 1. create directory for -walletdir without read access:
mkdir /tmp/foo && chmod a-r /tmp/foo

# 2. run bitcoin-qt and should print an error, but continues running:
/Volumes/Bitcoin-Core/Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt -regtest -walletdir=/tmp/foo
/private/tmp/foo: Permission denied

# 4. go to File -> Open Wallet and should segfault:
EXCEPTION: N5boost10filesystem16filesystem_errorE
boost::filesystem::directory_iterator::construct: Permission denied: "/private/tmp/foo"
bitcoin in Runaway exception
@promag

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

@hebasto

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Concept ACK.

@fanquake fanquake added the Wallet label Mar 11, 2019

@fanquake fanquake added this to the 0.18.0 milestone Mar 11, 2019

@ryanofsky
Copy link
Contributor

left a comment

utACK 8de53d5. Nice fix. I wouldn't have expected incrementing the iterator to throw.

If there's an easy way to add a test, it could be nice to have. But if it requires adding something platform-specific, probably better to skip.

Show resolved Hide resolved src/wallet/walletutil.cpp
@promag

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

If there's an easy way to add a test, it could be nice to have.

I'd like to confirm a gitian build first, cc @MarcoFalke.

@promag

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

Thanks @fanquake!

@Sjors

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Thanks for catching this @kiv06041992. I was able to reproduce with v0.18.0rc1 binary, as well as when building master from source.

This commit fixes it, so tACK 8de53d5 (test would be nice).

I'm even able to create a new wallet and use it, and the wallet will be hidden next time you start (no idea why one would want that).

@promag can you copy the steps to reproduce to the description?

@promag

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@promag can you copy the steps to reproduce to the description?

Done.

@promag promag force-pushed the promag:2019-03-fix-listwalletdir branch 2 times, most recently from df3d9d2 to 4c61be6 Mar 12, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Although I agree that the current behavior is undesirable, I don't like ignoring errors either. In my experience this makes troubleshooting as good as impossible when something unexpected happens. Please handle the error or at least log it.

@promag

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

@laanwj agree, I'll log.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Maybe break on failure to increment, since that might result in an infinite loop.

EDIT: It looks like failing to increment shouldn't cause infinite loops since m_imp->increment(&ec) is called unconditionally in https://www.boost.org/doc/libs/1_69_0/boost/filesystem/operations.hpp and end condition is based an internal stack.

@promag

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

Now logging the error and the respective path. Also log error in IsBerkeleyBtree. Will squash after review.

@promag promag changed the title wallet: Ignore recursive_directory_iterator errors in ListWalletDir wallet: Log errors in ListWalletDir and IsBerkeleyBtree Mar 13, 2019

@ryanofsky
Copy link
Contributor

left a comment

utACK d595965. Only change since last review is adding logging. Current PR description ("Log errors in ListWalletDir and IsBerkeleyBtree") seems misleading, though. Main change is not the logging, but making listwalletdir not throw exceptions.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Gitian builds for commit c3b1cb9 (master):

Gitian builds for commit dab434c (master and this pull):

@laanwj

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Now logging the error and the respective path. Also log error in IsBerkeleyBtree. Will squash after review.

Thank you, looks good to me now, utACK after squash

@promag promag force-pushed the promag:2019-03-fix-listwalletdir branch from d595965 to 15c69b1 Mar 13, 2019

@promag promag changed the title wallet: Log errors in ListWalletDir and IsBerkeleyBtree wallet: Log and ignore errors in ListWalletDir and IsBerkeleyBtree Mar 13, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

Reworded and squashed, thanks.

@laanwj laanwj merged commit 15c69b1 into bitcoin:master Mar 14, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 14, 2019

wallet: Log and ignore errors in ListWalletDir and IsBerkeleyBtree
Github-Pull: #15583
Rebased-From: 15c69b1
Tree-SHA512: edef7cafc5a2cb8d3355591a7742ef61454a5dedbb1dc22f6cc6bae42329d887f3f4a054f2aeedf31180051f50b6478d09e204066205699dabc0cb67b0ce4a96

laanwj added a commit that referenced this pull request Mar 14, 2019

Merge #15583: wallet: Log and ignore errors in ListWalletDir and IsBe…
…rkeleyBtree

15c69b1 wallet: Log and ignore errors in ListWalletDir and IsBerkeleyBtree (João Barbosa)

Pull request description:

  Use the `noexcept` members of `boost::filesystem::recursive_directory_iterator` in order to ignore `boost::filesystem::directory_iterator::construct: Permission denied` errors. The errors are logged though.

  Steps to reproduce the issue:

  ```sh
  # 1. create directory for -walletdir without read access:
  mkdir /tmp/foo && chmod a-r /tmp/foo

  # 2. run bitcoin-qt and should print an error, but continues running:
  /Volumes/Bitcoin-Core/Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt -regtest -walletdir=/tmp/foo
  /private/tmp/foo: Permission denied

  # 4. go to File -> Open Wallet and should segfault:
  EXCEPTION: N5boost10filesystem16filesystem_errorE
  boost::filesystem::directory_iterator::construct: Permission denied: "/private/tmp/foo"
  bitcoin in Runaway exception
  ```

Tree-SHA512: 37e8bf5a1e0defc331030fd511bf9cac2765d01dfbf23e7233f37506e85b8ad07edcde9ba6dae7a2c95700c78d28c7dd248153607381852da96273cb159c4934
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Gitian builds for commit 8e1704c (master):

Gitian builds for commit f5095fb (master and this pull):

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Apr 19, 2019

wallet: Log and ignore errors in ListWalletDir and IsBerkeleyBtree
Github-Pull: bitcoin#15583
Rebased-From: 15c69b1
Tree-SHA512: edef7cafc5a2cb8d3355591a7742ef61454a5dedbb1dc22f6cc6bae42329d887f3f4a054f2aeedf31180051f50b6478d09e204066205699dabc0cb67b0ce4a96

@promag promag deleted the promag:2019-03-fix-listwalletdir branch Apr 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.