dbwrapper: properly destroy objects in case CDBWrapper ctor throws #22663

Crypt-iQ · 2021-08-08T00:42:24Z

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

DrahtBot · 2021-08-08T05:54:39Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25862 (refactor, kernel: Remove gArgs accesses from dbwrapper and txdb by ryanofsky)
#25740 (assumeutxo: background validation completion by jamesob)
#25667 (assumeutxo: snapshot initialization by jamesob)

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.

Crypt-iQ · 2021-08-08T12:48:25Z

One build is failing due to a leak while fuzzing coins_view and I cannot see why with the CI. I am unable to reproduce this on my machine.

Talkless · 2021-08-10T15:06:41Z

src/dbwrapper.cpp

-    leveldb::Status status = leveldb::DB::Open(options, path.string(), &pdb);
+    leveldb::Status status = leveldb::DB::Open(*options, path.string(), &pdb);
+    if (!status.ok()) {
+        delete penv;


If penv is deleted, then ~CDBWrapper() gonna delete it again? Missing = nullptr?

If !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).

good point, I can look into making it a unique_ptr

re: #22663 (comment)

If penv is deleted, then ~CDBWrapper() gonna delete it again? Missing = nullptr?

In commit "dbwrapper: wrap options in unique_ptr and use DeleteOptions" (6770f06)

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

The latest change makes penv a unique_ptr

ryanofsky

6770f06 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:

struct LevelDbOptions : public leveldb::Options {
  ~LevelDbOptions() {
    delete filter_policy;
    delete info_log;
    ...
  }
}

ryanofsky · 2021-10-29T19:41:09Z

src/dbwrapper.cpp

+static void GetOptions(size_t nCacheSize, leveldb::Options* options)
 {
-    leveldb::Options options;
-    options.block_cache = leveldb::NewLRUCache(nCacheSize / 2);
-    options.write_buffer_size = nCacheSize / 4; // up to two write buffers may be held in memory simultaneously
-    options.filter_policy = leveldb::NewBloomFilterPolicy(10);
-    options.compression = leveldb::kNoCompression;
-    options.info_log = new CBitcoinLevelDBLogger();
+    options->block_cache = leveldb::NewLRUCache(nCacheSize / 2);
+    options->write_buffer_size = nCacheSize / 4; // up to two write buffers may be held in memory simultaneously
+    options->filter_policy = leveldb::NewBloomFilterPolicy(10);
+    options->compression = leveldb::kNoCompression;
+    options->info_log = new CBitcoinLevelDBLogger();
    if (leveldb::kMajorVersion > 1 || (leveldb::kMajorVersion == 1 && leveldb::kMinorVersion >= 16)) {
        // LevelDB versions before 1.16 consider short writes to be corruption. Only trigger error
        // on corruption in later versions.
-        options.paranoid_checks = true;
+        options->paranoid_checks = true;
    }
-    SetMaxOpenFiles(&options);
-    return options;
+    SetMaxOpenFiles(options);
+}


In commit "dbwrapper: wrap options in unique_ptr and use DeleteOptions" (6770f06)

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.

I agree with @ryanofsky. I don't think there's a strong need to change this function, but if there is, I would prefer it to return the target type 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).

Will work on a patch with this, thanks for the review

Latest change lifts the GetOptions logic into the ctor of LevelDbOptions

ryanofsky · 2021-10-29T19:51:55Z

src/dbwrapper.cpp

+    options->info_log = nullptr;
+    delete options->block_cache;
+    options->block_cache = nullptr;
+    options->env = nullptr;


In commit "dbwrapper: wrap options in unique_ptr and use DeleteOptions" (6770f06)

It seems dodgy to set env to null without freeing it here. I think it would be better to either:

Set this to null immediately in the places where the env is deleted, in which case you could assert(!options->env) here

Or, to just drop this line and treat env and options lifetimes as more independent

Or, to add a comment like "env was previously deleted and should not be freed here" here to make it clearer the inconsistency here is intentional

I opted for the second approach and dropped the line

ryanofsky · 2021-10-29T19:58:04Z

src/dbwrapper.cpp

