This is an archive of the discontinued LLVM Phabricator instance.

[sanitizers][Windows] implement __sanitizer_purge_allocator for Win64
ClosedPublic

Authored by mcgov on Feb 2 2021, 12:43 PM.

Details

Summary

On Windows memory is never released once it's mapped with VirtualAlloc. This isn't a huge deal for many applications but with LibFuzzer this can lead to OOM's quite quickly.

The memory management model for the sanitizer allocator doesn't seem to quite fit the needs of Windows in this case, I think the assumption is that pages will be reclaimed and remapped as needed by the OS when they are touched. This patch attempts to unmap and remap pages, I'm putting it up for comment and review, I'm guessing this will need some additional work to avoid putting Windows code in a shared section.

Thank you everyone for your time, attention, and expertise!

patch authored by mcgov and Jordyn Puryear

Diff Detail

Event Timeline

mcgov requested review of this revision.Feb 2 2021, 12:43 PM
mcgov created this revision.
vitalybuka added inline comments.Feb 2 2021, 1:26 PM
compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
152

MmapFixedOrDie can't return nulltpr

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
337–343

Isn't sanitizers on windows are always 64?

338

RoundUpTo/RoundDownTo?

mcgov updated this revision to Diff 321483.Feb 4 2021, 9:20 AM

added purge test, updated for Vitaly's comments

mcgov marked 3 inline comments as done.Feb 4 2021, 9:20 AM
mcgov updated this revision to Diff 321547.Feb 4 2021, 12:39 PM

update test to only apply to x64 (we still build x86)

vitalybuka accepted this revision.Feb 4 2021, 12:47 PM
vitalybuka added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
337–339

I see no reason to split declaration from initialization.

compiler-rt/test/asan/TestCases/Windows/sanitizer_purge.cpp
6–8

We usually put RUN: before includes

and
// RUN: %clang_cl_asan -Od %s -Fe%t && %t
is a little bit more convenient for copying reproducer line from failed test output

17–31

please clang-format

This revision is now accepted and ready to land.Feb 4 2021, 12:47 PM
mcgov updated this revision to Diff 321553.Feb 4 2021, 1:16 PM

nit, style fix

mcgov added a comment.Feb 4 2021, 1:22 PM

Thank you Vitaly! I'm going to wait on a review from @mstorsjo to land this

Seems to build fine in my configs, so no objections from me - thanks for looping me in!

Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2023, 12:05 AM
Herald added a subscriber: Enna1. · View Herald Transcript