Skip to content

[tsan] Fix calculation of shadow end address in MemoryAccessRangeT #98404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 11, 2024

Conversation

thurstond
Copy link
Contributor

MemoryAccessRangeT overestimates the size of the shadow region by 8x, occasionally leading to assertion failure:

  RawShadow* shadow_mem = MemToShadow(addr);
  ...
  // Check that end of shadow is valid
  if (!IsShadowMem(shadow_mem + size * kShadowCnt - 1)) {
    DCHECK(IsShadowMem(shadow_mem + size * kShadowCnt - 1));

It is erroneous for two separate reasons:

  • it uses kShadowCnt (== 4) instead of kShadowMultiplier (== 2)
  • since shadow_mem is a RawShadow*, pointer arithmetic is multiplied by sizeof(RawShadow) == 4

This patch fixes the calculation, and also improves the debugging information.

The assertion error was observed on a buildbot (https://lab.llvm.org/staging/#/builders/89/builds/656/steps/13/logs/stdio):

Bad shadow addr 0x3000000190bc (7fffffffe85f)
ThreadSanitizer: CHECK failed: tsan_rtl_access.cpp:690 "((IsShadowMem(shadow_mem + size * kShadowCnt - 1))) != (0)" (0x0, 0x0) (tid=2202676)

Notice that 0x3000000190bc is not the correct shadow for the end address 0x7fffffffe85f.

This error is more commonly observed on high-entropy ASLR systems, since ASLR may be disabled (if the randomized memory layout is incompatible), leading to an allocation near the boundaries of the high app memory region (and therefore a shadow end that may be erroneously calculated to be past the end of the shadow region). Also note that the assertion is guarded by SANITIZER_DEBUG.

MemoryAccessRangeT overestimates the size of the shadow region by 8x, occasionally leading to assertion failure:
```
  RawShadow* shadow_mem = MemToShadow(addr);
  ...
  // Check that end of shadow is valid
  if (!IsShadowMem(shadow_mem + size * kShadowCnt - 1)) {
    DCHECK(IsShadowMem(shadow_mem + size * kShadowCnt - 1));
```
It is erroneous for two separate reasons:
- it uses kShadowCnt (== 4) instead of kShadowMultiplier (== 2)
- since shadow_mem is a RawShadow*, pointer arithmetic is multiplied by sizeof(RawShadow) == 4

This patch fixes the calculation, and also improves the debugging information.

The assertion error was observed on a buildbot (https://lab.llvm.org/staging/#/builders/89/builds/656/steps/13/logs/stdio):
```
Bad shadow addr 0x3000000190bc (7fffffffe85f)
ThreadSanitizer: CHECK failed: tsan_rtl_access.cpp:690 "((IsShadowMem(shadow_mem + size * kShadowCnt - 1))) != (0)" (0x0, 0x0) (tid=2202676)
```
Notice that 0x3000000190bc is not the correct shadow for the end address 0x7fffffffe85f.

This error is more commonly observed on high-entropy ASLR systems, since ASLR may be disabled (if the randomized memory layout is incompatible), leading to an allocation near the boundaries of the high app memory region (and therefore a shadow end that may be erroneously calculated to be past the end of the shadow region). Also note that the assertion is guarded by SANITIZER_DEBUG.
@llvmbot
Copy link
Member

llvmbot commented Jul 10, 2024

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

Author: Thurston Dang (thurstond)

Changes

MemoryAccessRangeT overestimates the size of the shadow region by 8x, occasionally leading to assertion failure:

  RawShadow* shadow_mem = MemToShadow(addr);
  ...
  // Check that end of shadow is valid
  if (!IsShadowMem(shadow_mem + size * kShadowCnt - 1)) {
    DCHECK(IsShadowMem(shadow_mem + size * kShadowCnt - 1));

It is erroneous for two separate reasons:

  • it uses kShadowCnt (== 4) instead of kShadowMultiplier (== 2)
  • since shadow_mem is a RawShadow*, pointer arithmetic is multiplied by sizeof(RawShadow) == 4

This patch fixes the calculation, and also improves the debugging information.

The assertion error was observed on a buildbot (https://lab.llvm.org/staging/#/builders/89/builds/656/steps/13/logs/stdio):

Bad shadow addr 0x3000000190bc (7fffffffe85f)
ThreadSanitizer: CHECK failed: tsan_rtl_access.cpp:690 "((IsShadowMem(shadow_mem + size * kShadowCnt - 1))) != (0)" (0x0, 0x0) (tid=2202676)

Notice that 0x3000000190bc is not the correct shadow for the end address 0x7fffffffe85f.

This error is more commonly observed on high-entropy ASLR systems, since ASLR may be disabled (if the randomized memory layout is incompatible), leading to an allocation near the boundaries of the high app memory region (and therefore a shadow end that may be erroneously calculated to be past the end of the shadow region). Also note that the assertion is guarded by SANITIZER_DEBUG.


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

1 Files Affected:

  • (modified) compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp (+16-8)
diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
index 8b20984a01000..d03b1863735df 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
@@ -672,22 +672,30 @@ void MemoryAccessRangeT(ThreadState* thr, uptr pc, uptr addr, uptr size) {
 
 #if SANITIZER_DEBUG
   if (!IsAppMem(addr)) {
-    Printf("Access to non app mem %zx\n", addr);
+    Printf("Access to non app mem start: 0x%zx\n", addr);
     DCHECK(IsAppMem(addr));
   }
   if (!IsAppMem(addr + size - 1)) {
-    Printf("Access to non app mem %zx\n", addr + size - 1);
+    Printf("Access to non app mem end: 0x%zx\n", addr + size - 1);
     DCHECK(IsAppMem(addr + size - 1));
   }
   if (!IsShadowMem(shadow_mem)) {
-    Printf("Bad shadow addr %p (%zx)\n", static_cast<void*>(shadow_mem), addr);
+    Printf("Bad shadow start addr: %p (0x%zx)\n",
+           static_cast<void*>(shadow_mem), addr);
     DCHECK(IsShadowMem(shadow_mem));
   }
-  if (!IsShadowMem(shadow_mem + size * kShadowCnt - 1)) {
-    Printf("Bad shadow addr %p (%zx)\n",
-           static_cast<void*>(shadow_mem + size * kShadowCnt - 1),
-           addr + size - 1);
-    DCHECK(IsShadowMem(shadow_mem + size * kShadowCnt - 1));
+
+  // Be careful to cast to uptr before performing arithmetic.
+  RawShadow* shadow_mem_end = static_cast<RawShadow*>(
+      (void*)(((uptr)shadow_mem) + size * kShadowMultiplier - 1));
+  if (!IsShadowMem(shadow_mem_end)) {
+    Printf("Bad shadow end addr: %p (0x%zx)\n",
+           static_cast<void*>(shadow_mem_end), addr + size - 1);
+    Printf(
+        "Shadow start addr (ok): %p (0x%zx); size: 0x%zx; kShadowMultiplier: "
+        "%zx\n",
+        static_cast<void*>(shadow_mem), addr, size, kShadowMultiplier);
+    DCHECK(IsShadowMem(shadow_mem_end));
   }
 #endif
 

@vitalybuka vitalybuka merged commit 4052de6 into llvm:main Jul 11, 2024
4 of 5 checks passed
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
…lvm#98404)

MemoryAccessRangeT overestimates the size of the shadow region by 8x,
occasionally leading to assertion failure:
```
  RawShadow* shadow_mem = MemToShadow(addr);
  ...
  // Check that end of shadow is valid
  if (!IsShadowMem(shadow_mem + size * kShadowCnt - 1)) {
    DCHECK(IsShadowMem(shadow_mem + size * kShadowCnt - 1));
```
It is erroneous for two separate reasons:
- it uses kShadowCnt (== 4) instead of kShadowMultiplier (== 2)
- since shadow_mem is a RawShadow*, pointer arithmetic is multiplied by
sizeof(RawShadow) == 4

This patch fixes the calculation, and also improves the debugging
information.

The assertion error was observed on a buildbot
(https://lab.llvm.org/staging/#/builders/89/builds/656/steps/13/logs/stdio):
```
Bad shadow addr 0x3000000190bc (7fffffffe85f)
ThreadSanitizer: CHECK failed: tsan_rtl_access.cpp:690 "((IsShadowMem(shadow_mem + size * kShadowCnt - 1))) != (0)" (0x0, 0x0) (tid=2202676)
```
Notice that 0x3000000190bc is not the correct shadow for the end address
0x7fffffffe85f.

This error is more commonly observed on high-entropy ASLR systems, since
ASLR may be disabled (if the randomized memory layout is incompatible),
leading to an allocation near the boundaries of the high app memory
region (and therefore a shadow end that may be erroneously calculated to
be past the end of the shadow region). Also note that the assertion is
guarded by SANITIZER_DEBUG.

---------

Co-authored-by: Vitaly Buka <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants