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

pull promag wants to merge 1 commits into bitcoin:master from promag:2019-03-fix-listwalletdir changing 1 files +11 −2
  1. promag commented at 11:02 PM on March 11, 2019: member

    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
  2. promag commented at 11:02 PM on March 11, 2019: member

    Fixes #15555 (comment).

  3. hebasto commented at 11:10 PM on March 11, 2019: member

    Concept ACK.

  4. fanquake added the label Wallet on Mar 11, 2019
  5. fanquake added this to the milestone 0.18.0 on Mar 11, 2019
  6. in src/wallet/walletutil.cpp:60 in 8de53d5097 outdated 
      53 | @@ -54,8 +54,9 @@ std::vector<fs::path> ListWalletDir()
  54 |      const fs::path wallet_dir = GetWalletDir();
  55 |      const size_t offset = wallet_dir.string().size() + 1;
  56 |      std::vector<fs::path> paths;
  57 | +    boost::system::error_code ec;
    ryanofsky commented at 12:02 AM on March 12, 2019:

    Might be a good place to comment that errors are ignored.

    promag commented at 12:11 AM on March 12, 2019:

    Good point, will add.

    laanwj commented at 11:45 AM on March 13, 2019:

    Please don't ignore errors, at least log them

  7. ryanofsky approved
  8. ryanofsky commented at 12:05 AM on March 12, 2019: member

    utACK 8de53d5097c7e0936a15ec738e2580814ee0de9b. 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.

  9. promag commented at 12:13 AM on March 12, 2019: member

    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.

  10. fanquake added the label Needs gitian build on Mar 12, 2019
  11. promag commented at 1:27 AM on March 12, 2019: member

    Thanks @fanquake!

  12. Sjors commented at 12:52 PM on March 12, 2019: member

    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 8de53d5097 (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?

  13. promag commented at 2:41 PM on March 12, 2019: member

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

    Done.

  14. promag force-pushed on Mar 12, 2019
  15. promag force-pushed on Mar 12, 2019
  16. laanwj commented at 11:49 AM on March 13, 2019: member

    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.

  17. promag commented at 1:55 PM on March 13, 2019: member

    @laanwj agree, I'll log.

  18. ryanofsky commented at 2:33 PM on March 13, 2019: member

    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.

  19. promag commented at 4:28 PM on March 13, 2019: member

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

  20. promag renamed this:
    wallet: Ignore recursive_directory_iterator errors in ListWalletDir
    wallet: Log errors in ListWalletDir and IsBerkeleyBtree
    on Mar 13, 2019
  21. ryanofsky approved
  22. ryanofsky commented at 5:02 PM on March 13, 2019: member

    utACK d595965c599161b14346a2552867eb2869a097c7. 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.

  23. DrahtBot commented at 7:18 PM on March 13, 2019: member

    <!--a722867cd34abeea1fadc8d60700f111-->

    Gitian builds for commit c3b1cb958f7c57c5d87db663f2b1a83e3471d354 (master):

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

  24. DrahtBot removed the label Needs gitian build on Mar 13, 2019
  25. laanwj commented at 7:37 PM on March 13, 2019: member

    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

  26. MarcoFalke added the label Needs gitian build on Mar 13, 2019
  27. wallet: Log and ignore errors in ListWalletDir and IsBerkeleyBtree 15c69b158d
  28. promag force-pushed on Mar 13, 2019
  29. promag renamed this:
    wallet: Log errors in ListWalletDir and IsBerkeleyBtree
    wallet: Log and ignore errors in ListWalletDir and IsBerkeleyBtree
    on Mar 13, 2019
  30. promag commented at 8:52 PM on March 13, 2019: member

    Reworded and squashed, thanks.

  31. laanwj merged this on Mar 14, 2019
  32. laanwj closed this on Mar 14, 2019
  33. laanwj referenced this in commit ef27a060ac on Mar 14, 2019
  34. laanwj referenced this in commit 7fa1f6258c on Mar 14, 2019
  35. DrahtBot commented at 12:02 PM on March 15, 2019: member

    <!--a722867cd34abeea1fadc8d60700f111-->

    Gitian builds for commit 8e1704c01537d1750555de23bfae00efa5864b3e (master):

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

  36. DrahtBot removed the label Needs gitian build on Mar 15, 2019
  37. HashUnlimited referenced this in commit be35d3903e on Apr 19, 2019
  38. promag deleted the branch on Apr 22, 2019
  39. deadalnix referenced this in commit b92fb32932 on Sep 4, 2020
  40. PastaPastaPasta referenced this in commit 00db885d15 on Sep 11, 2021
  41. MarcoFalke locked this on Dec 16, 2021
Contributors
promaghebastoryanofskylaanwjSjorsDrahtBot
Labels
Wallet

Milestone
0.18.0

Linked (view graph)
#15912 [macOS] [GUI] log entries when hovering on / over File->Open Wallet
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 21:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me