There are two main changes: introduce LevelDbOptions to manage the leveldb::Options struct and changing CDBWrapper’s penv member to a unique_ptr. The first allows LevelDbOptions to destroy leveldb::Options members without CDBWrapper having to directly. The second change allows the leveldb::Env to be managed by penv. Both of these changes prevent a LeakSanitizer exception if the CDBWrapper ctor throws.
Fixes #22592
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
coins_view and I cannot see why with the CI. I am unable to reproduce this on my machine.
162 LogPrintf("Opening LevelDB in %s\n", path.string());
163 }
164- leveldb::Status status = leveldb::DB::Open(options, path.string(), &pdb);
165+ leveldb::Status status = leveldb::DB::Open(*options, path.string(), &pdb);
166+ if (!status.ok()) {
167+ delete penv;
penv is deleted, then ~CDBWrapper() gonna delete it again? Missing = nullptr?
!status.ok then HandleError throws and the destructor isn’t called
Maybe worth making penv a unique_ptr too, if we need manual handling with delete? Or do you think this would be kinda out of scope for this PR?
Adding addition delete into new code does not seem… “right” (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-newdelete).
unique_ptr
re: #22663 (review)
If
penvis deleted, then~CDBWrapper()gonna delete it again? Missing= nullptr?
In commit “dbwrapper: wrap options in unique_ptr and use DeleteOptions” (6770f066b69c6c271e7096cc647d5d42762f6b05)
It does seem fragile to rely on HandleError bypassing the destructor to avoid a double-delete. Would second the suggestion to add = nullptr to make the destructor safe if it were called. Or could add a “// Deleting env here without resetting it is safe because the destructor will never be called” comment to clarify. Or could make env a unique_ptr, which also seemed like a good suggestion
penv a unique_ptr
118+ options->paranoid_checks = true;
119 }
120- SetMaxOpenFiles(&options);
121- return options;
122+ SetMaxOpenFiles(options);
123+}
In commit “dbwrapper: wrap options in unique_ptr and use DeleteOptions” (6770f066b69c6c271e7096cc647d5d42762f6b05)
It’s not too important, but I don’t think there’s an actual need to change this function, and the previous version of this that just returned a struct instead of dereferencing a pointer seemed more straightforward.
To keep the previous version, I believe you would just need to write new leveldb::Options{GetOptions(...)} below.
Would be nice to add [[nodiscard]] to the previous version, if you did keep it though.
std::unique_ptr<leveldb::Options, void (*)(leveldb::Options*) directly, instead of using a pointer positional out argument (which, strictly, seems a deterioration of API clarity to me).
GetOptions logic into the ctor of LevelDbOptions
129+ options->filter_policy = nullptr;
130+ delete options->info_log;
131+ options->info_log = nullptr;
132+ delete options->block_cache;
133+ options->block_cache = nullptr;
134+ options->env = nullptr;
In commit “dbwrapper: wrap options in unique_ptr and use DeleteOptions” (6770f066b69c6c271e7096cc647d5d42762f6b05)
It seems dodgy to set env to null without freeing it here. I think it would be better to either:
assert(!options->env) hereenv and options lifetimes as more independent
182@@ -170,14 +183,7 @@ CDBWrapper::~CDBWrapper()
183 {
184 delete pdb;
In commit “dbwrapper: wrap options in unique_ptr and use DeleteOptions” (6770f066b69c6c271e7096cc647d5d42762f6b05)
I liked the suggestion to make env a unique_ptr #22663 (review), and maybe pdb could be a unique_ptr too?
pdb a unique_ptr and also pass it in to Open as leveldb::DB** when it is nullptr at this point. Maybe I am missing something.
re: #22663 (review)
I was not sure how to make
pdba unique_ptr and also pass it in toOpenasleveldb::DB**when it isnullptrat this point. Maybe I am missing something.
No need to do this, but you would need to pass a temporary pointer to Open, and switch to unique_ptr after it returns:
0leveldb::DB* new_db = nullptr;
1leveldb::Status status = leveldb::DB::Open(options, fs::PathToString(path), &new_db);
2pdb.reset(new_db);
121- return options;
122+ SetMaxOpenFiles(options);
123+}
124+
125+// Custom deleter for unique_ptr<leveldb::Options>.
126+static void DeleteOptions(leveldb::Options* options)
In commit “dbwrapper: wrap options in unique_ptr and use DeleteOptions” (6770f066b69c6c271e7096cc647d5d42762f6b05)
Custom deleter seems to be forgetting to delete options, which would explain the fuzzing leak? #22663 (comment)
~LevelDbOptions
6770f066b69c6c271e7096cc647d5d42762f6b05 looks pretty good if the leak is fixed. It generally seems to moves things in a good direction.
I do think more ideally, if the goal is to make the options struct more RAII-like and exception-safe, instead of requiring it to be allocated separately on the heap and requiring it to be paired with a custom deleter, it would be better to just inherit from it and write a normal destructor:
0struct LevelDbOptions : public leveldb::Options {
1 ~LevelDbOptions() {
2 delete filter_policy;
3 delete info_log;
4 ...
5 }
6}
103- options.block_cache = leveldb::NewLRUCache(nCacheSize / 2);
104- options.write_buffer_size = nCacheSize / 4; // up to two write buffers may be held in memory simultaneously
105- options.filter_policy = leveldb::NewBloomFilterPolicy(10);
106- options.compression = leveldb::kNoCompression;
107- options.info_log = new CBitcoinLevelDBLogger();
108+LevelDbOptions::LevelDbOptions() {
0LevelDbOptions::LevelDbOptions()
1{
nit: clang-format
You may install clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script to preempt whitespace nitpicking.
23+ LevelDbOptions();
24+
25+ ~LevelDbOptions();
26+
27+ /** Sets block_cache and write_buffer_size since nCacheSize isn't known when
28+ * LevelDbOptions ctor is called.
I don’t think this is strictly true? Or am I missing something? nCacheSize is passed into CDBWrapper::CDBWrapper, so it’s absolutely known at the beginning, couldn’t you pass it in like
0CDBWrapper::CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory, bool fWipe, bool obfuscate)
1 : m_name{fs::PathToString(path.stem())},
2 options(nCacheSize)
3⋮
4
5struct LevelDbOptions : public leveldb::Options {
6 LevelDbOptions(size_t nCacheSize);
183@@ -178,10 +184,10 @@ class CDBWrapper
184 friend const std::vector<unsigned char>& dbwrapper_private::GetObfuscateKey(const CDBWrapper &w);
185 private:
186 //! custom environment this database is using (may be nullptr in case of default environment)
187- leveldb::Env* penv;
188+ std::unique_ptr<leveldb::Env> penv;
I think this occurs and is unrelated to this PR because:
0main thread:
1* AppInitMain
2 * ChainstateManager::ActiveTip()
3 * CChain::Tip() -- accesses `vChain` without a lock (read)
0b-loadblk thread:
1* ThreadImport
2 * ActivateBestChain called (src/node/blockstorage.cpp:883)
3 * ActivateBestChainStep
4 * ConnectTip
5 * SetTip
6 * vChain resize (write)
This change allows LevelDbOptions to destroy leveldb::Options
members instead of doing so in CDBWrapper's destructor. This change
prevents a LeakSanitizer exception if the CDBWrapper constructor
throws.
This changes the naming of penv to m_env and also makes it a
unique_ptr. Like the previous commit, this prevents a LeakSanitizer
exception if the constructor throws since the unique_ptr will clean
itself up.
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.
unfortunately don’t have time to work on this, lmk if i should close it
Thanks. Closed and added up-for-grabs.