Fix several potential issues found by sanitizers #9743

1. 
   sipa commented at 8:49 pm on February 11, 2017: member

   Notes:

   • The LevelDB patch here is simple, and makes it switch to c++11 atomics when available over their MemoryBarrier() based solutions. A slightly better version is submitted upstream as https://github.com/google/leveldb/pull/449.
   • Several race fixes are not bug fixes (the numThreads one and the access to globals one both happen before any multithreaded access to those variables takes place; and the LevelDB one is due to tsan not recognizing the inline assembly used in MemoryBarrier), but in general improvements that result in less fragile code.

   After these, I believe our unit and rpc tests are clean for asan/lsan/ubsan/tsan, but:

   • The BDB environment holds locks that cross call stacks, and trigger the tsan deadlock detection. To avoid, create a suppressions file with the line deadlock:__db_pthread_mutex_lock in it, and pass an environment variable TSAN_OPTIONS="suppressions=$file" to the binaries.
   • The zapwallettxn RPC test causes an assertion failure inside libtsan for me.
   • You need to compile with -DBOOST_SP_USE_STD_ATOMIC to avoid tsan incorrectly detecting signals2 calls as racy (boost uses inline assembly for atomic refcounting by default, unless this compile options is passed; it was introduced in 1.54 I think).

   As tsan seems to become a usable option for race detection, I wonder if over time we could remove our own DEBUG_LOCKORDER features.

