refactor: Remove excess reserve() call for SecureString

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

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.

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.

DrahtBot added the label Refactoring on Feb 1, 2024

epiccurious commented at 1:04 am on February 2, 2024: none

Tested ACK. Passed the functional test runner.

Empact commented at 5:03 pm on February 2, 2024: member

@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?

knst commented at 5:26 pm on February 2, 2024: none

@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.

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

Is it not worthwhile to prevent reallocation if we can, even if there is not a data disclosure risk with this allocator?

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

refactor: avoid 2nd call of constructor for SecureString, add missing const b566359693

knst force-pushed on Feb 8, 2024

knst commented at 2:52 pm on February 8, 2024: none

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.

epiccurious commented at 2:13 am on February 11, 2024: none

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?

refactor: add a const for SecureString in rpc/wallet f99d92bde0

knst commented at 6:29 pm on February 12, 2024: none

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.

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.

achow101 closed this on Apr 9, 2024

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

Code Coverage

Reviews

Conflicts