Fix several potential issues found by sanitizers #9743

sipa · 2017-02-11T20:49:44Z

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 Prefer std::atomic over MemoryBarrier google/leveldb#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.

theuni · 2017-02-11T23:15:20Z

src/httpserver.cpp

    {
+        std::lock_guard<std::mutex> lock(cs);


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

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

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.

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.

sipa · 2017-02-11T23:18:21Z

It's not (see PR description). I think tsan is confused because it's accessed from another object.

jonasschnelli · 2017-02-12T14:06:06Z

src/bitcoin-tx.cpp

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


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

Sorry,.. missed the fact that std::unique_ptr<Secp256k1Init> ecc; initialises ecc with nullptr

sipa · 2017-02-12T18:37:51Z

@jonasschnelli Where would it be set to something else?

gmaxwell

ACK

sipa · 2017-02-20T20:16:04Z

I'll try to find something less insane that also doesn't trip it.

sipa · 2017-03-05T03:39:05Z

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

laanwj · 2017-03-05T12:43:45Z

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

I think you forgot to push, it's still there: a10eff9

sipa · 2017-03-29T18:42:31Z

Rebased, and removed the LevelDB and the patches for spurious warning. The LevelDB patch should go through the leveldb repo instead.

TheBlueMatt · 2017-04-17T14:25:51Z

src/sync.cpp

@@ -71,7 +71,7 @@ struct LockData {
    boost::mutex dd_mutex;
 } static lockdata;

-boost::thread_specific_ptr<LockStack> lockstack;
+boost::thread_specific_ptr<LockStack> lockstack([](LockStack* ptr) { delete ptr; });


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

Agree, and tests seem to agree that they are indeed invoked. I'll remove it.

TheBlueMatt · 2017-04-17T14:27:40Z

src/utiltime.cpp

@@ -25,7 +28,7 @@ int64_t GetTime()

 void SetMockTime(int64_t nMockTimeIn)
 {
-    nMockTime = nMockTimeIn;
+    nMockTime.store(nMockTimeIn, std::memory_order_relaxed);


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.

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

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

GetTime is called all over the place, no? I don't want to introduce extra synchronization there.

jtimon · 2017-04-24T19:45:46Z

utACK 1d31093

1d31093 fix tsan: utiltime race on nMockTime (Pieter Wuille) 321bbc2 fix ubsan: bitcoin-tx: not initialize context before IsFullyValid (Pieter Wuille) Tree-SHA512: 39ea83c6122f06339cd425deb236357694e84ce2e4e9c61c10b90a8909b6e42e8c7b76396175cdc4723ababd2fa4f935d48f8a469baf853c5a06d7b962a5c8dc

267ee79 fix tsan: utiltime race on nMockTime (Pieter Wuille) be2b6b5 fix ubsan: bitcoin-tx: not initialize context before IsFullyValid (Pieter Wuille) bb65b9e [Refactoring] Additions/updates to pivx-tx.cpp (random-zebra) 2c944c7 [Refactoring] Add SEQUENCE_FINAL constant in CTxIn (random-zebra) 4501198 [Refactoring] Move non-throwing ParseHashStr from rest to core_read (random-zebra) 29e23bf [Refactoring] Add constant for max pubkeys per multisig (random-zebra) Pull request description: Simple refactoring + backport bitcoin#9743 ACKs for top commit: Fuzzbawls: ACK 267ee79 furszy: looking good, utACK 267ee79 and merging.. Tree-SHA512: a46c46ea3b9b3e625b13978448787b0afbd9233a1893dea0a400abb3aa7c73524968453d5955ceca991f587a431daed0f4a44744b4bfdd8360d591355f40c8cf

fanquake added the Refactoring label Feb 11, 2017

theuni reviewed Feb 11, 2017

View reviewed changes

jonasschnelli reviewed Feb 12, 2017

View reviewed changes

gmaxwell approved these changes Feb 13, 2017

View reviewed changes

sipa force-pushed the fsanitize branch from 78f203d to 9169ced Compare March 29, 2017 18:41

TheBlueMatt reviewed Apr 17, 2017

View reviewed changes

sipa added 2 commits April 20, 2017 06:25

fix ubsan: bitcoin-tx: not initialize context before IsFullyValid

321bbc2

fix tsan: utiltime race on nMockTime

1d31093

sipa force-pushed the fsanitize branch from 9169ced to 1d31093 Compare April 20, 2017 13:25

laanwj merged commit 1d31093 into bitcoin:master Apr 26, 2017

sipa mentioned this pull request Mar 14, 2018

Add -ftrapv to CFLAGS and CXXFLAGS when --enable-debug is used. Enable -ftrapv in Travis. #12686

Merged

practicalswift mentioned this pull request Apr 16, 2018

tests: Make test_bitcoin pass under ThreadSanitzer (clang). Fix lock-order-inversion (potential deadlock). #12882

Merged

sickpig mentioned this pull request Jan 28, 2019

Fix several potential issues found by sanitizers BitcoinUnlimited/BitcoinUnlimited#1573

Merged

random-zebra mentioned this pull request Apr 2, 2021

[Core] Fix several potential issues found by sanitizers PIVX-Project/PIVX#2289

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Fix several potential issues found by sanitizers #9743

Fix several potential issues found by sanitizers #9743

Uh oh!

Conversation

sipa commented Feb 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Feb 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Feb 12, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

sipa commented Feb 20, 2017

Uh oh!

sipa commented Mar 5, 2017

Uh oh!

laanwj commented Mar 5, 2017

Uh oh!

sipa commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Apr 24, 2017

Uh oh!

Uh oh!