From 05ab316853877f0ace9dfbe52fe410d930d4d891 Mon Sep 17 00:00:00 2001 From: jeffro256 Date: Tue, 8 Apr 2025 01:46:46 -0500 Subject: [PATCH] epee: better scope leaver 1. Doesn't allocate a `boost::shared_ptr` 2. Doesn't use a vtable 3. Doesn't use `std::function` wrapper 4. Doesn't necessarily copy functional object 5. Doesn't need a factory function for simple declaration (yay CTAD) 6. Isolated to its own header 7. Name doesn't suck as much --- contrib/epee/include/misc_language.h | 38 +++--- contrib/epee/include/scope_guard.h | 99 +++++++++++++++ .../blockchain_blackball.cpp | 7 +- .../cryptonote_protocol_handler.inl | 5 +- src/wallet/wallet2.cpp | 8 +- tests/unit_tests/epee_utils.cpp | 118 +++++++++++++++++- 6 files changed, 241 insertions(+), 34 deletions(-) create mode 100644 contrib/epee/include/scope_guard.h diff --git a/contrib/epee/include/misc_language.h b/contrib/epee/include/misc_language.h index 4abc2f982..8a8713a1f 100644 --- a/contrib/epee/include/misc_language.h +++ b/contrib/epee/include/misc_language.h @@ -29,8 +29,14 @@ #pragma once #include -#include + +#include +#include +#include #include + +#include "scope_guard.h" + namespace epee { #define AUTO_VAL_INIT(v) boost::value_initialized() @@ -72,32 +78,16 @@ namespace misc_utils /* */ /************************************************************************/ - struct call_befor_die_base - { - virtual ~call_befor_die_base() = default; - }; - - typedef std::shared_ptr auto_scope_leave_caller; - + /// name exists for backwards compatibility. if writing new code, you shouldn't + /// use this type unless you *need* the scope leaver to be polymorphic because the std::function + /// adds unnecessary overhead if you know the type of the functional object at compile-time + using auto_scope_leave_caller = scope_guard>; + // name exists for backwards compatibility template - struct call_befor_die: public call_befor_die_base + auto_scope_leave_caller create_scope_leave_handler(t_scope_leave_handler &&f) { - t_scope_leave_handler m_func; - call_befor_die(t_scope_leave_handler f):m_func(f) - {} - ~call_befor_die() - { - try { m_func(); } - catch (...) { /* ignore */ } - } - }; - - template - auto_scope_leave_caller create_scope_leave_handler(t_scope_leave_handler f) - { - auto_scope_leave_caller slc = std::make_shared>(f); - return slc; + return auto_scope_leave_caller(std::forward(f)); } template struct struct_init: T diff --git a/contrib/epee/include/scope_guard.h b/contrib/epee/include/scope_guard.h new file mode 100644 index 000000000..a15df5527 --- /dev/null +++ b/contrib/epee/include/scope_guard.h @@ -0,0 +1,99 @@ +// Copyright (c) 2025, The Monero Project +// +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without modification, are +// permitted provided that the following conditions are met: +// +// 1. Redistributions of source code must retain the above copyright notice, this list of +// conditions and the following disclaimer. +// +// 2. Redistributions in binary form must reproduce the above copyright notice, this list +// of conditions and the following disclaimer in the documentation and/or other +// materials provided with the distribution. +// +// 3. Neither the name of the copyright holder nor the names of its contributors may be +// used to endorse or promote products derived from this software without specific +// prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY +// EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF +// MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL +// THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, +// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS +// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, +// STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF +// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +#pragma once + +#include + +namespace epee +{ + +/** + * @brief: scope_guard - RAII pattern to call action on scope leave + */ +template +class scope_guard final +{ +public: + // no copy + scope_guard(const scope_guard&) = delete; + scope_guard& operator=(const scope_guard&) = delete; + + // constructor with CTAD + scope_guard(F &&f): m_f(std::forward(f)) {} + + // call action + ~scope_guard() noexcept { try { m_f(); } catch (...) { /* ignore exception in destructor */ } } + +private: + F m_f; +}; + +/** + * @brief: unique_scope_guard - RAII pattern to call action on scope leave and/or at explicit call + */ +template +class unique_scope_guard final +{ +public: + // no copy + unique_scope_guard(const unique_scope_guard&) = delete; + unique_scope_guard& operator=(const unique_scope_guard&) = delete; + + // move resets current state and releases other instance + unique_scope_guard(unique_scope_guard &&other): m_f(std::move(other.m_f)), m_do(other.m_do) + { + other.release(); + } + unique_scope_guard& operator=(unique_scope_guard &&other) + { + if (this != &other) + { + reset(); + m_f = std::move(other.m_f); + m_do = other.m_do; + other.release(); + } + return *this; + } + + // constructor with CTAD + unique_scope_guard(F &&f): m_f(std::forward(f)), m_do(true) {} + + void reset() { if (m_do) { m_do = false; m_f(); } } + void release() { m_do = false; } + + // call action + ~unique_scope_guard() noexcept { try { reset(); } catch (...) { /* ignore exception in destructor */ } } + +private: + F m_f; + bool m_do; +}; + +} //namespace epee diff --git a/src/blockchain_utilities/blockchain_blackball.cpp b/src/blockchain_utilities/blockchain_blackball.cpp index cf41134d1..38c470662 100644 --- a/src/blockchain_utilities/blockchain_blackball.cpp +++ b/src/blockchain_utilities/blockchain_blackball.cpp @@ -528,7 +528,11 @@ static uint64_t find_first_diverging_transaction(const std::string &first_filena MDB_val k; MDB_val v[2]; - epee::misc_utils::auto_scope_leave_caller txn_dtor[2]; + epee::scope_guard txn_dtor([&tx_active, &txn](){ + for (int i = 0; i < 2; ++i) + if (tx_active[i]) + mdb_txn_abort(txn[i]); + }); for (int i = 0; i < 2; ++i) { dbr = mdb_env_create(&env[i]); @@ -542,7 +546,6 @@ static uint64_t find_first_diverging_transaction(const std::string &first_filena dbr = mdb_txn_begin(env[i], NULL, MDB_RDONLY, &txn[i]); if (dbr) throw std::runtime_error("Failed to create LMDB transaction: " + std::string(mdb_strerror(dbr))); - txn_dtor[i] = epee::misc_utils::create_scope_leave_handler([&, i](){if (tx_active[i]) mdb_txn_abort(txn[i]);}); tx_active[i] = true; dbr = mdb_dbi_open(txn[i], "txs_pruned", MDB_INTEGERKEY, &dbi[i]); diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl index ae8db1ef4..339475192 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl @@ -49,6 +49,7 @@ #include "common/pruning.h" #include "common/util.h" #include "misc_log_ex.h" +#include "scope_guard.h" #undef MONERO_DEFAULT_LOG_CATEGORY #define MONERO_DEFAULT_LOG_CATEGORY "net.cn" @@ -1494,7 +1495,7 @@ namespace cryptonote } bool stopped = false; - auto cleanup_on_exit = epee::misc_utils::create_scope_leave_handler([this, &stopped, &context, span_connection_id, start_height]() { + epee::unique_scope_guard cleanup_on_exit = [this, &stopped, &context, span_connection_id, start_height]() { if (!m_core.cleanup_handle_incoming_blocks()) { LOG_PRINT_CCONTEXT_L0("Failure in cleanup_handle_incoming_blocks"); @@ -1505,7 +1506,7 @@ namespace cryptonote return; m_block_queue.remove_spans(span_connection_id, start_height); - }); + }; uint64_t block_process_time_full = 0, transactions_process_time_full = 0; size_t num_txs = 0, blockidx = 0; diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 2746a5b5d..ea2982cbd 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -13993,11 +13993,11 @@ void wallet2::stop_background_sync(const epee::wipeable_string &wallet_password, // Set the plaintext spend key m_account.set_spend_key(recovered_spend_key); + if (is_key_encryption_enabled()) + this->encrypt_keys(wallet_password); - // Encrypt the spend key when done if needed - epee::misc_utils::auto_scope_leave_caller keys_reencryptor; - if (m_ask_password == AskPasswordToDecrypt && !m_unattended && !m_watch_only) - keys_reencryptor = epee::misc_utils::create_scope_leave_handler([&, this]{encrypt_keys(wallet_password);}); + // Decrypt now, then re-encrypt the spend key when done if needed + wallet_keys_unlocker unlocker(*this, &wallet_password); // Now we can use the decrypted spend key to process background cache process_background_cache(background_sync_data, background_synced_chain, last_block_reward); diff --git a/tests/unit_tests/epee_utils.cpp b/tests/unit_tests/epee_utils.cpp index 3363eb4a1..bc0c1a687 100644 --- a/tests/unit_tests/epee_utils.cpp +++ b/tests/unit_tests/epee_utils.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -54,6 +55,7 @@ #include "net/local_ip.h" #include "net/buffer.h" #include "p2p/net_peerlist_boost_serialization.h" +#include "scope_guard.h" #include "span.h" #include "string_tools.h" #include "storages/parserse_base_utils.h" @@ -72,7 +74,7 @@ namespace unsigned(std::is_assignable()); EXPECT_TRUE(count == 6 || count == 0) << "Mismatch on construction results - " << count << " were true"; - return count == 6; + return count == 6; } // This is probably stressing the compiler more than the implementation ... @@ -277,7 +279,7 @@ TEST(Span, Nullptr) EXPECT_EQ(0, data.size_bytes()); }; check_empty({}); - check_empty(nullptr); + check_empty(nullptr); } TEST(Span, Writing) @@ -1924,3 +1926,115 @@ TEST(parsing, strtoul) EXPECT_EQ(ERANGE, errno); EXPECT_EQ(ULLONG_MAX, ul); } + +TEST(scope_guard, scope_guard) +{ + int count = 0; + { + ASSERT_EQ(0, count); + const epee::scope_guard g([&count](){++count;}); + ASSERT_EQ(0, count); + } + ASSERT_EQ(1, count); +} + +TEST(scope_guard, scope_guard_error) +{ + int count = 0; + { + ASSERT_EQ(0, count); + const epee::scope_guard g([&count](){++count; throw std::logic_error( + "! Error" + "If you're seeing this, the code is in what I thought was an unreachable state." + "I could give you advice for what to do. But honestly, why should you trust me? I clearly screwed this up." + "I'm writing a message that should never appear, yet I know it will probably appear someday." + "On a deep level, I know I'm not up to this task. I'm so sorry.");}); + ASSERT_EQ(0, count); + } // hopefully no exception in destructor... + ASSERT_EQ(1, count); +} + +TEST(scope_guard, unique_scope_guard_basic) +{ + int count = 0; + { + ASSERT_EQ(0, count); + const epee::unique_scope_guard g([&count](){++count;}); + ASSERT_EQ(0, count); + } + ASSERT_EQ(1, count); +} + +TEST(scope_guard, unique_scope_guard_error) +{ + int count = 0; + { + ASSERT_EQ(0, count); + const epee::unique_scope_guard g([&count](){++count; throw std::logic_error( + "Never write error messages tired.");}); + ASSERT_EQ(0, count); + } // hopefully no exception in destructor... + ASSERT_EQ(1, count); +} + +TEST(scope_guard, unique_scope_guard_reset) +{ + int count = 0; + { + ASSERT_EQ(0, count); + epee::unique_scope_guard g([&count](){++count;}); + ASSERT_EQ(0, count); + g.reset(); + ASSERT_EQ(1, count); + } + ASSERT_EQ(1, count); +} + +TEST(scope_guard, unique_scope_guard_release) +{ + int count = 0; + { + ASSERT_EQ(0, count); + epee::unique_scope_guard g([&count](){++count;}); + ASSERT_EQ(0, count); + g.release(); + ASSERT_EQ(0, count); + } + ASSERT_EQ(0, count); +} + +TEST(scope_guard, unique_scope_guard_move_construct) +{ + int count = 0; + { + ASSERT_EQ(0, count); + epee::unique_scope_guard g([&count](){++count;}); + ASSERT_EQ(0, count); + epee::unique_scope_guard h(std::move(g)); + ASSERT_EQ(0, count); + g.reset(); + ASSERT_EQ(0, count); + } + ASSERT_EQ(1, count); +} + +TEST(scope_guard, unique_scope_guard_move_assign) +{ + int count1 = 0; + int count2 = 0; + { + ASSERT_EQ(0, count1); + ASSERT_EQ(0, count2); + epee::unique_scope_guard g(std::function([&count1](){++count1;})); + ASSERT_EQ(0, count1); + ASSERT_EQ(0, count2); + epee::unique_scope_guard h(std::function([&count2](){++count2;})); + ASSERT_EQ(0, count1); + ASSERT_EQ(0, count2); + h = std::move(g); + ASSERT_EQ(0, count1); + ASSERT_EQ(1, count2); + } + ASSERT_EQ(1, count1); + ASSERT_EQ(1, count2); +}