[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