Introduce SafeDbt to handle Dbt with free or memory

Empact commented at 7:24 AM on September 19, 2018: member

This provides additional exception-safety and case handling for the proper freeing of the associated buffers.

Drop unused setRange arg to BerkeleyBatch::ReadAtCursor 951a44e9cd

fanquake added the label Wallet on Sep 19, 2018

in src/wallet/db.h:185 in 985892e5aa outdated

 180 | +
 181 | +        ~MallocedDbt()
 182 | +        {
 183 | +            if (m_dbt.get_data() != nullptr) {
 184 | +                memory_cleanse(m_dbt.get_data(), m_dbt.get_size());
 185 | +                free(m_dbt.get_data());

practicalswift commented at 5:22 AM on September 20, 2018:

Perhaps add a comment here explaining why the explicit free is needed and where the matching allocation takes place?

in src/wallet/db.h:204 in 985892e5aa outdated

 199 | +        operator Dbt*()
 200 | +        {
 201 | +            return &m_dbt;
 202 | +        }
 203 | +    };
 204 | +}

practicalswift commented at 7:02 PM on September 22, 2018:

2018-09-22 20:42:07 cpplint(pr=14268): src/wallet/db.h:204:  Anonymous namespace should be terminated with "// namespace"  [readability/namespace] [5]

