The implementation in current master can not detect if the file ID is duplicate with flushed BerkeleyEnvironment. This PR would store the file ID in a global variable g_fileids and release it when the BerkeleyDatabase close. So it won't have to rely on a Db*.
Fix #14304
20 | @@ -20,6 +21,12 @@ 21 | #include <boost/thread.hpp> 22 | 23 | namespace { 24 | + 25 | +struct BerkeleyFileid { 26 | + u_int8_t value[DB_FILE_ID_LEN]; 27 | +}; 28 | +std::unordered_map<std::string, BerkeleyFileid> g_fileids;
I think it'd be more correct to add a std::unordered_map<std::string, BerkeleyFileid> m_fileids member in BerkeleyEnvironment member instead of a g_fileids global because there can be duplicate filenames if the files are in different directories.
Also, making this a member would be consistent with existing mapFileUseCount mapDb members there which are also maps indexed by filename. In the future it could be nice to consolidate the 3 maps.
Updated
Great catch, I was surprised to see this bug. It seems like the problem here is that BerkeleyEnvironment::Flush actually fully unloads databases, so the check for duplicate fileids when opening a new database doesn't work at all after flushing.
I wonder if you could make a python test case out of the steps in #14304.
<!--e57a25ab6845829454e8d69fc972939a-->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.
48 | - if (item.second && item.second->get_mpf()->get_fileid(item_fileid) == 0 && 49 | - memcmp(fileid, item_fileid, sizeof(fileid)) == 0) { 50 | - const char* item_filename = nullptr; 51 | - item.second->get_dbname(&item_filename, nullptr); 52 | + for (const auto& item : env.m_fileids) { 53 | + if (item.first != filename && memcmp(fileid.value, item.second.value, sizeof(fileid)) == 0) {
move memcmp to BerkeleyFileid::operator==?
done
56 | - item_filename ? item_filename : "(unknown database)")); 57 | + HexStr(std::begin(item.second.value), std::end(item.second.value)), item.first)); 58 | } 59 | } 60 | + 61 | + env.m_fileids.emplace(filename, fileid);
Since CheckUniqueFileid is called on all BerkeleyEnvironment instances, this will incorrectly add the filename to unrelated BerkeleyEnvironment instances that don't actually contain filename. I'd suggest dropping this line, adding a BerkeleyFileid& fileid output parameter to CheckUniqueFileid, and passing in this->env->m_fileids[strFilename] where CheckUniqueFileid is called in BerkeleyBatch::BerkeleyBatch.
done
88 | @@ -89,7 +89,7 @@ bool FileLock::TryLock() 89 | return false; 90 | } 91 | _OVERLAPPED overlapped = {0}; 92 | - if (!LockFileEx(hFile, LOCKFILE_EXCLUSIVE_LOCK | LOCKFILE_FAIL_IMMEDIATELY, 0, 0, 0, &overlapped)) { 93 | + if (!LockFileEx(hFile, LOCKFILE_EXCLUSIVE_LOCK | LOCKFILE_FAIL_IMMEDIATELY, 0, -1, -1, &overlapped)) {
In commit "util: Fix broken Windows file lock"
Can you describe how the lock is broken in the commit message?
That was introduced by #13862. I assume that it could work with length 0 if it is a exclusive lock, but it doesn't. I would have to specify -1, -1 to make it work.
The boost implementation:
825 | @@ -826,6 +826,8 @@ void BerkeleyDatabase::Flush(bool shutdown)
826 | LOCK(cs_db);
Note that if two or more BerkeleyDatabase share the same BerkeleyEnvironment, the env would be a wild pointer after the first call.
Good catch indeed. The bug was not introduced in 0b82bac but in a769461d?
Thread #14320 (review)
Good catch indeed. The bug was not introduced in 0b82bac but in a769461?
0b82bac76d0f842bd2294a290388536951fbc576 added the call to erase the environment too early and a769461d5e37ddcb771ae836254fdc69177a28c4 just moved the call, IIUC.
502 | @@ -503,8 +503,8 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo 503 | // be implemented, so no equality checks are needed at all. (Newer 504 | // versions of BDB have an set_lk_exclusive method for this 505 | // purpose, but the older version we use does not.) 506 | - for (const auto& env : g_dbenvs) { 507 | - CheckUniqueFileid(env.second, strFilename, *pdb_temp); 508 | + for (auto& env : g_dbenvs) {
I think you could restore auto& env to const auto& env here.
47 | - if (item.second && item.second->get_mpf()->get_fileid(item_fileid) == 0 && 48 | - memcmp(fileid, item_fileid, sizeof(fileid)) == 0) { 49 | - const char* item_filename = nullptr; 50 | - item.second->get_dbname(&item_filename, nullptr); 51 | + for (const auto& item : env.m_fileids) { 52 | + if (item.first != filename && fileid == item.second) {
I think item.first != filename condition makes the check too loose again because the same filename can exist in different directories (and we'd expect pretty much all wallet directories to have a wallet.dat file).
Perhaps change condition to: if (item.second == fileid && !(&item.second == &fileid))
50 | @@ -56,6 +51,11 @@ CCriticalSection cs_db; 51 | std::map<std::string, BerkeleyEnvironment> g_dbenvs GUARDED_BY(cs_db); //!< Map from directory name to open db environment. 52 | } // namespace 53 | 54 | +bool BerkeleyFileid::operator==(const BerkeleyFileid& b) const 55 | +{ 56 | + return memcmp(this, &b, sizeof(BerkeleyFileid)) == 0;
It might be safer if memcmp compared just the value field rather than the whole object, since it seems possible the compiler could pad the objects. Maybe:
return memcmp(this->field, b.field, sizeof(this->field)) == 0;
825 | @@ -826,6 +826,8 @@ void BerkeleyDatabase::Flush(bool shutdown) 826 | LOCK(cs_db); 827 | g_dbenvs.erase(env->Directory().string()); 828 | env = nullptr; 829 | + } else {
Why not call m_fileds.erase if shutdown is true? It seems ok to only erase if shutdown is false, but if there is a specific reason for not erasing when shutting down, it would be useful to note in a code comment.
That would cause an access violation on Windows since it's a wild pointer in the second call. Also, since the env is deleted when g_dbenvs.erase called, the map doesn't exist anymore.
utACK cb4af1969c03b56bc18b63286627922db86743d2. Thanks for the fix and all the followup!
Is this for backport?
Is this for backport?
Could be backported, but I think the benefit would be very minimal. I think you have to literally copy a wallet database file and use the loadwallet RPC in order to trigger the missing error check that this PR restores.
utACK f936b8d14bfa8c758984430181db20514d34accd. Only change since previous review is adding code comment.
<!--32850dd3fdea838b4049e64f46995ea2-->
| Coverage | Change (pull 14320) | Reference (master) |
|---|---|---|
| Lines | +0.0019 % | 87.0361 % |
| Functions | -0.0049 % | 84.1130 % |
| Branches | -0.0008 % | 51.5451 % |
Rebased
74 | wallet_names.remove('w7_symlink')
75 | + wallet_exclude = {'w7'}
76 | extra_args = ['-wallet={}'.format(n) for n in wallet_names]
77 | self.start_node(0, extra_args)
78 | - assert_equal(set(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), set(['', 'w3', 'w2', 'sub/w5', 'w7', 'w7', 'w1', 'w8', 'w']))
79 | + assert_equal(set(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), set(['', 'w3', 'w2', os.path.join('sub', 'w5'), 'w7', 'w7', 'w1', 'w8', 'w']) - wallet_exclude)
'w7', 'w7'
Code precedes this PR, but I think listing w7 twice has no effect.
utACK 02b6bd343da296dc29ea017447b18c9afa13b5e3. Only changes since last review are rebase and listwalletdir fixes.
Just split appveyor part to #14559 to make this PR easier to review.
825 | @@ -826,6 +826,10 @@ void BerkeleyDatabase::Flush(bool shutdown) 826 | LOCK(cs_db); 827 | g_dbenvs.erase(env->Directory().string()); 828 | env = nullptr; 829 | + } else { 830 | + // To avoid accessing a map that has already deconstructed, do not call this when shutdown is true. g_dbenvs.erase would also erase this value. 831 | + // TODO: get rid of wild pointers 832 | + env->m_fileids.erase(strFile);
It might not be clear what "wild pointers" refers to in this todo (laanwj asked about it here: #14531 (review)). Wild pointers could mean either dead pointers or raw pointers, but I think the TODO is referring to the bug described here: #14320 (review). Maybe could be expanded as:
// TODO: To avoid g_dbenvs.erase erasing the environment prematurely after the
// first database shutdown when multiple databases are open in the same
// environment, should replace raw database `env` pointers with shared or weak
// pointers, or else separate the database and environment shutdowns so
// environments can be shut down after databases.
utACK ef16fc5da6ca6b33d4a92e509d627d908f8ed995. Only change since last review is dropping appveyor commit and related wallet_multiwallet.py test fixes.
24 | #include <db_cxx.h>
25 |
26 | static const unsigned int DEFAULT_WALLET_DBLOGSIZE = 100;
27 | static const bool DEFAULT_WALLET_PRIVDB = true;
28 |
29 | +struct BerkeleyFileid {
ef16fc5 -> 4ea7732
utACK 4ea77320c5f0b275876be41ff530bb328ba0cb87. Only changes are fileid class rename and todo comment.
Tested ACK 4ea7732, new test fails without this fix.
utACK 6241eb3224d95ea04c04dcab2dac88687f49440e