util: Add Clang thread safety annotations for variables guarded by cs

practicalswift commented at 9:32 am on April 30, 2018: contributor

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

fanquake added the label Utils/log/libs on Apr 30, 2018

practicalswift force-pushed on May 12, 2018

practicalswift commented at 8:31 pm on May 12, 2018: contributor

Now includes a fix for a missing cs_args lock introduced in the recently merged PR #10267.

practicalswift force-pushed on May 14, 2018

MarcoFalke added the label Needs rebase on Jun 6, 2018

practicalswift force-pushed on Jun 6, 2018

practicalswift commented at 6:07 am on June 6, 2018: contributor

Rebased!

MarcoFalke removed the label Needs rebase on Jun 6, 2018

DrahtBot closed this on Jul 29, 2018

DrahtBot commented at 3:16 pm on July 29, 2018: member

DrahtBot reopened this on Jul 29, 2018

in src/util.cpp:891 in 7f1cc8fd2e outdated

849@@ -848,7 +850,12 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
850         }
851         // if there is an -includeconf in the override args, but it is empty, that means the user
852         // passed '-noincludeconf' on the command line, in which case we should not include anything
853-        if (m_override_args.count("-includeconf") == 0) {
854+        bool emptyIncludeConf;
855+        {
856+            LOCK(cs_args);

MarcoFalke commented at 3:32 pm on July 29, 2018:

ReadConfigFiles is done once on start. Couldn’t you just take the lock at the beginning of the method and remove those scoped locks further down in the method?

practicalswift commented at 8:16 pm on August 29, 2018:

This change caused the deadlock. Now reverted to previous version :-)

in src/util.cpp:391 in 7f1cc8fd2e outdated

387@@ -387,6 +388,7 @@ void ArgsManager::WarnForSectionOnlyArgs()
388     // if it's okay to use the default section for this network, don't worry
389     if (m_network == CBaseChainParams::MAIN) return;
390 
391+    LOCK(cs_args);

MarcoFalke commented at 3:34 pm on July 29, 2018:

Just take the lock in the very first line of this method, or is there a specific reason that m_network shouldn’t be protected?

practicalswift force-pushed on Aug 27, 2018

practicalswift commented at 4:52 pm on August 27, 2018: contributor

@MarcoFalke Thanks for reviewing! Feedback addressed. Please re-review :-)

practicalswift commented at 5:09 pm on August 27, 2018: contributor

Added GUARDED_BY(cs_args) to m_available_args and m_network_only_args too :-)

in src/util.cpp:307 in d0d69b1766 outdated

303@@ -304,6 +304,7 @@ class ArgsManagerHelper {
304     static inline bool GetNetBoolArg(const ArgsManager &am, const std::string& net_arg)
305     {
306         std::pair<bool,std::string> found_result(false,std::string());
307+        LOCK(am.cs_args);

MarcoFalke commented at 6:51 pm on August 27, 2018:

I’d prefer to annotate this function and then take the lock where GetNetBoolArg is called (in GetChainName)

in src/util.cpp:894 in d0d69b1766 outdated

890@@ -886,7 +891,8 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
891         }
892         // if there is an -includeconf in the override args, but it is empty, that means the user
893         // passed '-noincludeconf' on the command line, in which case we should not include anything
894-        if (m_override_args.count("-includeconf") == 0) {
895+        bool emptyIncludeConf = m_override_args.count("-includeconf") == 0;
896+        if (emptyIncludeConf) {

MarcoFalke commented at 6:52 pm on August 27, 2018:

Why this change?

practicalswift commented at 9:21 pm on August 27, 2018:

Oh, that was likely a left-over from a previous commit - a separate variable was needed to limit the scope of the lock. When the lock was moved higher up that was no longer needed but the variable was accidentally left around.

MarcoFalke commented at 6:52 pm on August 27, 2018: member

utACK, but could squash into two commits (1) add missing locks and (2) add lock annotations

practicalswift force-pushed on Aug 27, 2018

practicalswift commented at 9:23 pm on August 27, 2018: contributor

@MarcoFalke Good points. PR adjusted accordingly. Please re-review :-)

MarcoFalke commented at 6:50 pm on August 29, 2018: member

0POTENTIAL DEADLOCK DETECTED
1Previous lock order was:
2 (1)cs_args  util.cpp:874 (2)csPathCached  util.cpp:767
3Current lock order is:
4 (2)csPathCached  util.cpp:767 (1)cs_args  util.cpp:508

practicalswift commented at 6:56 pm on August 29, 2018: contributor

@MarcoFalke Thanks! I’ll investigate immediately!

Add missing locks (cs_args) db5e9d3c88

practicalswift force-pushed on Aug 29, 2018

MarcoFalke commented at 7:27 pm on August 29, 2018: member

Note that I reproduced this with --enable-debug in ./configure

practicalswift commented at 7:30 pm on August 29, 2018: contributor

@MarcoFalke Strange! I always build with --enable-debug. Triggered by make check?

MarcoFalke commented at 7:36 pm on August 29, 2018: member

No, triggered by bitcoind or bitcoin-qt. It happens early on when the args and data dir are read.

practicalswift force-pushed on Aug 29, 2018

in src/util.cpp:876 in 8c747a4753 outdated

870@@ -871,8 +871,10 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, std::string& error, boo
871 
872 bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
873 {
874-    LOCK(cs_args);
875-    m_config_args.clear();
876+    {
877+        LOCK(cs_args);
878+        m_config_args.clear();

MarcoFalke commented at 8:20 pm on August 29, 2018:

Couldn’t you just move those two lines after the GetConfigFile(); call? (And remove all scopes and re-locks?)

practicalswift force-pushed on Aug 29, 2018

Add lock annotations (cs_args) d58dc9f943

practicalswift force-pushed on Aug 29, 2018

practicalswift commented at 8:40 pm on August 29, 2018: contributor

@MarcoFalke Please re-review :-)

MarcoFalke commented at 9:23 pm on August 29, 2018: member

This still fails travis for the same reason. Sorry, I guess my suggestion doesn’t work.

Fix potential deadlock 1e29379d69

practicalswift force-pushed on Aug 30, 2018

practicalswift commented at 8:17 am on August 30, 2018: contributor

@MarcoFalke Reverted again :-) Travis is now happy. Please re-review!

MarcoFalke merged this on Aug 30, 2018

MarcoFalke closed this on Aug 30, 2018

MarcoFalke referenced this in commit 6c7cfc8da6 on Aug 30, 2018

deadalnix referenced this in commit fd82b9f9f3 on Jan 17, 2020

jonspock referenced this in commit b33287fdc6 on Oct 7, 2020

jonspock referenced this in commit 6c76265513 on Oct 10, 2020

practicalswift deleted the branch on Apr 10, 2021

Munkybooty referenced this in commit 55a48b372f on Jun 30, 2021

Munkybooty referenced this in commit fe90bf9e6a on Jul 2, 2021

Munkybooty referenced this in commit 6268f42ce1 on Jul 2, 2021

gades referenced this in commit 4ae6d69d1f on May 22, 2022

DrahtBot locked this on Aug 18, 2022

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