This is an archive of the discontinued LLVM Phabricator instance.

[Sanitizer] Fix page alignment for mmap calls
ClosedPublic

Authored by thetruestblue on Dec 19 2022, 4:15 PM.

Details

Summary

We are in the process of enabling sanitizer_common unit tests on arm64 for apple devices. rdar://101436019

The test CompactRingBuffer.int64 is failing on arm64 with the error:

==17265==ERROR: SanitizerTool failed to deallocate 0xfffffffffffff000 (-4096) bytes at address 0x000105c30000  
SanitizerTool: CHECK failed: sanitizer_posix.cpp:63 "(("unable to unmap" && 0)) != (0)" (0x0, 0x0) (tid=157296)

If page size is sufficiently larger than alignment then this code:

UnmapOrDie((void*)end, map_end - end);

end is will be greater than map_end causing the value passed to UnmapOrDie to be negative.

This is caused when GetPageSizeCached returns 16k and alignment is 8k.
map_size and what is mapped by mmap uses size and alignment which is smaller than what is calculated by end using the actual page size.
Therefore, map_end ends up being less than end.
The call to mmap is allocating sufficent page-aligned memory, because it calls RoundUp within MmapOrDieOnFatalError.
But this size is not being captured by map_size.

We can address this by rounding up map_size here to be page-aligned. This ensures that map_end will be greater than or equal to end and that it will match mmaps use of page-aligned value, and the
subsequent call to munmap will also be page-aligned.

Diff Detail

Event Timeline

thetruestblue created this revision.Dec 19 2022, 4:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 19 2022, 4:15 PM
thetruestblue requested review of this revision.Dec 19 2022, 4:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 19 2022, 4:15 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

This fixes the case where map_end is greater than end because of page size and alignment difference.
However, I believe this function could use further refinement, I don't think it is handling "alignment" versus "page-aligned" adequately.
alignment is being treated as a size, rather than a page aligned location, which seems to be what we should be expecting here.
Particularly with the calls to UnmaporDie, which expect a page-aligned value.

vitalybuka accepted this revision.Dec 21 2022, 1:52 PM
vitalybuka added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_posix.cpp
106

Needs {}

This revision is now accepted and ready to land.Dec 21 2022, 1:52 PM
This revision was landed with ongoing or failed builds.Dec 22 2022, 10:26 AM
This revision was automatically updated to reflect the committed changes.
yln added a comment.Dec 22 2022, 3:39 PM

@wrotki: you touched similar code recently. Can you do a post-merge review to convince yourself that this is doing the right thing? Thanks!