Commit b2a33051 authored by Kenny Yu's avatar Kenny Yu Committed by Facebook Github Bot

Fix TSAN lock inversion with folly::RequestContext::setContext

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
    #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
    #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
    #4 0x1d63a9 in LockedPtrBase ./folly/Synchronized.h:1026
    #5 0x1d6258 in LockedPtr ./folly/Synchronized.h:1326
    #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
    #7 0x1c7530 in folly::RequestContext::setContext(std::shared_ptr<folly::RequestContext>) ./folly/io/async/Request.cpp:243
    #8 0x36a7b in folly::RequestContext::create() ./folly/io/async/Request.h:119
    #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
    #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
    #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
    #4 0x1d63a9 in LockedPtrBase ./folly/Synchronized.h:1026
    #5 0x1d6258 in LockedPtr ./folly/Synchronized.h:1326
    #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
    #7 0x1c7516 in folly::RequestContext::setContext(std::shared_ptr<folly::RequestContext>) ./folly/io/async/Request.cpp:242
    #8 0x36a7b in folly::RequestContext::create() ./folly/io/async/Request.h:119
    #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
    #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
    #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
    #4 0x1d63a9 in LockedPtrBase ./folly/Synchronized.h:1026
    #5 0x1d6258 in LockedPtr ./folly/Synchronized.h:1326
    #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
    #7 0x1c7530 in folly::RequestContext::setContext(std::shared_ptr<folly::RequestContext>) ./folly/io/async/Request.cpp:243
    #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
    #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
    #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
    #4 0x1d63a9 in LockedPtrBase ./folly/Synchronized.h:1026
    #5 0x1d6258 in LockedPtr ./folly/Synchronized.h:1326
    #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
    #7 0x1c7516 in folly::RequestContext::setContext(std::shared_ptr<folly::RequestContext>) ./folly/io/async/Request.cpp:242
    #8 0x34b99 in ~RequestContextScopeGuard ./folly/io/async/Request.h:278

```

Reviewed By: igorsugak

Differential Revision: D9797715

fbshipit-source-id: a0f991ae0ec8f3e9d33af6e05db49eceaf3a5174
parent 272a2e9a
...@@ -239,8 +239,10 @@ std::shared_ptr<RequestContext> RequestContext::setContext( ...@@ -239,8 +239,10 @@ std::shared_ptr<RequestContext> RequestContext::setContext(
auto curCtx = staticCtx; auto curCtx = staticCtx;
if (newCtx && curCtx) { if (newCtx && curCtx) {
// Only call set/unset for all request data that differs // Only call set/unset for all request data that differs
auto newLock = newCtx->state_.rlock(); auto ret = folly::acquireLocked(
auto curLock = curCtx->state_.rlock(); 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& newData = newLock->callbackData_;
auto& curData = curLock->callbackData_; auto& curData = curLock->callbackData_;
exec_set_difference( exec_set_difference(
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment