From db5e9d3c88349f7e3b56f50f2e2862997e308fd9 Mon Sep 17 00:00:00 2001 From: practicalswift Date: Mon, 27 Aug 2018 23:19:18 +0200 Subject: [PATCH 1/3] Add missing locks (cs_args) --- src/util.cpp | 20 +++++++++++--------- src/util.h | 5 ++++- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/util.cpp b/src/util.cpp index 55b09dcff8e..ef5ea32ed12 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -372,6 +372,8 @@ ArgsManager::ArgsManager() : void ArgsManager::WarnForSectionOnlyArgs() { + LOCK(cs_args); + // if there's no section selected, don't worry if (m_network.empty()) return; @@ -400,6 +402,7 @@ void ArgsManager::WarnForSectionOnlyArgs() void ArgsManager::SelectConfigNetwork(const std::string& network) { + LOCK(cs_args); m_network = network; } @@ -468,6 +471,7 @@ bool ArgsManager::IsArgKnown(const std::string& key) const arg_no_net = std::string("-") + key.substr(option_index + 1, std::string::npos); } + LOCK(cs_args); for (const auto& arg_map : m_available_args) { if (arg_map.second.count(arg_no_net)) return true; } @@ -571,6 +575,7 @@ void ArgsManager::AddArg(const std::string& name, const std::string& help, const eq_index = name.size(); } + LOCK(cs_args); std::map& arg_map = m_available_args[cat]; auto ret = arg_map.emplace(name.substr(0, eq_index), Arg(name.substr(eq_index, name.size() - eq_index), help, debug_only)); assert(ret.second); // Make sure an insertion actually happened @@ -588,6 +593,7 @@ std::string ArgsManager::GetHelpMessage() const const bool show_debug = gArgs.GetBoolArg("-help-debug", false); std::string usage = ""; + LOCK(cs_args); for (const auto& arg_map : m_available_args) { switch(arg_map.first) { case OptionsCategory::OPTIONS: @@ -865,10 +871,8 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, std::string& error, boo bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) { - { - LOCK(cs_args); - m_config_args.clear(); - } + LOCK(cs_args); + m_config_args.clear(); const std::string confPath = GetArg("-conf", BITCOIN_CONF_FILENAME); fs::ifstream stream(GetConfigFile(confPath)); @@ -892,11 +896,8 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) // Remove -includeconf from configuration, so we can warn about recursion // later - { - LOCK(cs_args); - m_config_args.erase("-includeconf"); - m_config_args.erase(std::string("-") + chain_id + ".includeconf"); - } + m_config_args.erase("-includeconf"); + m_config_args.erase(std::string("-") + chain_id + ".includeconf"); for (const std::string& to_include : includeconf) { fs::ifstream include_config(GetConfigFile(to_include)); @@ -940,6 +941,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) std::string ArgsManager::GetChainName() const { + LOCK(cs_args); bool fRegTest = ArgsManagerHelper::GetNetBoolArg(*this, "-regtest"); bool fTestNet = ArgsManagerHelper::GetNetBoolArg(*this, "-testnet"); diff --git a/src/util.h b/src/util.h index e93489c1ed9..e27e73089ed 100644 --- a/src/util.h +++ b/src/util.h @@ -262,7 +262,10 @@ public: /** * Clear available arguments */ - void ClearArgs() { m_available_args.clear(); } + void ClearArgs() { + LOCK(cs_args); + m_available_args.clear(); + } /** * Get the help string From d58dc9f94365ba3f993594bab293915d79dbe117 Mon Sep 17 00:00:00 2001 From: practicalswift Date: Wed, 29 Aug 2018 22:33:33 +0200 Subject: [PATCH 2/3] Add lock annotations (cs_args) --- src/util.cpp | 4 ++-- src/util.h | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/util.cpp b/src/util.cpp index ef5ea32ed12..e58ff042e8a 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -216,7 +216,7 @@ public: /** Determine whether to use config settings in the default section, * See also comments around ArgsManager::ArgsManager() below. */ - static inline bool UseDefaultSection(const ArgsManager& am, const std::string& arg) + static inline bool UseDefaultSection(const ArgsManager& am, const std::string& arg) EXCLUSIVE_LOCKS_REQUIRED(am.cs_args) { return (am.m_network == CBaseChainParams::MAIN || am.m_network_only_args.count(arg) == 0); } @@ -295,7 +295,7 @@ public: /* Special test for -testnet and -regtest args, because we * don't want to be confused by craziness like "[regtest] testnet=1" */ - static inline bool GetNetBoolArg(const ArgsManager &am, const std::string& net_arg) + static inline bool GetNetBoolArg(const ArgsManager &am, const std::string& net_arg) EXCLUSIVE_LOCKS_REQUIRED(am.cs_args) { std::pair found_result(false,std::string()); found_result = GetArgHelper(am.m_override_args, net_arg, true); diff --git a/src/util.h b/src/util.h index e27e73089ed..7bf9fdbe12f 100644 --- a/src/util.h +++ b/src/util.h @@ -142,11 +142,11 @@ protected: }; mutable CCriticalSection cs_args; - std::map> m_override_args; - std::map> m_config_args; - std::string m_network; - std::set m_network_only_args; - std::map> m_available_args; + std::map> m_override_args GUARDED_BY(cs_args); + std::map> m_config_args GUARDED_BY(cs_args); + std::string m_network GUARDED_BY(cs_args); + std::set m_network_only_args GUARDED_BY(cs_args); + std::map> m_available_args GUARDED_BY(cs_args); bool ReadConfigStream(std::istream& stream, std::string& error, bool ignore_invalid_keys = false); From 1e29379d69a6db0820ae984733d1be98340e5ed0 Mon Sep 17 00:00:00 2001 From: practicalswift Date: Thu, 30 Aug 2018 10:02:49 +0200 Subject: [PATCH 3/3] Fix potential deadlock --- src/util.cpp | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/util.cpp b/src/util.cpp index e58ff042e8a..3bb52e9b3dd 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -871,8 +871,10 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, std::string& error, boo bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) { - LOCK(cs_args); - m_config_args.clear(); + { + LOCK(cs_args); + m_config_args.clear(); + } const std::string confPath = GetArg("-conf", BITCOIN_CONF_FILENAME); fs::ifstream stream(GetConfigFile(confPath)); @@ -884,7 +886,12 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) } // if there is an -includeconf in the override args, but it is empty, that means the user // passed '-noincludeconf' on the command line, in which case we should not include anything - if (m_override_args.count("-includeconf") == 0) { + bool emptyIncludeConf; + { + LOCK(cs_args); + emptyIncludeConf = m_override_args.count("-includeconf") == 0; + } + if (emptyIncludeConf) { std::string chain_id = GetChainName(); std::vector includeconf(GetArgs("-includeconf")); { @@ -896,8 +903,11 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) // Remove -includeconf from configuration, so we can warn about recursion // later - m_config_args.erase("-includeconf"); - m_config_args.erase(std::string("-") + chain_id + ".includeconf"); + { + LOCK(cs_args); + m_config_args.erase("-includeconf"); + m_config_args.erase(std::string("-") + chain_id + ".includeconf"); + } for (const std::string& to_include : includeconf) { fs::ifstream include_config(GetConfigFile(to_include));