CWallet::GetEncryptionKey() would return a reference to the internal
CWallet::vMasterKey, guarded by CWallet::cs_wallet, which is unsafe.
Returning a copy would be a shorter solution, but could have security
implications of the master key remaining somewhere in the memory even
after CWallet::Lock() (the current calls to
CWallet::GetEncryptionKey() are safe, but that is not future proof).
So, instead of EncryptSecret(m_storage.GetEncryptionKey(), ...)
change the GetEncryptionKey() method to provide the encryption
key to a given callback:
m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })
This silences the following (clang 18):
0wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return]
1 3520 | return vMasterKey;
2 | ^
Previously this PR modified both ArgsManager and wallet code. But the ArgsManager commit https://github.com/bitcoin/bitcoin/commit/856c88776f8486446602476a1c9e133ac0cff510 was merged in #29040 so now this only affects wallet code. The previous PR description was:
Avoid this unsafe pattern from ArgsManager and CWallet:
0class A
1{
2 Mutex mutex;
3 Foo member GUARDED_BY(mutex);
4 const Foo& Get()
5 {
6 LOCK(mutex);
7 return member;
8 } // callers of `Get()` will have access to `member` without owning the mutex.
