Creating a copy of the pointer to the underlying data of the stream is not enough to copy the data.
Currently this happens to work sometimes, because the stream may not immediately free unused memory. However, there is no guarantee by the stream interface to always behave this way. Also, if vector::clear is called on the underlying memory, any pointers to it are invalid.
Fix this, by creating a full copy of all bytes.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
I believe this bug existed since the code was written in commit 22401f17e026ead4bc3fe96967eec56a719a4f75.
If one wants to trigger the bug in any c++ standard library, forcing a clear to also shrink may work:
0diff --git a/src/streams.h b/src/streams.h
1index bc04a2babd..26320db602 100644
2--- a/src/streams.h
3+++ b/src/streams.h
4@@ -10,6 +10,7 @@
5 #include <span.h>
6 #include <support/allocators/zeroafterfree.h>
7 #include <util/overflow.h>
8+#include <util/vector.h>
9
10 #include <algorithm>
11 #include <assert.h>
12@@ -227,7 +228,7 @@ public:
13 memcpy(dst.data(), &vch[m_read_pos], dst.size());
14 if (next_read_pos.value() == vch.size()) {
15 m_read_pos = 0;
16- vch.clear();
17+ ClearShrink(vch);//.clear();
18 return;
19 }
20 m_read_pos = next_read_pos.value();
This should then crash tests, such as:
0./test/functional/test_runner.py --valgrind wallet_migration
1valgrind ./src/test/test_bitcoin -t walletdb_tests --catch_system_errors=no
An alternative would be to assert early when a clear happened:
0diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
1index 9820c7c0ee..dbdd4181c4 100644
2--- a/src/wallet/walletdb.cpp
3+++ b/src/wallet/walletdb.cpp
4@@ -1406,4 +1406,8 @@ bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& types)
5 std::string type;
6 key >> type;
7+ if(key.size()==0){
8+ std::cout << type << std::endl;
9+ assert(key.size());
10+ }
11
12 if (types.count(type) > 0) {
This shows that the violating keys are:
hdchain for the unit test walletdb_testsflags for wallet_migration.py
