From 951a44e9cd6cf2b8058244f3f95181c5ba683fdd Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Wed, 19 Sep 2018 02:36:23 -0400 Subject: [PATCH 1/3] Drop unused setRange arg to BerkeleyBatch::ReadAtCursor --- src/wallet/db.h | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/wallet/db.h b/src/wallet/db.h index 467ed13b450..15577e66f16 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -315,20 +315,14 @@ public: return pcursor; } - int ReadAtCursor(Dbc* pcursor, CDataStream& ssKey, CDataStream& ssValue, bool setRange = false) + int ReadAtCursor(Dbc* pcursor, CDataStream& ssKey, CDataStream& ssValue) { // Read at cursor Dbt datKey; - unsigned int fFlags = DB_NEXT; - if (setRange) { - datKey.set_data(ssKey.data()); - datKey.set_size(ssKey.size()); - fFlags = DB_SET_RANGE; - } Dbt datValue; datKey.set_flags(DB_DBT_MALLOC); datValue.set_flags(DB_DBT_MALLOC); - int ret = pcursor->get(&datKey, &datValue, fFlags); + int ret = pcursor->get(&datKey, &datValue, DB_NEXT); if (ret != 0) return ret; else if (datKey.get_data() == nullptr || datValue.get_data() == nullptr) From 1a9f9f7e5e2e73fb832f5b96ad7e9e57954f3f3c Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Wed, 19 Sep 2018 02:38:40 -0400 Subject: [PATCH 2/3] 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. --- src/wallet/db.cpp | 39 ++++++++++++++++++++++++++ src/wallet/db.h | 71 +++++++++++++++++++++-------------------------- 2 files changed, 70 insertions(+), 40 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index a7bf89c5728..9bac3c49ccf 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -247,6 +247,45 @@ BerkeleyEnvironment::VerifyResult BerkeleyEnvironment::Verify(const std::string& return (fRecovered ? VerifyResult::RECOVER_OK : VerifyResult::RECOVER_FAIL); } +BerkeleyBatch::SafeDbt::SafeDbt(u_int32_t flags) +{ + m_dbt.set_flags(flags); +} + +BerkeleyBatch::SafeDbt::SafeDbt(void *data, size_t size) + : m_dbt(data, size) +{ +} + +BerkeleyBatch::SafeDbt::~SafeDbt() +{ + 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()); + // under DB_DBT_MALLOC, data is malloced by the Dbt, but must be + // freed by the caller. + // https://docs.oracle.com/cd/E17275_01/html/api_reference/C/dbt.html + if (m_dbt.get_flags() & DB_DBT_MALLOC) { + free(m_dbt.get_data()); + } + } +} + +const void* BerkeleyBatch::SafeDbt::get_data() const +{ + return m_dbt.get_data(); +} + +u_int32_t BerkeleyBatch::SafeDbt::get_size() const +{ + return m_dbt.get_size(); +} + +BerkeleyBatch::SafeDbt::operator Dbt*() +{ + return &m_dbt; +} + bool BerkeleyBatch::Recover(const fs::path& file_path, void *callbackDataIn, bool (*recoverKVcallback)(void* callbackData, CDataStream ssKey, CDataStream ssValue), std::string& newFilename) { std::string filename; diff --git a/src/wallet/db.h b/src/wallet/db.h index 15577e66f16..73cbff98fbc 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -166,10 +166,27 @@ private: bool IsDummy() { return env == nullptr; } }; - /** RAII class that provides access to a Berkeley database */ class BerkeleyBatch { + /** RAII class that automatically cleanses its data on destruction */ + class SafeDbt final { + Dbt m_dbt; + + public: + // construct Dbt with data or flags + SafeDbt(u_int32_t flags = 0); + SafeDbt(void *data, size_t size); + ~SafeDbt(); + + // delegate to Dbt + const void* get_data() const; + u_int32_t get_size() const; + + // conversion operator to access the underlying Dbt + operator Dbt*(); + }; + protected: Db* pdb; std::string strFile; @@ -197,7 +214,6 @@ public: /* verifies the database file */ static bool VerifyDatabaseFile(const fs::path& file_path, std::string& warningStr, std::string& errorStr, BerkeleyEnvironment::recoverFunc_type recoverFunc); -public: template bool Read(const K& key, T& value) { @@ -208,13 +224,11 @@ public: CDataStream ssKey(SER_DISK, CLIENT_VERSION); ssKey.reserve(1000); ssKey << key; - Dbt datKey(ssKey.data(), ssKey.size()); + SafeDbt datKey(ssKey.data(), ssKey.size()); // Read - Dbt datValue; - datValue.set_flags(DB_DBT_MALLOC); - int ret = pdb->get(activeTxn, &datKey, &datValue, 0); - memory_cleanse(datKey.get_data(), datKey.get_size()); + SafeDbt datValue(DB_DBT_MALLOC); + int ret = pdb->get(activeTxn, datKey, datValue, 0); bool success = false; if (datValue.get_data() != nullptr) { // Unserialize value @@ -225,10 +239,6 @@ public: } catch (const std::exception&) { // In this case success remains 'false' } - - // Clear and free memory - memory_cleanse(datValue.get_data(), datValue.get_size()); - free(datValue.get_data()); } return ret == 0 && success; } @@ -245,20 +255,16 @@ public: CDataStream ssKey(SER_DISK, CLIENT_VERSION); ssKey.reserve(1000); ssKey << key; - Dbt datKey(ssKey.data(), ssKey.size()); + SafeDbt datKey(ssKey.data(), ssKey.size()); // Value CDataStream ssValue(SER_DISK, CLIENT_VERSION); ssValue.reserve(10000); ssValue << value; - Dbt datValue(ssValue.data(), ssValue.size()); + SafeDbt datValue(ssValue.data(), ssValue.size()); // Write - int ret = pdb->put(activeTxn, &datKey, &datValue, (fOverwrite ? 0 : DB_NOOVERWRITE)); - - // Clear memory in case it was a private key - memory_cleanse(datKey.get_data(), datKey.get_size()); - memory_cleanse(datValue.get_data(), datValue.get_size()); + int ret = pdb->put(activeTxn, datKey, datValue, (fOverwrite ? 0 : DB_NOOVERWRITE)); return (ret == 0); } @@ -274,13 +280,10 @@ public: CDataStream ssKey(SER_DISK, CLIENT_VERSION); ssKey.reserve(1000); ssKey << key; - Dbt datKey(ssKey.data(), ssKey.size()); + SafeDbt datKey(ssKey.data(), ssKey.size()); // Erase - int ret = pdb->del(activeTxn, &datKey, 0); - - // Clear memory - memory_cleanse(datKey.get_data(), datKey.get_size()); + int ret = pdb->del(activeTxn, datKey, 0); return (ret == 0 || ret == DB_NOTFOUND); } @@ -294,13 +297,10 @@ public: CDataStream ssKey(SER_DISK, CLIENT_VERSION); ssKey.reserve(1000); ssKey << key; - Dbt datKey(ssKey.data(), ssKey.size()); + SafeDbt datKey(ssKey.data(), ssKey.size()); // Exists - int ret = pdb->exists(activeTxn, &datKey, 0); - - // Clear memory - memory_cleanse(datKey.get_data(), datKey.get_size()); + int ret = pdb->exists(activeTxn, datKey, 0); return (ret == 0); } @@ -318,11 +318,9 @@ public: int ReadAtCursor(Dbc* pcursor, CDataStream& ssKey, CDataStream& ssValue) { // Read at cursor - Dbt datKey; - Dbt datValue; - datKey.set_flags(DB_DBT_MALLOC); - datValue.set_flags(DB_DBT_MALLOC); - int ret = pcursor->get(&datKey, &datValue, DB_NEXT); + SafeDbt datKey(DB_DBT_MALLOC); + SafeDbt datValue(DB_DBT_MALLOC); + int ret = pcursor->get(datKey, datValue, DB_NEXT); if (ret != 0) return ret; else if (datKey.get_data() == nullptr || datValue.get_data() == nullptr) @@ -335,16 +333,9 @@ public: ssValue.SetType(SER_DISK); ssValue.clear(); ssValue.write((char*)datValue.get_data(), datValue.get_size()); - - // Clear and free memory - memory_cleanse(datKey.get_data(), datKey.get_size()); - memory_cleanse(datValue.get_data(), datValue.get_size()); - free(datKey.get_data()); - free(datValue.get_data()); return 0; } -public: bool TxnBegin() { if (!pdb || activeTxn) From 4a86a0acd9ac3ca392f0584a5fd079a856e5e4ba Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Sat, 24 Nov 2018 20:49:08 -0600 Subject: [PATCH 3/3] 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. --- src/wallet/db.cpp | 6 +++--- src/wallet/db.h | 16 +++++++++------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 9bac3c49ccf..b3ae3b92abe 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -247,12 +247,12 @@ BerkeleyEnvironment::VerifyResult BerkeleyEnvironment::Verify(const std::string& return (fRecovered ? VerifyResult::RECOVER_OK : VerifyResult::RECOVER_FAIL); } -BerkeleyBatch::SafeDbt::SafeDbt(u_int32_t flags) +BerkeleyBatch::SafeDbt::SafeDbt() { - m_dbt.set_flags(flags); + m_dbt.set_flags(DB_DBT_MALLOC); } -BerkeleyBatch::SafeDbt::SafeDbt(void *data, size_t size) +BerkeleyBatch::SafeDbt::SafeDbt(void* data, size_t size) : m_dbt(data, size) { } diff --git a/src/wallet/db.h b/src/wallet/db.h index 73cbff98fbc..25e1cdafa55 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -170,13 +170,15 @@ private: class BerkeleyBatch { /** RAII class that automatically cleanses its data on destruction */ - class SafeDbt final { + class SafeDbt final + { Dbt m_dbt; public: - // construct Dbt with data or flags - SafeDbt(u_int32_t flags = 0); - SafeDbt(void *data, size_t size); + // construct Dbt with internally-managed data + SafeDbt(); + // construct Dbt with provided data + SafeDbt(void* data, size_t size); ~SafeDbt(); // delegate to Dbt @@ -227,7 +229,7 @@ public: SafeDbt datKey(ssKey.data(), ssKey.size()); // Read - SafeDbt datValue(DB_DBT_MALLOC); + SafeDbt datValue; int ret = pdb->get(activeTxn, datKey, datValue, 0); bool success = false; if (datValue.get_data() != nullptr) { @@ -318,8 +320,8 @@ public: int ReadAtCursor(Dbc* pcursor, CDataStream& ssKey, CDataStream& ssValue) { // Read at cursor - SafeDbt datKey(DB_DBT_MALLOC); - SafeDbt datValue(DB_DBT_MALLOC); + SafeDbt datKey; + SafeDbt datValue; int ret = pcursor->get(datKey, datValue, DB_NEXT); if (ret != 0) return ret;