Merge #13126: util: Add Clang thread safety annotations for variables guarded by cs_args

1e29379d69 Fix potential deadlock (practicalswift)
d58dc9f943 Add lock annotations (cs_args) (practicalswift)
db5e9d3c88 Add missing locks (cs_args) (practicalswift)

Pull request description:

  * Add missing `cs_args` locks
  * Add Clang thread safety annotations for variables guarded by `cs_args`

Tree-SHA512: bc562dbddf24a287bcf9bf902bc930f942f260a94e5c8ec4d190f7f2ddac448ed3d52acadaf9fc1c81a5cbff2c171c52c18ba0804eeb03f699d70394e1c977c5

2 files changed, 24 insertions, 9 deletions

diff --git a/src/util.cpp b/src/util.cpp
index 55b09dcff8..3bb52e9b3d 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<bool,std::string> found_result(false,std::string());
         found_result = GetArgHelper(am.m_override_args, net_arg, true);
@@ -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<std::string, Arg>& 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:
@@ -880,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<std::string> includeconf(GetArgs("-includeconf"));
             {
@@ -940,6 +951,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 e93489c1ed..7bf9fdbe12 100644
--- a/src/util.h
+++ b/src/util.h
@@ -142,11 +142,11 @@ protected:
     };
 
     mutable CCriticalSection cs_args;
-    std::map<std::string, std::vector<std::string>> m_override_args;
-    std::map<std::string, std::vector<std::string>> m_config_args;
-    std::string m_network;
-    std::set<std::string> m_network_only_args;
-    std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args;
+    std::map<std::string, std::vector<std::string>> m_override_args GUARDED_BY(cs_args);
+    std::map<std::string, std::vector<std::string>> m_config_args GUARDED_BY(cs_args);
+    std::string m_network GUARDED_BY(cs_args);
+    std::set<std::string> m_network_only_args GUARDED_BY(cs_args);
+    std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args GUARDED_BY(cs_args);
 
     bool ReadConfigStream(std::istream& stream, std::string& error, bool ignore_invalid_keys = false);
 
@@ -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