Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: Add missing cs_db lock #15322

Merged
merged 1 commit into from Feb 4, 2019

Conversation

Projects
None yet
5 participants
@promag
Copy link
Member

commented Feb 1, 2019

Without this lock BerkeleyEnvironment::~BerkeleyEnvironment and GetWalletEnv would race for g_dbenvs. This wasn't detected before because thread safety analysis does not check constructors
and destructors.

Reference: http://releases.llvm.org/5.0.2/tools/clang/docs/ThreadSafetyAnalysis.html#no-checking-inside-constructors-and-destructors

wallet: Add missing cs_db lock
Without this lock BerkeleyEnvironment::~BerkeleyEnvironment and
GetWalletEnv would race for g_dbenvs. This wasn't detected before
because thread safety analysis does not check constructors and
destructors.
@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

@practicalswift is this correct?

@ryanofsky please take a look too.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

utACK 712d35b. Would be nice if there was a (unit) test that run into problems with tsan.

@jonasschnelli jonasschnelli added the Wallet label Feb 2, 2019

@practicalswift

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

utACK 712d35b

Good catch!

Could be similar issues in other constructors/destructors.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

utACK, good catch ! 712d35b

@laanwj laanwj merged commit 712d35b into bitcoin:master Feb 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Feb 4, 2019

Merge #15322: wallet: Add missing cs_db lock
712d35b wallet: Add missing cs_db lock (João Barbosa)

Pull request description:

  Without this lock `BerkeleyEnvironment::~BerkeleyEnvironment` and `GetWalletEnv` would race for `g_dbenvs`. This wasn't detected before because thread safety analysis does not check constructors
  and destructors.

  Reference: http://releases.llvm.org/5.0.2/tools/clang/docs/ThreadSafetyAnalysis.html#no-checking-inside-constructors-and-destructors

Tree-SHA512: 350cb2b991ca699a6bca85f87c82c38f0814484c8ccb0d7d83cb3bff9afcf60dd32b2a9554a9e72eb5803bfad8b6970fe7da618b39be5889178b86faa1b74124

@promag promag deleted the promag:2019-01-cs_db branch Feb 4, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

... race for g_dbenvs. This wasn't detected before because thread safety analysis does not check constructors and destructors.

A solution to that would be to require that constructors don't access globals in their body. Calling a function that does it for them would be fine, since that is covered by the static analyser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.