[compiler-rt] [TSan] Fix potentially problematic shadow end calculations (PR #144648)

Kunqiu Chen via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 18 01:26:50 PDT 2025


Camsyn wrote:

> 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:
```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:
```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.



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


More information about the llvm-commits mailing list