refactor: Remove excess reserve() call for SecureString #29364

1. 
   knst commented at 3:48 pm on February 1, 2024: contributor

   This PR removes excess reserve() call for SecureString

   Call reverse was introduced when std::string was used. For std::string, this makes sense as it prevents re-allocation when the string’s size increases to prevent a situation that memory will be re-allocated and “zeroing” will clean only the latest region, but not all of them. However, with the use of a custom allocator, this call is no longer necessary; secure_allocator from support/allocators/secure.h ensures that all re-allocation would be safely re-written and secret passphrase won’t stay in memory.

2. 
   DrahtBot commented at 3:48 pm on February 1, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	epiccurious

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #25722 (refactor: Use util::Result class for wallet loading 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.

3. DrahtBot added the label Refactoring on Feb 1, 2024
4. 
   epiccurious commented at 1:04 am on February 2, 2024: contributor

   Tested ACK. Passed the functional test runner.
5. 
   Empact commented at 5:03 pm on February 2, 2024: contributor

   @knst can you explain or cite a reference as to why these calls are no longer beneficial? I would expect the implementation of reserve to be relative to the supplied allocator. Is it because size change is inexpensive with a reserved page via LockedPoolManager?
6. 
   knst commented at 5:26 pm on February 2, 2024: contributor

   @Empact

   In the original 2011 implementation, a regular std::string was used for the passphrase:

    0commit b7bcaf940d27fa8cfe89422943fbeaab7a350930
    1Author: Wladimir J. van der Laan <laanwj@gmail.com>
    2Date:   Wed Aug 24 22:07:26 2011 +0200
    3
    4    Wallet encryption part 2: ask passphrase when needed, add menu options
    5diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp
    6new file mode 100644
    7index 0000000000..a297513a62
    8--- /dev/null
    9+++ b/src/qt/askpassphrasedialog.cpp
   10....
   11    std::string oldpass, newpass1, newpass2;
   12    // TODO: mlock memory / munlock on return so they will not be swapped out, really need "mlockedstring" wrapper class to do this safely
   13    oldpass.reserve(MAX_PASSPHRASE_SIZE);
   14    newpass1.reserve(MAX_PASSPHRASE_SIZE);
   15    newpass2.reserve(MAX_PASSPHRASE_SIZE);
   16....
   

   Reserving memory ensured that the memory would not be reallocated inside std::string when the content changed. This helped implement a crucial feature: we could zero out the memory before freeing it, ensuring that if the memory were reallocated to another object, the application would crash in a dump or something similar would happen—instead of the passphrase, there would be zeroes. To ensure that std::string always worked with the same memory region (allowing it to be zeroed), reserve was called.

   Later on, std::string was replaced with SecureString (which is still a std::string but uses a special allocator). We no longer need to worry about reallocation inside SecureString, because all memory is allocated by a special allocator that handles all cleaning, ensuring that the secret bytes of the passphrase would never leak out of this object. Here’s safety memory cleaning in deallocation (whatever how many times std::string allocated/deallocated memory – all memory would be “cleansed”): https://github.com/bitcoin/bitcoin/blob/master/src/support/allocators/secure.h#L39 https://github.com/bitcoin/bitcoin/blob/master/src/support/cleanse.h#L11-L13

   Summarizing: the use of reserve has been introduced years ago not for optimization purposes, but to ensure the safety of the user’s passphrase. This concern is no longer necessary with the introduction of SecureString.

7. 
   Empact commented at 12:22 pm on February 7, 2024: contributor

   Is it not worthwhile to prevent reallocation if we can, even if there is not a data disclosure risk with this allocator?
8. refactor: remove exceess reserve() call for SecureString

   This call was introduced when std::string was used. For std::string, this
   makes sense as it prevents re-allocation when the string's size increases.
   However, with the use of a custom allocator, this call is no longer necessary.

   9060ac23bf
9. refactor: avoid 2nd call of constructor for SecureString, add missing const b566359693
10. knst force-pushed on Feb 8, 2024
11. 
    knst commented at 2:52 pm on February 8, 2024: contributor

    Is it not worthwhile to prevent reallocation if we can @Empact I added one more commit which adds const and changes way of initialization of SecureString - there were not any re-allocations, but now that’s enforced by const.

    Except wallet/rpc/wallet.cpp - there’s also no reallocations, objects are not changed, but code readability would be worsened if I add there const.

12. 
    epiccurious commented at 2:13 am on February 11, 2024: contributor

    code readability would be worsened if I add there const

    Seems like adding a const here would be worthwhile. Can you elaborate on the worsened code readability?

13. refactor: add a const for SecureString in rpc/wallet f99d92bde0
14. 
    knst commented at 6:29 pm on February 12, 2024: contributor

    Seems like adding a const here would be worthwhile. Can you elaborate on the worsened code readability? @epiccurious , here’s it: https://github.com/bitcoin/bitcoin/pull/29364/commits/f99d92bde055efaf20c89198eb22dc3e35778d1e not too bad actually, I added a commit.

15. 
    achow101 commented at 3:55 pm on April 9, 2024: member

    The PR didn’t seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open PRs.

    Closing due to lack of interest.

16. achow101 closed this on Apr 9, 2024

    ---

17. bitcoin locked this on Apr 9, 2025

Contributors
knst DrahtBot epiccurious Empact achow101

Labels
Refactoring