Introduce SafeDbt to handle Dbt with free or memory_cleanse raii-style by Empact · Pull Request #14268 · bitcoin/bitcoin

Empact · 2018-09-19T07:24:30Z

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

l2a5b1

utACK 985892e

DrahtBot · 2018-09-28T06:09:47Z

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

promag

utACK 985892e.

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

achow101 · 2018-10-09T01:18:11Z

utACK 985892e5aa75b76f71a354c8d835d7729048cccc

meshcollider · 2018-11-10T06:45:08Z

utACK 985892e

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 · 2018-11-12T23:42:46Z

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

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

Empact · 2018-11-13T04:43:17Z

Appveyor failure looks unrelated.

ken2812221 · 2018-11-13T05:56:56Z

utACK 1a9f9f7

practicalswift · 2018-11-20T06:56:45Z

+    return m_dbt.get_size();
+}
+
+BerkeleyBatch::SafeDbt::operator Dbt*()


const? :-)

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 · 2018-11-23T07:13:45Z

utACK 1a9f9f7

laanwj · 2018-11-23T07:17:53Z

+{
+    if (m_dbt.get_data() != nullptr) {
+        // Clear memory, e.g. in case it was a private key
+        memory_cleanse(m_dbt.get_data(), m_dbt.get_size());


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.

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 · 2018-11-25T02:51:41Z

Added another commit:

Make SafeDbt DB_DBT_MALLOC on default initialization
Ran clang-format

Separate for easier review, can squash.

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

Empact · 2018-11-25T04:41:02Z

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

…anse raii-style 4a86a0a Make SafeDbt DB_DBT_MALLOC on default initialization (Ben Woosley) 1a9f9f7 Introduce SafeDbt to handle DB_DBT_MALLOC raii-style (Ben Woosley) 951a44e Drop unused setRange arg to BerkeleyBatch::ReadAtCursor (Ben Woosley) Pull request description: This provides additional exception-safety and case handling for the proper freeing of the associated buffers. Tree-SHA512: a038d728290cdb3905e7d881608052a6675b6425729ceaf7cfe69a6e91c2ee293cdb01e4b695a20963459ffdd9d4a1f9a08b3c07b1b5ba1aa8590a8149f686db

Summary: bitcoin/bitcoin@951a44e --- Partial backport of Core [[bitcoin/bitcoin#14268 | PR14268]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7321

Summary: This provides additional exception-safety and case handling for the proper freeing of the associated buffers. bitcoin/bitcoin@1a9f9f7 --- Depends on D7321 Partial backport of Core [[bitcoin/bitcoin#14268 | PR14268]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7322

Summary: If we're constructing the SafeDbt without provided data, it is always malloced, so that is the case we expose. Also run clang-format. bitcoin/bitcoin@4a86a0a --- Depends on D7322 Concludes backport of Core [[bitcoin/bitcoin#14268 | PR14268]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7323

…ory_cleanse raii-style 4a86a0a Make SafeDbt DB_DBT_MALLOC on default initialization (Ben Woosley) 1a9f9f7 Introduce SafeDbt to handle DB_DBT_MALLOC raii-style (Ben Woosley) 951a44e Drop unused setRange arg to BerkeleyBatch::ReadAtCursor (Ben Woosley) Pull request description: This provides additional exception-safety and case handling for the proper freeing of the associated buffers. Tree-SHA512: a038d728290cdb3905e7d881608052a6675b6425729ceaf7cfe69a6e91c2ee293cdb01e4b695a20963459ffdd9d4a1f9a08b3c07b1b5ba1aa8590a8149f686db

…94_14969 Merge 0.18 PRs: bitcoin#14268, bitcoin#15408, bitcoin#14094, bitcoin#14935 and bitcoin#14969

Drop unused setRange arg to BerkeleyBatch::ReadAtCursor

951a44e

fanquake added the Wallet label Sep 19, 2018

practicalswift reviewed Sep 20, 2018

View reviewed changes

Comment thread src/wallet/db.h Outdated

practicalswift reviewed Sep 22, 2018

View reviewed changes

Comment thread src/wallet/db.h Outdated

l2a5b1 reviewed Sep 24, 2018

View reviewed changes

Comment thread src/wallet/db.h Outdated

promag reviewed Oct 1, 2018

View reviewed changes

Comment thread src/wallet/db.h Outdated

Empact force-pushed the malloced-dbt branch from 985892e to d18bea5 Compare November 12, 2018 23:39

Empact changed the title ~~Introduce MallocedDbt to handle DB_DBT_MALLOC raii-style~~ Introduce SafeDbt to handle Dbt with free or memory_cleanse raii-style Nov 12, 2018

Empact changed the title ~~Introduce SafeDbt to handle Dbt with free or memory_cleanse raii-style~~ Introduce SafeDbt to handle Dbt with free or memory_cleanse raii-style Nov 12, 2018

Introduce SafeDbt to handle DB_DBT_MALLOC raii-style

1a9f9f7

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

Empact force-pushed the malloced-dbt branch from d18bea5 to 1a9f9f7 Compare November 12, 2018 23:44

practicalswift reviewed Nov 20, 2018

View reviewed changes

laanwj reviewed Nov 23, 2018

View reviewed changes

Make SafeDbt DB_DBT_MALLOC on default initialization

4a86a0a

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

Empact force-pushed the malloced-dbt branch from 1943b40 to 4a86a0a Compare November 25, 2018 03:08

laanwj merged commit 4a86a0a into bitcoin:master Jan 16, 2019

UdjinM6 added a commit to dashpay/dash that referenced this pull request Oct 1, 2021

Merge pull request #4453 from dzutte-cpp/merge_14268_15408_partial140…

4c72a1a

…94_14969 Merge 0.18 PRs: bitcoin#14268, bitcoin#15408, bitcoin#14094, bitcoin#14935 and bitcoin#14969

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Uh oh!

Conversation

Empact commented Sep 19, 2018

Uh oh!

Uh oh!

Uh oh!

l2a5b1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Sep 28, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Oct 9, 2018

Uh oh!

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

Uh oh!

Uh oh!

Empact commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Nov 13, 2018

Uh oh!

ken2812221 commented Nov 13, 2018

Uh oh!

practicalswift Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj Nov 23, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 23, 2018

Uh oh!

laanwj Nov 23, 2018 • 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!

Empact Nov 25, 2018

Choose a reason for hiding this comment

Uh oh!

Empact commented Nov 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Nov 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

meshcollider commented Nov 10, 2018 •

edited

Loading

Empact commented Nov 12, 2018 •

edited

Loading

laanwj Nov 23, 2018 •

edited

Loading

Empact commented Nov 25, 2018 •

edited

Loading