[compiler-rt] 4c5476b - tsan: fix NULL deref in TraceSwitchPart

[Dmitry Vyukov via llvm-commits]

Author: Dmitry Vyukov
Date: 2021-12-20T18:55:51+01:00
New Revision: 4c5476b0664a2ba4bd51f69552852160ba6451be

URL: https://github.com/llvm/llvm-project/commit/4c5476b0664a2ba4bd51f69552852160ba6451be
DIFF: https://github.com/llvm/llvm-project/commit/4c5476b0664a2ba4bd51f69552852160ba6451be.diff

LOG: tsan: fix NULL deref in TraceSwitchPart

There is a small chance that the slot may be not queued in TraceSwitchPart.
This can happen if the slot has kEpochLast epoch and another thread
in FindSlotAndLock discovered that it's exhausted and removed it from
the slot queue. kEpochLast can happen in 2 cases: (1) if TraceSwitchPart
was called with the slot locked and epoch already at kEpochLast,
or (2) if we've acquired a new slot in SlotLock in the beginning
of the function and the slot was at kEpochLast - 1, so after increment
in SlotAttachAndLock it become kEpochLast.

If this happens we crash on ctx->slot_queue.Remove(thr->slot).
Skip the requeueing if the slot is not queued.
The slot is exhausted, so it must not be ctx->slot_queue.

The existing stress test triggers this with very small probability.
I am not sure how to make this condition more likely to be triggered,
it evaded lots of testing.

Depends on D116040.

Reviewed By: melver

Differential Revision: https://reviews.llvm.org/D116041

Added: 
    

Modified: 
    compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

Removed: 
    


################################################################################
diff  --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp
index 6d37363558fb1..8822ebaea4fc7 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp
@@ -964,8 +964,18 @@ void TraceSwitchPartImpl(ThreadState* thr) {
   }
   {
     Lock lock(&ctx->slot_mtx);
-    ctx->slot_queue.Remove(thr->slot);
-    ctx->slot_queue.PushBack(thr->slot);
+    // There is a small chance that the slot may be not queued at this point.
+    // This can happen if the slot has kEpochLast epoch and another thread
+    // in FindSlotAndLock discovered that it's exhausted and removed it from
+    // the slot queue. kEpochLast can happen in 2 cases: (1) if TraceSwitchPart
+    // was called with the slot locked and epoch already at kEpochLast,
+    // or (2) if we've acquired a new slot in SlotLock in the beginning
+    // of the function and the slot was at kEpochLast - 1, so after increment
+    // in SlotAttachAndLock it become kEpochLast.
+    if (ctx->slot_queue.Queued(thr->slot)) {
+      ctx->slot_queue.Remove(thr->slot);
+      ctx->slot_queue.PushBack(thr->slot);
+    }
     if (recycle)
       ctx->trace_part_recycle.PushBack(recycle);
   }