Skip to content

Wallet: Fix error handling (copy_file failure in RestoreWallet, and in general via interfaces)#26005

Merged
fanquake merged 2 commits intobitcoin:masterfrom
luke-jr:fix_wallet_copyfail_nullresult
Sep 19, 2022
Merged

Wallet: Fix error handling (copy_file failure in RestoreWallet, and in general via interfaces)#26005
fanquake merged 2 commits intobitcoin:masterfrom
luke-jr:fix_wallet_copyfail_nullresult

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Sep 4, 2022 •
edited
Loading

Bug 1: copy_file can throw exceptions, but RestoreWallet is expected to return a nullptr with a populated errors parameter. This is fixed by wrapping copy_file and LoadWallet (for good measure) in a try block, and converting any exceptions to the intended return style.

Bug 2: util::Result turns what would have been a false unique_ptr into a true nullptr result, which leads to nullptr dereferences in at least the 3 cases of wallet creation/loading/restoring. This is fixed by keeping the pointer as a plain std::unique_ptr until actually returning it (ie, after the nullptr check).

Fixes bitcoin-core/gui#661

@luke-jr luke-jr mentioned this pull request Sep 4, 2022
Open
@fanquake fanquake added the Wallet label Sep 4, 2022
w0xlt
w0xlt approved these changes Sep 4, 2022
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK c1cd7b4.

Not sure if b60a2ce ("Bugfix: Wallet: Wrap RestoreWallet content in a try block ...") is needed.
See #22541 (comment).

@luke-jr
Copy link
Member Author

luke-jr commented Sep 5, 2022 •
edited
Loading

Not sure if b60a2ce ("Bugfix: Wallet: Wrap RestoreWallet content in a try block ...") is needed.
See #22541 (comment).

Without it, the GUI crashes quite uncleanly (bitcoin-core/gui#661), and an empty directory is left behind.

@maflcko maflcko added this to the 24.0 milestone Sep 5, 2022
maflcko
maflcko reviewed Sep 5, 2022
View reviewed changes
src/wallet/interfaces.cpp Outdated
Copy link
Member

@maflcko maflcko Sep 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was likely introduced in fa475e9, and 07df6cd

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 5, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26022 (Add util::ResultPtr class 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.

@DrahtBot DrahtBot mentioned this pull request Sep 5, 2022
Draft
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 6, 2022
@ryanofsky
Use ResultPtr<unique_ptr> instead of Result<unique_ptr>
4b8ee99 
This is a partial fix for a GUI crash
bitcoin-core/gui#661 that avoids the crash but
doesn't fix the restoreWallet error reporting. bitcoin#26005 is a more complete fix.
@achow101
Copy link
Member

achow101 commented Sep 6, 2022 •
edited
Loading

The try should probably be moved up to also include the TryCreateDirectories as well? If I change the permissions on the entire wallets/ directory, it still crashes.

Or maybe TryCreateDirectories needs to be modified to handle when it actually can't create a directory. That would also help with the case where a wallet dir can't be created during wallet creation.

ryanofsky
ryanofsky approved these changes Sep 6, 2022
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK c1cd7b4ac60d00dec369b3fe8f883cad07fbe833

@furszy furszy mentioned this pull request Sep 6, 2022
Merged
@luke-jr luke-jr force-pushed the fix_wallet_copyfail_nullresult branch from c1cd7b4 to b298bee Compare September 6, 2022 19:12
@luke-jr
Copy link
Member Author

luke-jr commented Sep 6, 2022

The try should probably be moved up to also include the TryCreateDirectories as well? If I change the permissions on the entire wallets/ directory, it still crashes.

Done

furszy
furszy approved these changes Sep 6, 2022
View reviewed changes
Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK b298beed

@DrahtBot DrahtBot mentioned this pull request Sep 6, 2022
Draft
@achow101
Copy link
Member

achow101 commented Sep 15, 2022

ACK b298beed38fba06d354e44778e6ecb3526feec1d

achow101
achow101 reviewed Sep 15, 2022
View reviewed changes
@luke-jr
Bugfix: Wallet: Wrap RestoreWallet content in a try block to ensure e…
335ff98 
…xceptions become returned errors and incomplete wallet directory is removed
@luke-jr luke-jr force-pushed the fix_wallet_copyfail_nullresult branch from b298bee to 6c5af62 Compare September 16, 2022 21:07
achow101
achow101 reviewed Sep 16, 2022
View reviewed changes
@luke-jr luke-jr force-pushed the fix_wallet_copyfail_nullresult branch from 6c5af62 to c3e5365 Compare September 16, 2022 23:29
@achow101
Copy link
Member

achow101 commented Sep 17, 2022

ACK c3e5365

@fanquake fanquake merged commit 9f65006 into bitcoin:master Sep 19, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 20, 2022
@fanquake
Merge bitcoin#26005: Wallet: Fix error handling (copy_file failure in…
3bd691f 
… RestoreWallet, and in general via interfaces)

c3e5365 Bugfix: Wallet: Return util::Error rather than non-error nullptr when CreateWallet/LoadWallet/RestoreWallet fail (Luke Dashjr)
335ff98 Bugfix: Wallet: Wrap RestoreWallet content in a try block to ensure exceptions become returned errors and incomplete wallet directory is removed (Luke Dashjr)

Pull request description:

  Bug 1: `copy_file` can throw exceptions, but `RestoreWallet` is expected to return a nullptr with a populated `errors` parameter. This is fixed by wrapping `copy_file` and `LoadWallet` (for good measure) in a `try` block, and converting any exceptions to the intended return style.

  Bug 2: `util::Result` turns what would have been a `false` unique_ptr into a `true` nullptr result, which leads to nullptr dereferences in at least the 3 cases of wallet creation/loading/restoring. This is fixed by keeping the pointer as a plain `std::unique_ptr` until actually returning it (ie, after the nullptr check).

  Fixes bitcoin-core/gui#661

ACKs for top commit:
  achow101:
    ACK c3e5365

Tree-SHA512: 4291b3dbbb147acea2e63a704324c9371bc16ecb4237f8753729b0b0a6e55c9758ad61bfe8bd432fd7b0bae95d8b63a9831e61ac8b8d5c0197b550a2e0f4a105
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 20, 2022
@ryanofsky
Use ResultPtr<unique_ptr> instead of Result<unique_ptr>
b69cc6f 
This is a partial fix for a GUI crash
bitcoin-core/gui#661 that avoids the crash but
doesn't fix the restoreWallet error reporting. bitcoin#26005 is a more complete fix.
@hebasto hebasto mentioned this pull request Mar 27, 2023
Closed
@bitcoin bitcoin locked and limited conversation to collaborators Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@achow101 achow101 achow101 left review comments

@ryanofsky ryanofsky ryanofsky approved these changes

+3 more reviewers

@furszy furszy furszy approved these changes

@maflcko maflcko maflcko left review comments

@w0xlt w0xlt w0xlt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

24.0

Development

Successfully merging this pull request may close these issues.

Attempt to restore a non-readable wallet.dat silently crashes the GUI client

8 participants

@luke-jr @DrahtBot @achow101 @furszy @maflcko @ryanofsky @w0xlt @fanquake