Avoid triggering undefined behaviour (std::memset(nullptr, 0, 0)) if an invalid string is passed to DecodeSecret(...).
Background reading: memcpy (and friends) with NULL pointers
Steps to reproduce:
./configure --with-sanitizers=undefined && make check && ./test/functional/test_runner.py
Shouldn't memory_cleanse be more robust in handling zero-length data?
@MarcoFalke Sure! Added a commit. Please re-review :-)
141 | @@ -142,7 +142,9 @@ CKey DecodeSecret(const std::string& str) 142 | key.Set(data.begin() + privkey_prefix.size(), data.begin() + privkey_prefix.size() + 32, compressed); 143 | } 144 | } 145 | - memory_cleanse(data.data(), data.size()); 146 | + if (data.data()) { 147 | + memory_cleanse(data.data(), data.size()); 148 | + }
This check is no longer needed?
Notes If size() is 0, data() may or may not return a null pointer. https://en.cppreference.com/w/cpp/container/vector/data
Shouldn't memory_cleanse be more robust in handling zero-length data? @MarcoFalke the same for
memset?
-0 ae17de5 because it changes memory_cleanse invariant.
IMO should avoid the call to memory_cleanse.
@MarcoFalke I think @promag has a good point. Now reverted to the original version.
@promag Thanks for reviewing. Now updated. Please re-review :-)
Could remove the suppression from the file in contrib?
@MarcoFalke Good point! Added commit 1cab4bb7e09c9d68e104460b67e5804848cd3c2e which removes UBSan suppression.
Please re-review :-)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
Rebased!
141 | @@ -142,7 +142,9 @@ CKey DecodeSecret(const std::string& str) 142 | key.Set(data.begin() + privkey_prefix.size(), data.begin() + privkey_prefix.size() + 32, compressed); 143 | } 144 | } 145 | - memory_cleanse(data.data(), data.size()); 146 | + if (data.data()) {
We don't need to memory_cleanse if data.size() is zero, right?
C.f. ae17de5
Now calling memory_cleanse only if data.data() != nullptr and data.size() > 0.
@luke-jr @promag @MarcoFalke Would you mind re-reviewing? :-)
@MarcoFalke Could this one get a release milestone? :-)
141 | @@ -142,7 +142,9 @@ CKey DecodeSecret(const std::string& str) 142 | key.Set(data.begin() + privkey_prefix.size(), data.begin() + privkey_prefix.size() + 32, compressed); 143 | } 144 | } 145 | - memory_cleanse(data.data(), data.size()); 146 | + if (data.data() != nullptr && data.size() > 0) {
seems like an !data.empty() check would be enough here?
utACK d855e4ca.