[TSan] Fix potentially problematic shadow end calculations

[Camsyn]

This is an improvement that enhances the robustness of the code.

Previously, the correct calculation of exclusive EndShadow relied on the assumption that addr_end % kShadowCell == 0; however, in many current usages, this assumption was not strictly guaranteed (although it did in fact meet).

In addition, computing EndShadow does not require the corresponding address to be AppMem; for example, HighAppEnd is not AppMem, but can still be used to calculate EndShadow.

For example, for the AppMem range [0, 1), s = MemToShadow(0) is equal to MemToShadow(1). The previous logic would incorrectly deduce an empty shadow range [s, s).
The correct shadow range should be [s, s + kShadowSize * kShadowCnt) to cover all the shadow memory for the accessed cell.

This commit addresses this in two ways:

1. It introduces a dedicated utility function, i.e., MemToEndShadow, to correctly calculate the end of a shadow memory range, accounting for the memory cell granularity.
2. It replaces existing (and potentially incorrect) calculations of the shadow end with this new utility function.

Additionally, the previous commit 4052de6 resolved a problem with overestimating the shadow end; it did not consider kShadowCell and could therefore lead to underestimates.
This is also corrected by utilizing the MemToEndShadow function.

[llvmbot]

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

Author: Kunqiu Chen (Camsyn)

Changes

This is an improvement that enhances the robustness of the code.

Previously, the correct calculation of exclusive EndShadow relied on the assumption that addr_end % kShadowCell == 0; however, in many current usages, this assumption was not strictly guaranteed (although it did in fact meet).

In addition, computing EndShadow does not require the corresponding address to be AppMem; for example, HighAppEnd is not AppMem, but can still be used to calculate EndShadow.

For example, for the AppMem range [0, 1), s = MemToShadow(0) is equal to MemToShadow(1). The previous logic would incorrectly deduce an empty shadow range [s, s).
The correct shadow range should be [s, s + kShadowSize * kShadowCnt) to cover all the shadow memory for the accessed cell.

This commit addresses this in two ways:

1. It introduces a dedicated utility function, i.e., MemToEndShadow, to correctly calculate the end of a shadow memory range, accounting for the memory cell granularity.
2. It replaces existing (and potentially incorrect) calculations of the shadow end with this new utility function.

Additionally, the previous commit 4052de6 resolved a problem with overestimating the shadow end; it did not consider kShadowCell and could therefore lead to underestimates.
This is also corrected by utilizing the MemToEndShadow function.

---

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

5 Files Affected:

• (modified) compiler-rt/lib/tsan/rtl/tsan_interface_java.cpp (+1-2)
• (modified) compiler-rt/lib/tsan/rtl/tsan_platform.h (+18)
• (modified) compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp (+1-1)
• (modified) compiler-rt/lib/tsan/rtl/tsan_rtl.cpp (+3-3)
• (modified) compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp (+4-5)