in src/wallet/db.h:172 in 985892e5aa outdated

 165 | @@ -166,6 +166,42 @@ class BerkeleyDatabase
 166 |      bool IsDummy() { return env == nullptr; }
 167 |  };
 168 |  
 169 | +namespace {
 170 | +    /** RAII class that provides access to a DB_DBT_MALLOC Dbt */
 171 | +    class MallocedDbt {
 172 | +    private:

l2a5b1 commented at 8:36 AM on September 24, 2018:

nit: You can drop the private access modifier. m_dbt is implicitly declared private, because private is the default class access level.

l2a5b1 commented at 8:44 AM on September 24, 2018: contributor

utACK 985892e

DrahtBot commented at 6:09 AM on September 28, 2018: member

Coverage	Change (pull 14268)	Reference (master)
Lines	+0.0099 %	87.0361 %
Functions	+0.0179 %	84.1130 %
Branches	-0.0022 %	51.5451 %

in src/wallet/db.h:171 in 985892e5aa outdated

 165 | @@ -166,6 +166,42 @@ class BerkeleyDatabase
 166 |      bool IsDummy() { return env == nullptr; }
 167 |  };
 168 |  
 169 | +namespace {
 170 | +    /** RAII class that provides access to a DB_DBT_MALLOC Dbt */
 171 | +    class MallocedDbt {

promag commented at 5:57 PM on October 1, 2018:

final

promag commented at 6:07 PM on October 1, 2018:

nit, IMO class name could be improved, as it also calls memory_cleanse. Maybe SafeDbt — I'm sure there is a better name 😛

promag commented at 6:17 PM on October 1, 2018: member

utACK 985892e.

nit, first commit kind of unrelated — could mention the dead code cleanup in the PR description.

achow101 commented at 1:18 AM on October 9, 2018: member

utACK 985892e5aa75b76f71a354c8d835d7729048cccc

meshcollider commented at 6:45 AM on November 10, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/14268/commits/985892e5aa75b76f71a354c8d835d7729048cccc

Style-nit: the developer notes say "No indentation for public/protected/private or for namespace", so you might want to change the MallocedDbt class to fit that.

First commit is unrelated as promag said, but a nice cleanup, the last use of setRange was in ListAccountCreditDebit which was removed when accounts were.

Empact force-pushed on Nov 12, 2018

Empact renamed this:
~~Introduce MallocedDbt to handle DB_DBT_MALLOC raii-style~~
Introduce `SafeDbt` to handle `Dbt` with `free` or `memory_cleanse` raii-style
on Nov 12, 2018

Empact renamed this:
~~Introduce `SafeDbt` to handle `Dbt` with `free` or `memory_cleanse` raii-style~~
Introduce SafeDbt to handle Dbt with free or memory_cleanse raii-style
on Nov 12, 2018

Empact commented at 11:42 PM on November 12, 2018: member

Ok just pushed some significant changes:

Now called SafeDbt, and applied in all cases where memory_cleanse was used with Dbt
No longer in an anonymous namespace as that is a violation of DCL59-CPP, instead nest it inside BerkeleyBatch, as all uses are therein.
Implementation is now in db.cpp

Introduce SafeDbt to handle DB_DBT_MALLOC raii-style

This provides additional exception-safety and case handling for the proper
freeing of the associated buffers.

1a9f9f7e5e

Empact force-pushed on Nov 12, 2018

Empact commented at 4:43 AM on November 13, 2018: member

Appveyor failure looks unrelated.

ken2812221 commented at 5:56 AM on November 13, 2018: contributor

utACK 1a9f9f7e5e2e73fb832f5b96ad7e9e57954f3f3c

in src/wallet/db.cpp:284 in 1a9f9f7e5e outdated

 279 | +u_int32_t BerkeleyBatch::SafeDbt::get_size() const
 280 | +{
 281 | +    return m_dbt.get_size();
 282 | +}
 283 | +
 284 | +BerkeleyBatch::SafeDbt::operator Dbt*()

practicalswift commented at 6:56 AM on November 20, 2018:

const? :-)

laanwj commented at 7:11 AM on November 23, 2018:

it's returning a mutable pointer to the internal structure, I don't think it'd make sense to make the method const (unless you mean it should return a const* pointer too, but that's likely not good enough for the BDB functions)

laanwj commented at 7:13 AM on November 23, 2018: member

utACK 1a9f9f7e5e2e73fb832f5b96ad7e9e57954f3f3c

in src/wallet/db.cpp:264 in 1a9f9f7e5e outdated

 259 | +
 260 | +BerkeleyBatch::SafeDbt::~SafeDbt()
 261 | +{
 262 | +    if (m_dbt.get_data() != nullptr) {
 263 | +        // Clear memory, e.g. in case it was a private key
 264 | +        memory_cleanse(m_dbt.get_data(), m_dbt.get_size());

laanwj commented at 7:17 AM on November 23, 2018:

This changes behavior: in the case of Write it was also cleansing the dtabase key, now it no longer is. Not sure if this is necessary, I don't think the database keys ever contain sensitive information, but it should be noted.

Empact commented at 2:28 AM on November 25, 2018:

I believe it is cleansing the key - if you're referring to the cleansing here: https://github.com/bitcoin/bitcoin/pull/14268/files#diff-c557c8e6d496041acccfd8ac4e3db625L257

Then the key and value are separate SafeDbt objects, with each managing/cleansing its own data: https://github.com/bitcoin/bitcoin/pull/14268/files#diff-c557c8e6d496041acccfd8ac4e3db625R258 https://github.com/bitcoin/bitcoin/pull/14268/files#diff-c557c8e6d496041acccfd8ac4e3db625R264

Empact commented at 2:51 AM on November 25, 2018: member

Added another commit:

Make SafeDbt DB_DBT_MALLOC on default initialization
Ran clang-format

Separate for easier review, can squash.

Make SafeDbt DB_DBT_MALLOC on default initialization

If we're constructing the SafeDbt without provided data, it is always malloced,
so that is the case we expose.

Also run clang-format.

4a86a0acd9

Empact force-pushed on Nov 25, 2018

Empact commented at 4:41 AM on November 25, 2018: member

Appveyor failure looks unrelated: "ConnectionAbortedError: [WinError 10053] An established connection was aborted by the software in your host machine"

laanwj merged this on Jan 16, 2019

laanwj closed this on Jan 16, 2019

laanwj referenced this in commit d44b01f028 on Jan 16, 2019

deadalnix referenced this in commit 88e8e4d48b on Sep 2, 2020

deadalnix referenced this in commit 481508fdbc on Sep 2, 2020

deadalnix referenced this in commit fe89cb23e2 on Sep 2, 2020

PastaPastaPasta referenced this in commit 69d625d186 on Jun 26, 2021

PastaPastaPasta referenced this in commit d1392bd021 on Jun 27, 2021

PastaPastaPasta referenced this in commit 1793714bac on Jun 28, 2021

PastaPastaPasta referenced this in commit cff8c0266e on Jun 29, 2021

Munkybooty referenced this in commit fc6cf31ec4 on Aug 21, 2021

dzutto referenced this in commit da1c405074 on Sep 24, 2021

dzutto referenced this in commit 228f1832cc on Sep 24, 2021

dzutto referenced this in commit ccf507f2e7 on Sep 24, 2021

dzutto referenced this in commit c8f58cac68 on Sep 27, 2021

dzutto referenced this in commit 0d1db5cde2 on Sep 30, 2021

dzutto referenced this in commit 8b47451341 on Oct 1, 2021

UdjinM6 referenced this in commit 4c72a1a975 on Oct 1, 2021

kittywhiskers referenced this in commit d6b71d811c on Oct 12, 2021

DrahtBot locked this on Dec 16, 2021

Introduce SafeDbt to handle Dbt with free or memory_cleanse raii-style #14268