Skip to content

Commit

Permalink
Fix TSAN lock inversion with folly::RequestContext::setContext
Browse files Browse the repository at this point in the history 
Summary:
TSAN flagged the following lock inversion below. The reason for this lock inversion
is because of the following example usage:

1. set request context B, save old one A (lock B, lock A)
2. later on, reset request context back to A, get the old one back B (lock A, lock B)

In the two calls, we always locked the the new context first, resulting
in a lock inversion. The fix is to always acquire them in a consistent
order using `folly::acquireLocked`

TSAN report:

```
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=1574307)
  Cycle in lock order graph: M159 (0x7b1400000138) => M157 (0x7b14000000e8) => M159

  Mutex M157 acquired here while holding mutex M159 in main thread:
    #0 0x48f18c in AnnotateRWLockAcquired ??:?
    #1 0x315495 in folly::SharedMutexImpl<false, void, std::atomic, false>::lock_shared() ./folly/SharedMutex.h:385
    facebook#2 0x1b0eb8 in folly::detail::LockTraitsImpl<folly::SharedMutexImpl<false, void, std::atomic, false>, (folly::detail::MutexLevel)1, false>::lock_shared(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:157
    facebook#3 0x1b0e68 in std::integral_constant<bool, true> folly::LockPolicyShared::lock<folly::SharedMutexImpl<false, void, std::atomic, false> >(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:493
    facebook#4 0x1d63a9 in LockedPtrBase ./folly/Synchronized.h:1026
    facebook#5 0x1d6258 in LockedPtr ./folly/Synchronized.h:1326
    facebook#6 0x1c9a17 in folly::SynchronizedBase<folly::Synchronized<folly::RequestContext::State, folly::SharedMutexImpl<false, void, std::atomic, false> >, (folly::detail::MutexLevel)1>::rlock() const ./folly/Synchronized.h:123
    facebook#7 0x1c7530 in folly::RequestContext::setContext(std::shared_ptr<folly::RequestContext>) ./folly/io/async/Request.cpp:243
    facebook#8 0x36a7b in folly::RequestContext::create() ./folly/io/async/Request.h:119
    facebook#9 0x34916 in RequestContextScopeGuard ./folly/io/async/Request.h:269

  Mutex M159 previously acquired by the same thread here:
    #0 0x48f18c in AnnotateRWLockAcquired ??:?
    #1 0x315495 in folly::SharedMutexImpl<false, void, std::atomic, false>::lock_shared() ./folly/SharedMutex.h:385
    facebook#2 0x1b0eb8 in folly::detail::LockTraitsImpl<folly::SharedMutexImpl<false, void, std::atomic, false>, (folly::detail::MutexLevel)1, false>::lock_shared(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:157
    facebook#3 0x1b0e68 in std::integral_constant<bool, true> folly::LockPolicyShared::lock<folly::SharedMutexImpl<false, void, std::atomic, false> >(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:493
    facebook#4 0x1d63a9 in LockedPtrBase ./folly/Synchronized.h:1026
    facebook#5 0x1d6258 in LockedPtr ./folly/Synchronized.h:1326
    facebook#6 0x1c9a17 in folly::SynchronizedBase<folly::Synchronized<folly::RequestContext::State, folly::SharedMutexImpl<false, void, std::atomic, false> >, (folly::detail::MutexLevel)1>::rlock() const ./folly/Synchronized.h:123
    facebook#7 0x1c7516 in folly::RequestContext::setContext(std::shared_ptr<folly::RequestContext>) ./folly/io/async/Request.cpp:242
    facebook#8 0x36a7b in folly::RequestContext::create() ./folly/io/async/Request.h:119
    facebook#9 0x34916 in RequestContextScopeGuard ./folly/io/async/Request.h:269

  Mutex M159 acquired here while holding mutex M157 in main thread:
    #0 0x48f18c in AnnotateRWLockAcquired ??:?
    #1 0x315495 in folly::SharedMutexImpl<false, void, std::atomic, false>::lock_shared() ./folly/SharedMutex.h:385
    facebook#2 0x1b0eb8 in folly::detail::LockTraitsImpl<folly::SharedMutexImpl<false, void, std::atomic, false>, (folly::detail::MutexLevel)1, false>::lock_shared(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:157
    facebook#3 0x1b0e68 in std::integral_constant<bool, true> folly::LockPolicyShared::lock<folly::SharedMutexImpl<false, void, std::atomic, false> >(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:493
    facebook#4 0x1d63a9 in LockedPtrBase ./folly/Synchronized.h:1026
    facebook#5 0x1d6258 in LockedPtr ./folly/Synchronized.h:1326
    facebook#6 0x1c9a17 in folly::SynchronizedBase<folly::Synchronized<folly::RequestContext::State, folly::SharedMutexImpl<false, void, std::atomic, false> >, (folly::detail::MutexLevel)1>::rlock() const ./folly/Synchronized.h:123
    facebook#7 0x1c7530 in folly::RequestContext::setContext(std::shared_ptr<folly::RequestContext>) ./folly/io/async/Request.cpp:243
    facebook#8 0x34b99 in ~RequestContextScopeGuard ./folly/io/async/Request.h:278

  Mutex M157 previously acquired by the same thread here:
    #0 0x48f18c in AnnotateRWLockAcquired ??:?
    #1 0x315495 in folly::SharedMutexImpl<false, void, std::atomic, false>::lock_shared() ./folly/SharedMutex.h:385
    facebook#2 0x1b0eb8 in folly::detail::LockTraitsImpl<folly::SharedMutexImpl<false, void, std::atomic, false>, (folly::detail::MutexLevel)1, false>::lock_shared(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:157
    facebook#3 0x1b0e68 in std::integral_constant<bool, true> folly::LockPolicyShared::lock<folly::SharedMutexImpl<false, void, std::atomic, false> >(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:493
    facebook#4 0x1d63a9 in LockedPtrBase ./folly/Synchronized.h:1026
    facebook#5 0x1d6258 in LockedPtr ./folly/Synchronized.h:1326
    facebook#6 0x1c9a17 in folly::SynchronizedBase<folly::Synchronized<folly::RequestContext::State, folly::SharedMutexImpl<false, void, std::atomic, false> >, (folly::detail::MutexLevel)1>::rlock() const ./folly/Synchronized.h:123
    facebook#7 0x1c7516 in folly::RequestContext::setContext(std::shared_ptr<folly::RequestContext>) ./folly/io/async/Request.cpp:242
    facebook#8 0x34b99 in ~RequestContextScopeGuard ./folly/io/async/Request.h:278

```

Reviewed By: igorsugak

Differential Revision: D9797715

fbshipit-source-id: a0f991ae0ec8f3e9d33af6e05db49eceaf3a5174
  • Loading branch information
@kennyyu @sandraiser
kennyyu authored and sandraiser committed Mar 4, 2019
1 parent 937f581 commit d528696
Showing 1 changed file with 4 additions and 2 deletions.
6 changes: 4 additions & 2 deletions folly/io/async/Request.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,10 @@ std::shared_ptr<RequestContext> RequestContext::setContext(
auto curCtx = staticCtx;
if (newCtx && curCtx) {
// Only call set/unset for all request data that differs
auto newLock = newCtx->state_.rlock();
auto curLock = curCtx->state_.rlock();
auto ret = folly::acquireLocked(
as_const(newCtx->state_), as_const(curCtx->state_));
auto& newLock = std::get<0>(ret);
auto& curLock = std::get<1>(ret);
auto& newData = newLock->callbackData_;
auto& curData = curLock->callbackData_;
exec_set_difference(
Expand Down

0 comments on commit d528696

Please sign in to comment.