diff --git a/compiler-rt/lib/tsan/rtl/tsan_interface_java.cpp b/compiler-rt/lib/tsan/rtl/tsan_interface_java.cpp
index 7c15a16388268..3e324eef9cb8d 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interface_java.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interface_java.cpp
@@ -122,7 +122,6 @@ void __tsan_java_move(jptr src, jptr dst, jptr size) {
   DCHECK_GE(dst, jctx->heap_begin);
   DCHECK_LE(dst + size, jctx->heap_begin + jctx->heap_size);
   DCHECK_NE(dst, src);
-  DCHECK_NE(size, 0);
 
   // Assuming it's not running concurrently with threads that do
   // memory accesses and mutex operations (stop-the-world phase).
@@ -132,7 +131,7 @@ void __tsan_java_move(jptr src, jptr dst, jptr size) {
   // We used to move shadow from src to dst, but the trace format does not
   // support that anymore as it contains addresses of accesses.
   RawShadow *d = MemToShadow(dst);
-  RawShadow *dend = MemToShadow(dst + size);
+  RawShadow *dend = MemToEndShadow(dst + size);
   ShadowSet(d, dend, Shadow::kEmpty);
 }
 
diff --git a/compiler-rt/lib/tsan/rtl/tsan_platform.h b/compiler-rt/lib/tsan/rtl/tsan_platform.h
index 354f6da6a64a1..5b589df482256 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_platform.h
+++ b/compiler-rt/lib/tsan/rtl/tsan_platform.h
@@ -968,6 +968,24 @@ RawShadow *MemToShadow(uptr x) {
   return reinterpret_cast<RawShadow *>(SelectMapping<MemToShadowImpl>(x));
 }
 
+struct MemToEndShadowImpl {
+  template <typename Mapping>
+  static uptr Apply(uptr x) {
+    return (((x + kShadowCell - 1) &
+             ~(Mapping::kShadowMsk | (kShadowCell - 1))) ^
+            Mapping::kShadowXor) *
+               kShadowMultiplier +
+           Mapping::kShadowAdd;
+  }
+};
+
+// If addr % kShadowCell == 0, then MemToEndShadow(addr) == MemToShadow(addr)
+// Otherwise, MemToEndShadow(addr) == MemToShadow(addr) + kShadowCnt
+ALWAYS_INLINE
+RawShadow *MemToEndShadow(uptr x) {
+  return reinterpret_cast<RawShadow *>(SelectMapping<MemToEndShadowImpl>(x));
+}
+
 struct MemToMetaImpl {
   template <typename Mapping>
   static u32 *Apply(uptr x) {
diff --git a/compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp b/compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp
index 2c55645a15479..dbf583b362359 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp
@@ -195,7 +195,7 @@ static NOINLINE void MapRodata(char* buffer, uptr size) {
         !segment.IsWritable() && IsAppMem(segment.start)) {
       // Assume it's .rodata
       char *shadow_start = (char *)MemToShadow(segment.start);
-      char *shadow_end = (char *)MemToShadow(segment.end);
+      char *shadow_end = (char *)MemToEndShadow(segment.end);
       for (char *p = shadow_start; p < shadow_end;
            p += marker.size() * sizeof(RawShadow)) {
         internal_mmap(
diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp
index c83efec8eaca2..4afb84e89936a 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp
@@ -532,7 +532,7 @@ static void StopBackgroundThread() {
 
 void DontNeedShadowFor(uptr addr, uptr size) {
   ReleaseMemoryPagesToOS(reinterpret_cast<uptr>(MemToShadow(addr)),
-                         reinterpret_cast<uptr>(MemToShadow(addr + size)));
+                         reinterpret_cast<uptr>(MemToEndShadow(addr + size)));
 }
 
 #if !SANITIZER_GO
@@ -588,12 +588,12 @@ void MapShadow(uptr addr, uptr size) {
   // CHECK_EQ(addr, addr & ~((64 << 10) - 1));  // windows wants 64K alignment
   const uptr kPageSize = GetPageSizeCached();
   uptr shadow_begin = RoundDownTo((uptr)MemToShadow(addr), kPageSize);
-  uptr shadow_end = RoundUpTo((uptr)MemToShadow(addr + size), kPageSize);
+  uptr shadow_end = RoundUpTo((uptr)MemToEndShadow(addr + size), kPageSize);
   if (!MmapFixedNoReserve(shadow_begin, shadow_end - shadow_begin, "shadow"))
     Die();
 #else
   uptr shadow_begin = RoundDownTo((uptr)MemToShadow(addr), (64 << 10));
-  uptr shadow_end = RoundUpTo((uptr)MemToShadow(addr + size), (64 << 10));
+  uptr shadow_end = RoundUpTo((uptr)MemToEndShadow(addr + size), (64 << 10));
   VPrintf(2, "MapShadow for (0x%zx-0x%zx), begin/end: (0x%zx-0x%zx)\n",
           addr, addr + size, shadow_begin, shadow_end);
 
diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
index cf07686d968dc..9652cd0267a79 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
@@ -684,16 +684,15 @@ void MemoryAccessRangeT(ThreadState* thr, uptr pc, uptr addr, uptr size) {
     DCHECK(IsShadowMem(shadow_mem));
   }
 
-  RawShadow* shadow_mem_end = reinterpret_cast<RawShadow*>(
-      reinterpret_cast<uptr>(shadow_mem) + size * kShadowMultiplier - 1);
-  if (!IsShadowMem(shadow_mem_end)) {
-    Printf("Bad shadow end addr: %p (%p)\n", shadow_mem_end,
+  RawShadow* shadow_mem_end = MemToEndShadow(addr + size);
+  if (size > 0 && !IsShadowMem(shadow_mem_end - 1)) {
+    Printf("Bad shadow end addr: %p (%p)\n", shadow_mem_end - 1,
            (void*)(addr + size - 1));
     Printf(
         "Shadow start addr (ok): %p (%p); size: 0x%zx; kShadowMultiplier: "
         "%zx\n",
         shadow_mem, (void*)addr, size, kShadowMultiplier);
-    DCHECK(IsShadowMem(shadow_mem_end));
+    DCHECK(IsShadowMem(shadow_mem_end - 1));
   }
 #endif
 

[Camsyn]

If it is not appropriate to add a new utility function in tsan_platform.h, we can follow the example of MemoryRangeSet or MemoryResetRange, and

• add an assertion to guarantee addr_end % kShadowCell == 0; or
• use MemToShadow(RoundUpTo(addr_end, kShadowCell)) instead of MemToShadow(addr_end)

for shadow end computation.
However, due to the DCHECK(IsAppMemImpl::Apply<Mapping>(x)) for MemToShadow(x), it still throws an unreasonable error for MemToShadow(AppMem::end), which should be a valid shadow end.

[melver]

Is there a bug? If there's a bug, this needs a test. Otherwise, it's unclear what this is improving.

[Camsyn]

Is there a bug? If there's a bug, this needs a test. Otherwise, it's unclear what this is improving.

I acknowledge that "This is an improvement that enhances the robustness of the code", because the potential 'bugs' can hardly be triggered under the current implementation.

---

In tsan_interface_java.cpp:

  DCHECK_EQ(dst % kHeapAlignment, 0);
  DCHECK_EQ(size % kHeapAlignment, 0);
  ...
  // Duplicated assertion
  DCHECK_NE(size, 0);
  ...
  RawShadow *dend = MemToShadow(dst + size);

It should be a bug if kHeapAlignment != kShadowCell, although they are both equal to 8 in the current implementation.

---

In tsan_rtl_access.cpp:

  RawShadow* shadow_mem_end = reinterpret_cast<RawShadow*>(
      reinterpret_cast<uptr>(shadow_mem) + size * kShadowMultiplier - 1);
  if (!IsShadowMem(shadow_mem_end)) {

The calculation of shadow_mem_end did not consider kShadowCell and could therefore lead to underestimates.
E.g., for app mem [0, 1), it gives a shadow end shadow_of(0) + 1, while the real shadow end is shadow_of(0) + 16.

What's more, if size == 0 (which is not allowed in its caller MemoryAccessRange but allowed in MemoryAccessRangeT), the current calculation of end is problematic.

---

In tsan_platform_linux.cpp:

TSan iterates the proc map and for each segment,
char *shadow_end = (char *)MemToShadow(segment.end); is correct as the assumption of segment.end % kShadowCell == 0 always holds. But do we need to guarantee this condition more explicitly?

---

In tsan_rtl.cpp:

void DontNeedShadowFor(uptr addr, uptr size) {
  ReleaseMemoryPagesToOS(reinterpret_cast<uptr>(MemToShadow(addr)),
                         reinterpret_cast<uptr>(MemToShadow(addr + size)));
}

DontNeedShadowFor(addr, size) is used in ThreadFinish to release the shadow related to the thread stack, and used in unmap(p, size) to release the shadow allocated for mmap.
What if size % kShadowCell != 0? I think it might lead to a missing cleanup of the tail shadow.

E.g., for mmap(addr, size, ..), TSan rounds up the size to poison the shadow, while in munmap, TSan does not (see UnmapShadow). If MemToShadow(addr + size) % kPageSize == 0 and MemToEndShadow(addr + size) > MemToShadow(addr + size), the page of the tail shadow cannot be released theoretically.

What's more, I found another potential bug in DontNeedShadowFor while analyzing: DontNeedShadowFor receives [beg, end] as parameters while TSan transfers [beg, end) as arguments, which might lead to page overreleasing.

In addition, another interface MapShadow(addr, size) shares the same problem.

By the way, this problem seems can be triggered by a well-designed testcase. If necessary, I will design one when I have time.

[thurstond]

Thanks for the patch!

If it is not appropriate to add a new utility function in tsan_platform.h, we can follow the example of MemoryRangeSet or MemoryResetRange, and

• add an assertion to guarantee addr_end % kShadowCell == 0; or

If the assertion is added, do all the tests pass, and does it fix all the problems that adding MemToEndShadow would? If yes, then that is the more elegant solution (besides, I'd probably personally forget to use MemToEndShadow instead of MemToShadow when updating the code).
(If no: are there are some minor code cleanups that would make the assertion approach work?)