From 1a9f9f7e5e2e73fb832f5b96ad7e9e57954f3f3c Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Wed, 19 Sep 2018 02:38:40 -0400 Subject: [PATCH] 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 a7bf89c572..9bac3c49cc 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 15577e66f1..73cbff98fb 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -166,10 +166,27 @@ class BerkeleyDatabase 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 @@ class BerkeleyBatch /* 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 @@ class BerkeleyBatch 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 @@ class BerkeleyBatch } 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 @@ class BerkeleyBatch 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 @@ class BerkeleyBatch 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 @@ class BerkeleyBatch 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 @@ class BerkeleyBatch 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 @@ class BerkeleyBatch 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)