-    leveldb::Status status = leveldb::DB::Open(options, path.string(), &pdb);
+    leveldb::Status status = leveldb::DB::Open(*options, path.string(), &pdb);
+    if (!status.ok()) {
+        delete penv;


re: #22663 (comment)

If penv is deleted, then ~CDBWrapper() gonna delete it again? Missing = nullptr?

In commit "dbwrapper: wrap options in unique_ptr and use DeleteOptions" (6770f06)

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

ryanofsky · 2021-10-29T20:00:07Z

src/dbwrapper.cpp

@@ -170,14 +183,7 @@ CDBWrapper::~CDBWrapper()
 {
    delete pdb;


In commit "dbwrapper: wrap options in unique_ptr and use DeleteOptions" (6770f06)

I liked the suggestion to make env a unique_ptr #22663 (comment), and maybe pdb could be a unique_ptr too?

I was not sure how to make 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 (comment)

I was not sure how to make 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.

No need to do this, but you would need to pass a temporary pointer to Open, and switch to unique_ptr after it returns:

leveldb::DB* new_db = nullptr; leveldb::Status status = leveldb::DB::Open(options, fs::PathToString(path), &new_db); pdb.reset(new_db);

ryanofsky · 2021-10-29T20:07:30Z

src/dbwrapper.cpp

+}
+
+// Custom deleter for unique_ptr<leveldb::Options>.
+static void DeleteOptions(leveldb::Options* options)


In commit "dbwrapper: wrap options in unique_ptr and use DeleteOptions" (6770f06)

Custom deleter seems to be forgetting to delete options, which would explain the fuzzing leak? #22663 (comment)

No longer using a custom deleter and instead using ~LevelDbOptions

laanwj · 2021-12-15T19:52:06Z

@Crypt-iQ can you please respond to @ryanofsky's comments and rebase ?

Crypt-iQ · 2021-12-15T21:31:45Z

Hi yes, I can get to it this weekend

Crypt-iQ · 2021-12-20T16:07:28Z

I've rebased and hopefully addressed all of the comments

src/dbwrapper.cpp

src/dbwrapper.h

ryanofsky

Code review ACK 2a5d230. This all looks good and is straightforward. Thanks for implementing all the suggested changes

ryanofsky · 2022-04-27T01:30:05Z

src/dbwrapper.cpp

@@ -170,14 +183,7 @@ CDBWrapper::~CDBWrapper()
 {
    delete pdb;


re: #22663 (comment)

I was not sure how to make 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.

No need to do this, but you would need to pass a temporary pointer to Open, and switch to unique_ptr after it returns:

leveldb::DB* new_db = nullptr; leveldb::Status status = leveldb::DB::Open(options, fs::PathToString(path), &new_db); pdb.reset(new_db);

src/dbwrapper.h

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.

achow101 · 2022-10-12T18:36:35Z

cc @jamesob

DrahtBot · 2022-10-13T16:08:35Z

🐙 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".}

Crypt-iQ · 2022-10-18T16:22:40Z

unfortunately don't have time to work on this, lmk if i should close it

fanquake · 2022-10-19T12:46:37Z

unfortunately don't have time to work on this, lmk if i should close it

Thanks. Closed and added up-for-grabs.

DrahtBot added the UTXO Db and Indexes label Aug 8, 2021

bitcoin deleted a comment from mrnguyenthanhtu Aug 8, 2021

DrahtBot mentioned this pull request Aug 8, 2021

assumeutxo #15606

Closed

18 tasks

Talkless reviewed Aug 10, 2021

View reviewed changes

DrahtBot mentioned this pull request Sep 10, 2021

refactor: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method #22937

Merged

DrahtBot added the Needs rebase label Oct 15, 2021

ryanofsky approved these changes Oct 29, 2021

View reviewed changes

Crypt-iQ force-pushed the unique_ptr_options_08072021 branch from 6770f06 to 7ba6aa2 Compare December 19, 2021 22:11

DrahtBot removed the Needs rebase label Dec 19, 2021

Crypt-iQ changed the title ~~dbwrapper: wrap options in unique_ptr and use DeleteOptions~~ dbwrapper: properly destroy objects in case CDBWrapper ctor throws Dec 20, 2021

maflcko reviewed Dec 20, 2021

View reviewed changes

src/dbwrapper.cpp Outdated Show resolved Hide resolved

Crypt-iQ force-pushed the unique_ptr_options_08072021 branch from 7ba6aa2 to 541b66f Compare February 5, 2022 20:14

DrahtBot mentioned this pull request Mar 27, 2022

[WIP] [kernelheaders 1/n] Cleave LevelDB headers from our header tree #24676

Closed

laanwj requested changes Apr 6, 2022

View reviewed changes

src/dbwrapper.h Outdated Show resolved Hide resolved

Crypt-iQ force-pushed the unique_ptr_options_08072021 branch from 541b66f to 2a5d230 Compare April 6, 2022 15:22

This was referenced Apr 14, 2022

assumeutxo: add init and completion logic #24232

Closed

Isolate the storage abstraction layer from the application/serialization layer #24922

Closed

ryanofsky requested a review from laanwj April 27, 2022 01:47

ryanofsky approved these changes Apr 27, 2022

View reviewed changes

maflcko approved these changes May 3, 2022

View reviewed changes

src/dbwrapper.h Outdated Show resolved Hide resolved

Crypt-iQ added 2 commits May 5, 2022 18:36

dbwrapper: make CDBWrapper use new LevelDbOptions

7177b4c

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.

dbwrapper: change CDBWrapper penv to unique_ptr, rename to m_env

b6a1677

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.

Crypt-iQ force-pushed the unique_ptr_options_08072021 branch from 2a5d230 to b6a1677 Compare May 5, 2022 22:39

This was referenced Jul 16, 2022

[kernel 3e/n] Decouple CDBWrapper and its kernel users from ArgsManager #25623

Closed

assumeutxo: snapshot initialization #25667

Merged

DrahtBot mentioned this pull request Jul 29, 2022

assumeutxo: background validation completion #25740

Merged

DrahtBot mentioned this pull request Aug 17, 2022

refactor, kernel: Remove gArgs accesses from dbwrapper and txdb #25862

Merged

DrahtBot added the Needs rebase label Oct 13, 2022

fanquake added the Up for grabs label Oct 19, 2022

fanquake closed this Oct 19, 2022

bitcoin locked and limited conversation to collaborators Oct 19, 2023

dbwrapper: properly destroy objects in case CDBWrapper ctor throws #22663

dbwrapper: properly destroy objects in case CDBWrapper ctor throws #22663

Uh oh!

Conversation

Crypt-iQ commented Aug 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Crypt-iQ commented Aug 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Dec 15, 2021

Uh oh!

Crypt-iQ commented Dec 15, 2021

Uh oh!

Crypt-iQ commented Dec 20, 2021

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Oct 12, 2022

Uh oh!

DrahtBot commented Oct 13, 2022

Uh oh!

Crypt-iQ commented Oct 18, 2022

Uh oh!

fanquake commented Oct 19, 2022

Uh oh!

Uh oh!

Crypt-iQ commented Aug 8, 2021 •

edited

Loading

DrahtBot commented Aug 8, 2021 •

edited

Loading

Crypt-iQ commented Aug 8, 2021 •

edited

Loading

laanwj Nov 10, 2021 •

edited

Loading