wallet: Fix segfault in CreateWalletFromFile, Pass error to rpc caller #16661

1. 
   MarcoFalke commented at 9:18 PM on August 19, 2019: member

   CreateWalletFromFile has a bunch of issues:

   • walletFile refers to freed memory and will thus corrupt the debug.log and/or crash the node if read
   • WalletParameterInteraction was moved to CreateWalletFromFile and WalletInit::ParameterInteraction without updating the documentation
   • loadwallet and createwallet will not pass up errors from CreateWalletFromFile to the RPC caller

   Fix each of those issues in a separate commit.

2. MarcoFalke force-pushed on Aug 19, 2019
3. 
   DrahtBot commented at 9:48 PM on August 19, 2019: member

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #16341 (Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
   • #16261 (gui: Refactor OpenWalletActivity by promag)
   • #15450 (gui: Create wallet menu option by achow101)
   • #10102 ([experimental] Multiprocess bitcoin 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.

4. 
   practicalswift commented at 9:52 PM on August 19, 2019: contributor

   Concept ACK -- very good find!

   The problem was introduced in #15491 which was merged into master on March 18, 2019. @MarcoFalke While checking this PR I stumbled upon what seems to be another lifetime issue in wallet.cpp (introduced in #14941 which was merged into master on January 15, 2019):

   https://github.com/bitcoin/bitcoin/blob/fa6431055abfacea4f72ef11d061c7c755de327a/src/wallet/wallet.cpp#L107-L111

   /cc @promag regarding the latter issue

   I guess we could make use of more static analysis to help us catch cases like this before merge :-)

5. DrahtBot added the label Tests on Aug 19, 2019
6. DrahtBot added the label Wallet on Aug 19, 2019
7. MarcoFalke removed the label Tests on Aug 19, 2019
8. MarcoFalke added this to the milestone 0.19.0 on Aug 19, 2019
9. 
   MarcoFalke commented at 11:05 PM on August 19, 2019: member

   @practicalswift I think it would be better to open a new issue for the lifetime issue you mention. And either explain why it is a lifetime issue or even better, include a test case that fails on current master.

10. 
    practicalswift commented at 10:28 AM on August 20, 2019: contributor

    I think it would be better to open a new issue for the lifetime issue you mention.

    I've now submitted #16668.

    And either explain why it is a lifetime issue or even better, include a test case that fails on current master.

    Constructing a test case that fails on current master will be tricky since lifetime issues are typically not handled consistently across C++ compiler implementations (or even across optimisation levels for the same implementation), but explaining is easy:

    After delete wallet; the pointer is invalid and shouldn't be used like in the call g_unloading_wallet_set.erase(wallet) :-)

    https://github.com/bitcoin/bitcoin/blob/fa6431055abfacea4f72ef11d061c7c755de327a/src/wallet/wallet.cpp#L107-L111

11. ariard approved
12. 
    ariard commented at 2:19 PM on August 20, 2019: member

    ACK 6f51ece, reviewed code and tests pass, maybe segfault fix could be more elaborated on the manual process to trigger crash in its message commit.

13. MarcoFalke force-pushed on Aug 20, 2019
14. MarcoFalke force-pushed on Aug 20, 2019
15. MarcoFalke force-pushed on Aug 20, 2019
16. 
    promag commented at 6:15 PM on August 20, 2019: member

    Concept ACK.

17. MarcoFalke force-pushed on Aug 20, 2019
18. 
    MarcoFalke commented at 6:26 PM on August 20, 2019: member

    Split up the changes into separate commits and added a commit body with an explanation to each. Also, fixed an appveyor issue with / vs \ in paths.

19. MarcoFalke force-pushed on Aug 21, 2019
20. MarcoFalke force-pushed on Aug 22, 2019
21. laanwj referenced this in commit 6e431296da on Sep 3, 2019
22. wallet: Fix segmentation fault in CreateWalletFromFile fa6e1900d5
23. wallet: Fix documentation around WalletParameterInteraction fa420d2a70
24. wallet: Pass error from CreateWalletFromFile to RPC

    Common errors and warnings should be translated when displayed in the
    GUI, but not translated when displayed elsewhere. The wallet method
    CreateWalletFromFile does not know its caller, so this commit changes it
    to return a bilingual_str to the caller.

    fad650db3f
25. test: Print both messages on failure in assert_raises_message fa504deefa
26. MarcoFalke force-pushed on Sep 3, 2019
27. 
    MarcoFalke commented at 6:40 PM on September 3, 2019: member

    I will fix the GUI issue (LC_ALL=de_DE.UTF-8 XDG_SESSION_TYPE=x11 BITCOIND=bitcoin-qt ./test/functional/wallet_multiwallet.py does not pass) in a separate pull request and the daemon issue in #16796. Thanks for the input everyone.

28. MarcoFalke closed this on Sep 3, 2019

    ---

29. MarcoFalke deleted the branch on Sep 3, 2019
30. sidhujag referenced this in commit f32507a747 on Sep 3, 2019
31. MarcoFalke locked this on Dec 16, 2021

Milestone
0.19.0