Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Jan 16, 2019

Conversation

Projects
None yet
10 participants
@Empact
Copy link
Member

commented Sep 19, 2018

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

@fanquake fanquake added the Wallet label Sep 19, 2018

Show resolved Hide resolved src/wallet/db.h Outdated
Show resolved Hide resolved src/wallet/db.h Outdated
@251Labs
Copy link
Contributor

left a comment

utACK 985892e

Show resolved Hide resolved src/wallet/db.h Outdated
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2018

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

left a comment

utACK 985892e.

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

Show resolved Hide resolved src/wallet/db.h Outdated
@achow101

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

utACK 985892e5aa75b76f71a354c8d835d7729048cccc

@meshcollider

This comment has been minimized.

Copy link
Member

commented Nov 10, 2018

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 Empact force-pushed the Empact:malloced-dbt branch Nov 12, 2018

@Empact 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 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

@Empact

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2018

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.

@Empact Empact force-pushed the Empact:malloced-dbt branch to 1a9f9f7 Nov 12, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2018

Appveyor failure looks unrelated.

@ken2812221

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

utACK 1a9f9f7

return m_dbt.get_size();
}

BerkeleyBatch::SafeDbt::operator Dbt*()

This comment has been minimized.

Copy link
@practicalswift

practicalswift Nov 20, 2018

Member

const? :-)

This comment has been minimized.

Copy link
@laanwj

laanwj Nov 23, 2018

Member

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

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

utACK 1a9f9f7

{
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 comment has been minimized.

Copy link
@laanwj

laanwj Nov 23, 2018

Member

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.

This comment has been minimized.

Copy link
@Empact
@Empact

This comment has been minimized.

Copy link
Member Author

commented Nov 25, 2018

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.

@Empact Empact force-pushed the Empact:malloced-dbt branch to 4a86a0a Nov 25, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Nov 25, 2018

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

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 16, 2019

Merge #14268: Introduce SafeDbt to handle Dbt with free or memory_cle…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.