Skip to content

Commit bc90166

Browse files
authored
[TSan] Clarify and enforce shadow end alignment (#144648)
In TSan, every `k` bytes of application memory (where `k = 8`) maps to a single shadow/meta cell. This design leads to two distinct outcomes when calculating the end of a shadow range using `MemToShadow(addr_end)`, depending on the alignment of `addr_end`: - **Exclusive End:** If `addr_end` is aligned (`addr_end % k == 0`), `MemToShadow(addr_end)` points to the first shadow cell *past* the intended range. This address is an exclusive boundary marker, not a cell to be operated on. - **Inclusive End:** If `addr_end` is not aligned (`addr_end % k != 0`), `MemToShadow(addr_end)` points to the last shadow cell that *is* part of the range (i.e., the same cell as `MemToShadow(addr_end - 1)`). Different TSan functions have different expectations for whether the shadow end should be inclusive or exclusive. However, these expectations are not always explicitly enforced, which can lead to subtle bugs or reliance on unstated invariants. The core of this patch is to ensure that functions ONLY requiring an **exclusive shadow end** behave correctly. 1. Enforcing Existing Invariants: For functions like `MetaMap::MoveMemory` and `MapShadow`, the assumption is that the end address is always `k`-aligned. While this holds true in the current codebase (e.g., due to some external implicit conditions), this invariant is not guaranteed by the function's internal context. We add explicit assertions to make this requirement clear and to catch any future changes that might violate this assumption. 2. Fixing Latent Bugs: In other cases, unaligned end addresses are possible, representing a latent bug. This was the case in `UnmapShadow`. The `size` of a memory region being unmapped is not always a multiple of `k`. When this happens, `UnmapShadow` would fail to clear the final (tail) portion of the shadow memory. This patch fixes `UnmapShadow` by rounding up the `size` to the next multiple of `k` before clearing the shadow memory. This is safe because the underlying OS `unmap` operation is page-granular, and the page size is guaranteed to be a multiple of `k`. Notably, this fix makes `UnmapShadow` consistent with its inverse operation, `MemoryRangeImitateWriteOrResetRange`, which already performs a similar size round-up. In summary, this PR: - **Adds assertions** to `MetaMap::MoveMemory` and `MapShadow` to enforce their implicit requirement for k-aligned end addresses. - **Fixes a latent bug** in `UnmapShadow` by rounding up the size to ensure the entire shadow range is cleared. Two new test cases have been added to cover this scenario. - Removes a redundant assertion in `__tsan_java_move`. - Fixes an incorrect shadow end calculation introduced in commit 4052de6. The previous logic, while fixing an overestimation issue, did not properly account for `kShadowCell` alignment and could lead to underestimation.
1 parent 7726103 commit bc90166

File tree

6 files changed

+42
-14
lines changed

6 files changed

+42
-14
lines changed

compiler-rt/lib/tsan/rtl/tsan_interface_java.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,6 @@ void __tsan_java_move(jptr src, jptr dst, jptr size) {
122122
DCHECK_GE(dst, jctx->heap_begin);
123123
DCHECK_LE(dst + size, jctx->heap_begin + jctx->heap_size);
124124
DCHECK_NE(dst, src);
125-
DCHECK_NE(size, 0);
126125

127126
// Assuming it's not running concurrently with threads that do
128127
// memory accesses and mutex operations (stop-the-world phase).

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

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -566,17 +566,32 @@ static bool IsValidMmapRange(uptr addr, uptr size) {
566566
return false;
567567
}
568568

569-
void UnmapShadow(ThreadState *thr, uptr addr, uptr size) {
569+
void UnmapShadow(ThreadState* thr, uptr addr, uptr size) {
570570
if (size == 0 || !IsValidMmapRange(addr, size))
571571
return;
572-
DontNeedShadowFor(addr, size);
572+
// unmap shadow is related to semantic of mmap/munmap, so we
573+
// should clear the whole shadow range, including the tail shadow
574+
// while addr + size % kShadowCell != 0.
575+
uptr rounded_size_shadow = RoundUp(addr + size, kShadowCell) - addr;
576+
DontNeedShadowFor(addr, rounded_size_shadow);
573577
ScopedGlobalProcessor sgp;
574578
SlotLocker locker(thr, true);
575-
ctx->metamap.ResetRange(thr->proc(), addr, size, true);
579+
uptr rounded_size_meta = RoundUp(addr + size, kMetaShadowCell) - addr;
580+
ctx->metamap.ResetRange(thr->proc(), addr, rounded_size_meta, true);
576581
}
577582
#endif
578583

579584
void MapShadow(uptr addr, uptr size) {
585+
// Although named MapShadow, this function's semantic is unrelated to
586+
// UnmapShadow. This function currently only used for Go's lazy allocation
587+
// of shadow, whose targets are program section (e.g., bss, data, etc.).
588+
// Therefore, we can guarantee that the addr and size align to kShadowCell
589+
// and kMetaShadowCell by the following assertions.
590+
DCHECK_EQ(addr % kShadowCell, 0);
591+
DCHECK_EQ(size % kShadowCell, 0);
592+
DCHECK_EQ(addr % kMetaShadowCell, 0);
593+
DCHECK_EQ(size % kMetaShadowCell, 0);
594+
580595
// Ensure thead registry lock held, so as to synchronize
581596
// with DoReset, which also access the mapped_shadow_* ctxt fields.
582597
ThreadRegistryLock lock0(&ctx->thread_registry);

compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -688,16 +688,18 @@ void MemoryAccessRangeT(ThreadState* thr, uptr pc, uptr addr, uptr size) {
688688
DCHECK(IsShadowMem(shadow_mem));
689689
}
690690

691-
RawShadow* shadow_mem_end = reinterpret_cast<RawShadow*>(
692-
reinterpret_cast<uptr>(shadow_mem) + size * kShadowMultiplier - 1);
693-
if (!IsShadowMem(shadow_mem_end)) {
694-
Printf("Bad shadow end addr: %p (%p)\n", shadow_mem_end,
691+
uptr rounded_size =
692+
(RoundUpTo(addr + size, kShadowCell) - RoundDownTo(addr, kShadowCell));
693+
RawShadow* shadow_mem_end =
694+
shadow_mem + rounded_size / kShadowCell * kShadowCnt;
695+
if (!IsShadowMem(shadow_mem_end - 1)) {
696+
Printf("Bad shadow end addr: %p (%p)\n", shadow_mem_end - 1,
695697
(void*)(addr + size - 1));
696698
Printf(
697-
"Shadow start addr (ok): %p (%p); size: 0x%zx; kShadowMultiplier: "
698-
"%zx\n",
699-
shadow_mem, (void*)addr, size, kShadowMultiplier);
700-
DCHECK(IsShadowMem(shadow_mem_end));
699+
"Shadow start addr (ok): %p (%p); size: 0x%zx; rounded_size: 0x%zx; "
700+
"kShadowMultiplier: %zx\n",
701+
shadow_mem, (void*)addr, size, rounded_size, kShadowMultiplier);
702+
DCHECK(IsShadowMem(shadow_mem_end - 1));
701703
}
702704
#endif
703705

compiler-rt/lib/tsan/rtl/tsan_sync.cpp

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,20 @@ void MetaMap::MoveMemory(uptr src, uptr dst, uptr sz) {
246246
// there are no concurrent accesses to the regions (e.g. stop-the-world).
247247
CHECK_NE(src, dst);
248248
CHECK_NE(sz, 0);
249+
250+
// The current MoveMemory implementation behaves incorrectly when src, dst,
251+
// and sz are not aligned to kMetaShadowCell.
252+
// For example, with kMetaShadowCell == 8:
253+
// - src = 4: unexpectedly clears the metadata for the range [0, 4).
254+
// - src = 16, dst = 4, size = 8: A sync variable for addr = 20, which should
255+
// be moved to the metadata for address 8, is incorrectly moved to the
256+
// metadata for address 0 instead.
257+
// - src = 0, sz = 4: fails to move the tail metadata.
258+
// Therefore, the following assertions is needed.
259+
DCHECK_EQ(src % kMetaShadowCell, 0);
260+
DCHECK_EQ(dst % kMetaShadowCell, 0);
261+
DCHECK_EQ(sz % kMetaShadowCell, 0);
262+
249263
uptr diff = dst - src;
250264
u32 *src_meta, *dst_meta, *src_meta_end;
251265
uptr inc;

compiler-rt/test/tsan/java_heap_init2.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
// RUN: %clangxx_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
2-
// XFAIL: *
32

43
#include "java.h"
54
#include <errno.h>

compiler-rt/test/tsan/munmap_clear_shadow.c

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
// RUN: %clang_tsan %s -o %t && %run %t | FileCheck %s
2-
// XFAIL: *
32

43
#include "test.h"
54
#include <assert.h>

0 commit comments

Comments
 (0)