Skip to content

Commit 0aafeb8

Browse files
authored
Reland [TSan] Clarify and enforce shadow end alignment (#146676)
#144648 was reverted because it failed the new sanitizer test `munmap_clear_shadow.c` in IOS's CI. That issue could be fixed by disabling the test on some platforms, due to the incompatibility of the test on these platforms. In detail, we should disable the test in FreeBSD, Apple, NetBSD, Solaris, and Haiku, where `ReleaseMemoryPagesToOS` executes `madvise(beg, end, MADV_FREE)`, which tags the relevant pages as 'FREE' and does not release them immediately.
1 parent c0e9084 commit 0aafeb8

File tree

6 files changed

+47
-14
lines changed

6 files changed

+47
-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: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
// RUN: %clang_tsan %s -o %t && %run %t | FileCheck %s
2-
// XFAIL: *
2+
3+
// In these systems, the behavior of ReleaseMemoryPagesToOS is madvise(beg, end, MADV_FREE),
4+
// which tags the relevant pages as 'FREE' and does not release them immediately.
5+
// Therefore, we cannot assume that __tsan_read1 will not race with the shadow cleared.
6+
// UNSUPPORTED: darwin,target={{.*(freebsd|netbsd|solaris|haiku).*}}
37

48
#include "test.h"
59
#include <assert.h>

0 commit comments

Comments
 (0)