2. fanquake added the label Refactoring on Feb 11, 2017
3. in src/httpserver.cpp: in a10eff9440 outdated

    92@@ -93,9 +93,10 @@ class WorkQueue
    93 
    94 public:
    95     WorkQueue(size_t _maxDepth) : running(true),
    96-                                 maxDepth(_maxDepth),
    97-                                 numThreads(0)
    98+                                 maxDepth(_maxDepth)
    99     {
   100+        std::lock_guard<std::mutex> lock(cs);
   

   ---

   ---

   

   theuni commented at 11:15 pm on February 11, 2017:

   I don’t understand this one. How could the ctor be racy?

   ---

   

   laanwj commented at 4:48 pm on February 20, 2017:

   This is really confusing. I don’t think we should be making this nonsensical change just to make a sanitizer tool happy.

   ---

   

   TheBlueMatt commented at 8:02 pm on February 20, 2017:

   I’d highly prefer we do - such tools are pretty critical to our ability to find issues - but should probably have a comment noting why we have this insanity.

   ---

   

   laanwj commented at 12:07 pm on February 22, 2017:

   The way forward would be to fix the tools, not contort our code around whatever happen to be the bugs in the analysis tools of the day.
4. 
   sipa commented at 11:18 pm on February 11, 2017: member

   It’s not (see PR description). I think tsan is confused because it’s accessed from another object.
5. in src/bitcoin-tx.cpp: in 78f203d401 outdated

   666@@ -667,11 +667,13 @@ static void MutateTx(CMutableTransaction& tx, const std::string& command,
   667         MutateTxDelOutput(tx, commandVal);
   668     else if (command == "outaddr")
   669         MutateTxAddOutAddr(tx, commandVal);
   670-    else if (command == "outpubkey")
   671+    else if (command == "outpubkey") {
   672+        if (!ecc) { ecc.reset(new Secp256k1Init()); }
   

   ---

   ---

   

   jonasschnelli commented at 2:06 pm on February 12, 2017:

   Hmm… how can ecc be a nullptr at this point?

   ---

   

   jonasschnelli commented at 8:13 pm on February 12, 2017:

   Sorry,.. missed the fact that std::unique_ptr<Secp256k1Init> ecc; initialises ecc with nullptr
6. 
   sipa commented at 6:37 pm on February 12, 2017: member

   @jonasschnelli Where would it be set to something else?
7. gmaxwell approved
8. 
   gmaxwell commented at 8:28 pm on February 13, 2017: contributor

   ACK
9. 
   sipa commented at 8:16 pm on February 20, 2017: member

   I’ll try to find something less insane that also doesn’t trip it.
10. 
    sipa commented at 3:39 am on March 5, 2017: member

    I’m unable to reproduce that tsan warning, so I just removed the commit.
11. 
    laanwj commented at 12:43 pm on March 5, 2017: member

    I’m unable to reproduce that tsan warning, so I just removed the commit.

    I think you forgot to push, it’s still there: https://github.com/bitcoin/bitcoin/pull/9743/commits/a10eff9440c0beb6393effdacf34f59e9ca68fda

12. sipa force-pushed on Mar 29, 2017
13. 
    sipa commented at 6:42 pm on March 29, 2017: member

    Rebased, and removed the LevelDB and the patches for spurious warning. The LevelDB patch should go through the leveldb repo instead.
14. in src/sync.cpp:74 in 9169cedc4f outdated

    70@@ -71,7 +71,7 @@ struct LockData {
    71     boost::mutex dd_mutex;
    72 } static lockdata;
    73 
    74-boost::thread_specific_ptr<LockStack> lockstack;
    75+boost::thread_specific_ptr<LockStack> lockstack([](LockStack* ptr) { delete ptr; });
    

    ---

    ---

    

    TheBlueMatt commented at 2:25 pm on April 17, 2017:

    I’m confused. The boost docs appear to indicate this should be identical.

    ---

    

    sipa commented at 1:21 pm on April 20, 2017:

    Agree, and tests seem to agree that they are indeed invoked. I’ll remove it.
15. in src/utiltime.cpp:31 in 9169cedc4f outdated

    27@@ -25,7 +28,7 @@ int64_t GetTime()
    28 
    29 void SetMockTime(int64_t nMockTimeIn)
    30 {
    31-    nMockTime = nMockTimeIn;
    32+    nMockTime.store(nMockTimeIn, std::memory_order_relaxed);
    

    ---

    ---

    

    TheBlueMatt commented at 2:27 pm on April 17, 2017:

    relaxed here just seems needlessly lossy - mocktime is for testing, it can be slow, and running a test with a race because we’re using relaxed everyhwere just seems needlessly confusing.

    ---

    

    sipa commented at 1:24 pm on April 20, 2017:

    It’s read by a relaxed read anyway, using a stronger serializing write won’t matter (if I understand the semantics correctly).

    ---

    

    TheBlueMatt commented at 1:36 pm on April 20, 2017:

    Yes, my suggestion was to change the read as well :)

    ---

    

    sipa commented at 1:38 pm on April 20, 2017:

    GetTime is called all over the place, no? I don’t want to introduce extra synchronization there.
16. fix ubsan: bitcoin-tx: not initialize context before IsFullyValid 321bbc2079
17. fix tsan: utiltime race on nMockTime 1d31093d4d
18. sipa force-pushed on Apr 20, 2017
19. 
    jtimon commented at 7:45 pm on April 24, 2017: contributor

    utACK 1d31093d4d8501a5dc031413a963707f6cae0e0a
20. laanwj merged this on Apr 26, 2017
21. laanwj closed this on Apr 26, 2017

    ---

22. laanwj referenced this in commit 6fdb319165 on Apr 26, 2017
23. PastaPastaPasta referenced this in commit 726f3489ca on Jun 10, 2019
24. PastaPastaPasta referenced this in commit ee54890805 on Jun 10, 2019
25. PastaPastaPasta referenced this in commit 699ccfb413 on Jun 10, 2019
26. PastaPastaPasta referenced this in commit a5227d5204 on Jun 11, 2019
27. PastaPastaPasta referenced this in commit 447d079c4c on Jun 11, 2019
28. PastaPastaPasta referenced this in commit 1e7690869d on Jun 12, 2019
29. PastaPastaPasta referenced this in commit 191578f86e on Jun 14, 2019
30. PastaPastaPasta referenced this in commit ef84ea4fc1 on Jun 14, 2019
31. PastaPastaPasta referenced this in commit cd69bd0450 on Jun 15, 2019
32. PastaPastaPasta referenced this in commit 1ee6eb75b7 on Jun 19, 2019
33. barrystyle referenced this in commit a002cf242f on Jan 22, 2020
34. furszy referenced this in commit 21c4555cfe on Apr 5, 2021
35. MarcoFalke locked this on Sep 8, 2021

Contributors
sipa theuni laanwj TheBlueMatt jonasschnelli gmaxwell jtimon

Labels
Refactoring