From 1fce19b3e92bc4077ca9896f5ed51aa48071ae83 Mon Sep 17 00:00:00 2001 From: Colin LeMahieu Date: Sun, 27 Oct 2024 11:50:03 +0000 Subject: [PATCH 1/2] Fixing leak where cursor would not be closed before being overwritten. --- nano/store/lmdb/iterator.cpp | 21 +++++++-------------- nano/store/lmdb/iterator.hpp | 4 +--- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/nano/store/lmdb/iterator.cpp b/nano/store/lmdb/iterator.cpp index 76cfc35e5b..5fc87d1d09 100644 --- a/nano/store/lmdb/iterator.cpp +++ b/nano/store/lmdb/iterator.cpp @@ -21,7 +21,7 @@ void iterator::update (int status) if (status == MDB_SUCCESS) { value_type init; - auto status = mdb_cursor_get (cursor, &init.first, &init.second, MDB_GET_CURRENT); + auto status = mdb_cursor_get (cursor.get (), &init.first, &init.second, MDB_GET_CURRENT); release_assert (status == MDB_SUCCESS); current = init; } @@ -33,8 +33,10 @@ void iterator::update (int status) iterator::iterator (MDB_txn * tx, MDB_dbi dbi) noexcept { + MDB_cursor * cursor; auto open_status = mdb_cursor_open (tx, dbi, &cursor); release_assert (open_status == MDB_SUCCESS); + this->cursor.reset (cursor); this->current = std::monostate{}; } @@ -53,7 +55,7 @@ auto iterator::end (MDB_txn * tx, MDB_dbi dbi) -> iterator auto iterator::lower_bound (MDB_txn * tx, MDB_dbi dbi, MDB_val const & lower_bound) -> iterator { iterator result{ tx, dbi }; - auto status = mdb_cursor_get (result.cursor, const_cast (&lower_bound), nullptr, MDB_SET_RANGE); + auto status = mdb_cursor_get (result.cursor.get (), const_cast (&lower_bound), nullptr, MDB_SET_RANGE); result.update (status); return std::move (result); } @@ -63,18 +65,9 @@ iterator::iterator (iterator && other) noexcept *this = std::move (other); } -iterator::~iterator () -{ - if (cursor) - { - mdb_cursor_close (cursor); - } -} - auto iterator::operator= (iterator && other) noexcept -> iterator & { - cursor = other.cursor; - other.cursor = nullptr; + cursor = std::move (other.cursor); current = other.current; other.current = std::monostate{}; return *this; @@ -83,7 +76,7 @@ auto iterator::operator= (iterator && other) noexcept -> iterator & auto iterator::operator++ () -> iterator & { auto operation = is_end () ? MDB_FIRST : MDB_NEXT; - auto status = mdb_cursor_get (cursor, nullptr, nullptr, operation); + auto status = mdb_cursor_get (cursor.get (), nullptr, nullptr, operation); release_assert (status == MDB_SUCCESS || status == MDB_NOTFOUND); update (status); return *this; @@ -92,7 +85,7 @@ auto iterator::operator++ () -> iterator & auto iterator::operator-- () -> iterator & { auto operation = is_end () ? MDB_LAST : MDB_PREV; - auto status = mdb_cursor_get (cursor, nullptr, nullptr, operation); + auto status = mdb_cursor_get (cursor.get (), nullptr, nullptr, operation); release_assert (status == MDB_SUCCESS || status == MDB_NOTFOUND); update (status); return *this; diff --git a/nano/store/lmdb/iterator.hpp b/nano/store/lmdb/iterator.hpp index ff921ca3aa..da2280adf3 100644 --- a/nano/store/lmdb/iterator.hpp +++ b/nano/store/lmdb/iterator.hpp @@ -22,7 +22,7 @@ namespace nano::store::lmdb */ class iterator { - MDB_cursor * cursor{ nullptr }; + std::unique_ptr cursor{ nullptr, mdb_cursor_close }; std::variant> current; void update (int status); iterator (MDB_txn * tx, MDB_dbi dbi) noexcept; @@ -39,8 +39,6 @@ class iterator static auto end (MDB_txn * tx, MDB_dbi dbi) -> iterator; static auto lower_bound (MDB_txn * tx, MDB_dbi dbi, MDB_val const & lower_bound) -> iterator; - ~iterator (); - iterator (iterator const &) = delete; auto operator= (iterator const &) -> iterator & = delete; From 5ca6ff70dcdc0ede4eb054f7e7f1dde366e0bf53 Mon Sep 17 00:00:00 2001 From: Colin LeMahieu Date: Sun, 27 Oct 2024 14:05:06 +0000 Subject: [PATCH 2/2] Making use of unique_ptr to ensure environment handle never leaks. --- nano/store/lmdb/lmdb.cpp | 8 +++----- nano/store/lmdb/lmdb_env.cpp | 9 ++++----- nano/store/lmdb/lmdb_env.hpp | 2 +- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/nano/store/lmdb/lmdb.cpp b/nano/store/lmdb/lmdb.cpp index a16d53cb88..07bbc34ded 100644 --- a/nano/store/lmdb/lmdb.cpp +++ b/nano/store/lmdb/lmdb.cpp @@ -121,9 +121,7 @@ bool nano::store::lmdb::component::vacuum_after_upgrade (std::filesystem::path c if (vacuum_success) { // Need to close the database to release the file handle - mdb_env_sync (env.environment, true); - mdb_env_close (env.environment); - env.environment = nullptr; + mdb_env_sync (env, true); // Replace the ledger file with the vacuumed one std::filesystem::rename (vacuum_path, path_a); @@ -155,7 +153,7 @@ void nano::store::lmdb::component::serialize_mdb_tracker (boost::property_tree:: void nano::store::lmdb::component::serialize_memory_stats (boost::property_tree::ptree & json) { MDB_stat stats; - auto status (mdb_env_stat (env.environment, &stats)); + auto status (mdb_env_stat (env, &stats)); release_assert (status == 0); json.put ("branch_pages", stats.ms_branch_pages); json.put ("depth", stats.ms_depth); @@ -448,7 +446,7 @@ std::string nano::store::lmdb::component::error_string (int status) const bool nano::store::lmdb::component::copy_db (std::filesystem::path const & destination_file) { - return !mdb_env_copy2 (env.environment, destination_file.string ().c_str (), MDB_CP_COMPACT); + return !mdb_env_copy2 (env, destination_file.string ().c_str (), MDB_CP_COMPACT); } void nano::store::lmdb::component::rebuild_db (store::write_transaction const & transaction_a) diff --git a/nano/store/lmdb/lmdb_env.cpp b/nano/store/lmdb/lmdb_env.cpp index cd1897c80a..b30718960f 100644 --- a/nano/store/lmdb/lmdb_env.cpp +++ b/nano/store/lmdb/lmdb_env.cpp @@ -19,8 +19,10 @@ void nano::store::lmdb::env::init (bool & error_a, std::filesystem::path const & nano::set_secure_perm_directory (path_a.parent_path (), error_chmod); if (!error_mkdir) { + MDB_env * environment; auto status1 (mdb_env_create (&environment)); release_assert (status1 == 0); + this->environment.reset (environment); auto status2 (mdb_env_set_maxdbs (environment, options_a.config.max_databases)); release_assert (status2 == 0); auto map_size = options_a.config.map_size; @@ -66,13 +68,11 @@ void nano::store::lmdb::env::init (bool & error_a, std::filesystem::path const & else { error_a = true; - environment = nullptr; } } else { error_a = true; - environment = nullptr; } } @@ -81,14 +81,13 @@ nano::store::lmdb::env::~env () if (environment != nullptr) { // Make sure the commits are flushed. This is a no-op unless MDB_NOSYNC is used. - mdb_env_sync (environment, true); - mdb_env_close (environment); + mdb_env_sync (environment.get (), true); } } nano::store::lmdb::env::operator MDB_env * () const { - return environment; + return environment.get (); } nano::store::read_transaction nano::store::lmdb::env::tx_begin_read (store::lmdb::txn_callbacks mdb_txn_callbacks) const diff --git a/nano/store/lmdb/lmdb_env.hpp b/nano/store/lmdb/lmdb_env.hpp index e912653250..07e89ffd2e 100644 --- a/nano/store/lmdb/lmdb_env.hpp +++ b/nano/store/lmdb/lmdb_env.hpp @@ -62,7 +62,7 @@ class env final store::read_transaction tx_begin_read (txn_callbacks callbacks = txn_callbacks{}) const; store::write_transaction tx_begin_write (txn_callbacks callbacks = txn_callbacks{}) const; MDB_txn * tx (store::transaction const & transaction_a) const; - MDB_env * environment; + std::unique_ptr environment{ nullptr, mdb_env_close }; nano::id_t const store_id{ nano::next_id () }; }; } // namespace nano::store::lmdb