[TSan] Fix p == end == ShadowMem::end in ShadowSet

[Camsyn]

In ShadowSet, when p == end == ShadowMem::end, it triggered an assertion fail previously.

Now we do not allow p == end anymore in ShadowSet.

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Kunqiu Chen (Camsyn)

Changes

In ShadowSet, when p == end == ShadowMem::end, it triggered an assertion fail previously.

Now we do not allow p == end anymore in ShadowSet.

---

Full diff: https://github.com/llvm/llvm-project/pull/144994.diff

1 Files Affected:

• (modified) compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp (+4-2)

diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
index dbdc6359d92aa..57434099e26ff 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
@@ -523,7 +523,7 @@ ALWAYS_INLINE USED void UnalignedMemoryAccess(ThreadState* thr, uptr pc,
 }
 
 void ShadowSet(RawShadow* p, RawShadow* end, RawShadow v) {
-  DCHECK_LE(p, end);
+  DCHECK_LT(p, end);
   DCHECK(IsShadowMem(p));
   DCHECK(p == end || IsShadowMem(end - 1));
   UNUSED const uptr kAlign = kShadowCnt * kShadowSize;
@@ -569,6 +569,7 @@ static void MemoryRangeSet(uptr addr, uptr size, RawShadow val) {
   RawShadow* mid1 =
       Min(end, reinterpret_cast<RawShadow*>(RoundUp(
                    reinterpret_cast<uptr>(begin) + kPageSize / 2, kPageSize)));
+  // begin must < mid1
   ShadowSet(begin, mid1, val);
   // Reset middle part.
   RawShadow* mid2 = RoundDown(end, kPageSize);
@@ -577,7 +578,8 @@ static void MemoryRangeSet(uptr addr, uptr size, RawShadow val) {
       Die();
   }
   // Set the ending.
-  ShadowSet(mid2, end, val);
+  if (mid2 < end)
+    ShadowSet(mid2, end, val);
 }
 
 void MemoryResetRange(ThreadState* thr, uptr pc, uptr addr, uptr size) {

[Camsyn]

My previous PR #144647 changed TSan's Shadow/Meta to exclusive end, but introduced some test fails (mmap_lots.cpp).

• https://lab.llvm.org/buildbot/#/builders/51/builds/18222
• https://lab.llvm.org/buildbot/#/builders/72/builds/12303

After analysis, I believe it is due to the following reasons:

Large mmap -> MemoryRangeSet -> only set the first and last page of shadow.

static void MemoryRangeSet(uptr addr, uptr size, RawShadow val) {
  if (size == 0)
    return;
  RawShadow* begin = MemToShadow(addr);
  // end == ShadowMem::end
  RawShadow* end = begin + size / kShadowCell * kShadowCnt;
  ...
  RawShadow* mid2 = RoundDown(end, kPageSize);
  // What if mid2 == end == ShadowMem::end?
  ShadowSet(mid2, end, val);
  

void ShadowSet(RawShadow* p, RawShadow* end, RawShadow v) {
  DCHECK_LE(p, end); // O: p == end
  DCHECK(IsShadowMem(p)); // X : p == ShadowMem::end 
  DCHECK(p == end || IsShadowMem(end - 1));

I think this actually exposes some degree of flaw in the current implementation.

---

Possible fixes:

1. Do not allow p == end in ShadowSet
   • This is reasonable because both callers of ShadowSet (i.e., MemoryRangeSet and __tsan_java_move) are actually guaranteed that size > 0.
   • Although MemoryRangeSet's dedicated handling for overly large mmap bypasses this limit, we can append a guard that checks for size > 0, i.e., if (mid2 < end) ShadowSet(mid2, end, val);

   //   DCHECK_LE(p, end);
   DCHECK_LT(p, end);

2. Modify the assertion in ShadowSet as follows:

   // DCHECK(IsShadowMem(p));
   DCHECK(p == end || IsShadowMem(p));

3. ShadowSet : Just return while p == end.

    void ShadowSet(RawShadow* p, RawShadow* end, RawShadow v) {
       if (p == end) return;

---

This PR applies the first fix schema.

[thurstond]

Meta comment: LLVM generally encourages reverting broken changes rather than fixing-forward:

'Any time you learn of a serious problem with a change, you should revert it. We strongly encourage “revert to green” as opposed to “fixing forward”. We encourage reverting first, investigating offline, and then reapplying the fixed patch - possibly after another round of review if warranted.' (https://llvm.org/docs/DeveloperPolicy.html)

If the fix was trivial then a fix-forward would be fine (and could skip even review). This fix is a bit finicky (it requires reasoning about all the calls to ShadowSet) so I wouldn't consider it a trivial fix-forward.

That said, if you are confident this fixes the issue (and have re-run the failing tests?), this looks good to me.

[thurstond]

cc @pcc (build watch): FYI

[Camsyn]

Hi @thurstond ,

Thank you so much for the detailed review, the approval, and especially for the guidance on LLVM's developer policy. As a new contributor to LLVM, this is incredibly helpful.

You've made a very good point. I now understand that reverting the breaking change is the preferred way to keep the main branch stable, especially when the fix is not trivial.

FYI, I think this change is more than just a pure fix for that PR. I found that the ShadowSet function's requirements for its input were too relaxed. Therefore, I restricted its input and updated all of its callers to adapt to the new rule.

Since the PR is approved, should I:

1. Merge this fix now? Or
2. Revert the original change first and then re-submit a new PR (and request reviews) as you suggested?

Just want to make sure I'm following the correct procedure. Thanks for your help!

[thurstond]

Since the PR is approved, should I:

1. Merge this fix now? Or
2. Revert the original change first and then re-submit a new PR (and request reviews) as you suggested?

Just want to make sure I'm following the correct procedure. Thanks for your help!

Either is fine, I'll leave it up to your judgment.

Although it is "strongly encouraged" to revert to green, that means it is not mandatory to do so.

• Since you already have this fix-forward prepared (and reviewed), if you are confident this resolves all the issues, then please merge it. This would avoid the unnecessary effort of reverting the previous change, then submitting a new PR.
• However, if you have any doubts at all that there may be lingering issues, then the revert is preferred (because otherwise there will be two separate patches to revert to get back to green).

Welcome to LLVM, and thanks for your contributions :-)