This is an archive of the discontinued LLVM Phabricator instance.

[sanitizers][windows] Rtl-Heap Interception and tests
ClosedPublic

Authored by mcgov on Jun 5 2019, 12:35 PM.

Details

Summary

Adds interceptors for Rtl(Allocate|Free|Size|ReAllocate)Heap family of functions on windows. Hooking these requires more thorough hooking of the Heap(Alloc|Free|Size|ReAlloc) family. This feature is hidden behind a CMake configuration variable since it will incur a performance penalty due to many calls to RtlValidateHeap and HeapValidate.

The tests that exercise this code are also hidden with an environment variable that will add an available feature. This is a hack since I'm not sure the best way to turn these on and off based on the cmake configuration. I am super open to suggestions on a better way to do this :)

Diff Detail

Event Timeline

mcgov created this revision.Jun 5 2019, 12:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2019, 12:35 PM
mcgov updated this revision to Diff 203225.Jun 5 2019, 12:36 PM
mcgov updated this revision to Diff 203441.Jun 6 2019, 1:53 PM

updating to mask RTL interception from x64 (not supported yet)

mcgov updated this revision to Diff 203844.Jun 10 2019, 9:25 AM
mcgov planned changes to this revision.Jun 19 2019, 2:12 PM

Modifying to make this option selectable at runtime. This makes it cleaner, without needing config and env var hacks

mcgov updated this revision to Diff 205834.Jun 20 2019, 9:02 AM

Switches CMake config and environment variable to turn tests on to an ASAN_OPTIONS runtime variable. This makes the activation and testing cleaner, as well as the code itself.

vitalybuka added inline comments.Jun 20 2019, 11:03 AM
compiler-rt/lib/asan/asan_malloc_win.cc
53

can you please upload the patch with "arc" tool, otherwise context is not visible

209

Not sure I understand this "if".
Can we get here when windows_hook_rtl_allocators==false?
If true, I don't see much difference after asan_inited set to true.

211

can you please clang-format the patch?

218

I'd expect this check is needed unconditionally, we can get to asan_malloc_usable_size even "if-then" about was executed.

462

Isn't better to use REAL in cases above instead of CHECK(Flags...) ?

mcgov marked 4 inline comments as done.Jun 20 2019, 11:46 AM
mcgov added inline comments.
compiler-rt/lib/asan/asan_malloc_win.cc
53

will do

209

These if's are handling some edge cases when you hook the RTL allocators. There are some allocations which will occur before interception happens, so you have to check where the allocation came from.

I agree these look pretty gross and aren't super clear. I'll see if I can clean it up or write some comments to make them obvious.

211

I have my editor set to format on save/paste but something must have gone wrong. Will make sure the new sections are formatted properly

218

Oh good point, I'll check assertions again file-wide.

462

I need more context on what you mean here, where would it be better?

mcgov updated this revision to Diff 205885.Jun 20 2019, 1:14 PM

Code cleaning, adding comments for clarity

mcgov marked 2 inline comments as done and an inline comment as not done.Jun 20 2019, 1:19 PM

addressing comments from @vitalybuka

compiler-rt/lib/asan/asan_malloc_win.cc
209

I've cleaned these up, they look a little weird at first but the new code makes sure to split up unsupported flags at allocation time when the runtime flag is set. In that case it is enough to check who owns the allocation, since the user can allocate a memory section without the unsupported flags but then use them on a later call to HeapSize or HeapReAlloc. When the RTL interception is turned on it's enough to just check who owns it, since if the user wants to use HEAP_NO_SERIALIZE on Free or Size it will just get ignored in the event ASAN owns the allocation. If the allocation is passed into the RTL library it will pass the flag through.

If the flag is turned off we keep the original behavior of asserting on those illegal flags, since there is no falling back to the original allocator in that version.

That's a lot, does that make sense or am I talking myself into garbage lol

vitalybuka added inline comments.Jun 20 2019, 1:21 PM
compiler-rt/lib/asan/asan_malloc_win.cc
218

this is marked as done, but I don't see a difference.

I expected something like:

if (flags()->windows_hook_rtl_allocators) {
  ...
  REAL...
}
CHECK(dwFlags == 0 && "unsupported heap flags");

without else

mcgov added inline comments.Jun 20 2019, 1:46 PM
compiler-rt/lib/asan/asan_malloc_win.cc
218

Right, I don't think this is actually the behavior that's needed though. It's the same reason as in HeapFree, when the RTL interception is turned on the check for incomaptible flags occurs only on (re)allocation. If the user is trying to pass unsupported flags to Size or Free for an allocation that's owned by the ASAN allocator we still want that size or free to come back to the user code, reguardless of where it lives. It's a weird situation that comes from juggling the original and asan allocator.

@vitalybuka Heya, not marking as done but need some feedback from you.

mcgov marked 6 inline comments as done.Jun 24 2019, 12:36 PM

Another bump for review from @rnk or @vitalybuka

The goals of this patch were to enable RTL interception and preserve the current functionality when the option is turned off. This is why there are some weird IF conditions that seem like they should be global. To get the RTL interception to work the way you would expect it required changing some of the ways flags that are passed into the Allocate/Free/ReAllocate/Size functions are handled. This may not be worth much since you can't see the test results (for now) but we have been using this patch internally for a few months now :-)

vitalybuka marked an inline comment as done.Jun 24 2019, 1:31 PM
vitalybuka added inline comments.
.gitignore
48 ↗(On Diff #205885)

Separate patch?

compiler-rt/lib/asan/asan_malloc_win.cc
39

Why these and new constants are macros and not regular consts or constexpr?

277

Leave this to compiler?

template<class ReAllocFunction...> 
static void* SharedReAlloc(ReAllocFunction reallocFunc, ...
280

this should be static on in namespace {}

280

void* SharedReAlloc?

462

Ignore my comment, now I understand your approach.

compiler-rt/lib/asan/asan_win.cc
181

Still shows "Context not available."

218

could you please use consts?

mcgov updated this revision to Diff 206488.Jun 25 2019, 11:25 AM
mcgov marked 4 inline comments as done.

addressing comments from @vitalybuka
changing macros to constexpr
moving private stuff into asan namespace

Addressing comments

.gitignore
48 ↗(On Diff #205885)

Sure, I can wrap this up with some other msvc host compat stuff (coming soon)

compiler-rt/lib/asan/asan_malloc_win.cc
39

No reason, I'll switch them

277

I did try this solution first, but we're stuck here because the template parameters would end up being pointers that are not compile-time constants. The arguments that are being pointed to are not the regular HeapAlloc address (which get patched), it's the pointers to the REAL(HeapAlloc) which are not known until hooking is finished.

The templated solution is cleaner but doesn't work in this situation. I will address these other concerns though it should be in the namespace. Thank you for the feedback!

compiler-rt/lib/asan/asan_win.cc
218

been reading too much old win32 code sorry 😂
will fix!

mcgov marked 2 inline comments as done.Jun 28 2019, 3:52 PM

updated consexpr and namespace issue

mcgov added a comment.Jul 3 2019, 10:00 AM

@rnk @vitalybuka

Just pinging again for review to land this if possible.

rnk added a comment.Jul 3 2019, 11:36 AM

I reviewed the code and mostly came up with surface formatting issues. Functionally I think it looks good and the only way to really find out if it works is to ship it. :) So, I think we should fix the surface issues and try to move forward with that.

compiler-rt/lib/asan/asan_malloc_win.cc
269

Please capitalize the type name of the enum, so AllocationOwnership.

280

This is used in one place, I would sink it down to the use.

284

This is only used if RTL allocator hooking is enabled, I'd sink it into the if.

compiler-rt/lib/asan/asan_win.cc
0–1

I think it would be simpler to drop this check and always do the IsTlsInitialized() check. I understand that adding the new heap interceptors is what causes us to run code prior to TLS initialization, but in the future ASan could change again in some other way that causes us to clal ASanTSDGet early and removing an if and defending against that possibility seems good.

Also, this isn't indented correctly, please fix that before committing.

0–1

There should be a space between the ){. I'd recommend using git-clang-format or some other tool to run clang-format to fix these minor issues.

In D62927#1568926, @rnk wrote:

I reviewed the code and mostly came up with surface formatting issues. Functionally I think it looks good and the only way to really find out if it works is to ship it. :) So, I think we should fix the surface issues and try to move forward with that.

LGTM, @rnk please accept when ready

mcgov updated this revision to Diff 207888.Jul 3 2019, 2:11 PM
mcgov marked 4 inline comments as done.

Updating for RNK comments

rnk accepted this revision.Jul 3 2019, 2:37 PM

lgtm

This revision is now accepted and ready to land.Jul 3 2019, 2:37 PM
jfb added a comment.Jul 8 2019, 1:21 PM

Reverted in https://reviews.llvm.org/rL365384

because of: http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/28842/steps/build%20stage%201/logs/stdio

/home/buildbots/ppc64be-clang-lnt-test/clang-ppc64be-lnt/llvm/projects/compiler-rt/lib/asan/asan_malloc_win.cc:23:2: error: #error "Missing arch or unsupported platform for Windows."
 #error "Missing arch or unsupported platform for Windows."
  ^~~~~
/home/buildbots/ppc64be-clang-lnt-test/clang-ppc64be-lnt/llvm/projects/compiler-rt/lib/asan/asan_malloc_win.cc:25:10: fatal error: heapapi.h: No such file or directory
 #include <heapapi.h>
          ^~~~~~~~~~~
compilation terminated.
[39/1151] Building CXX object projects/compiler-rt/lib/asan/CMakeFiles/RTAsan.powerpc64.dir/asan_debugging.cc.o
[40/1151] Building CXX object projects/compiler-rt/lib/asan/CMakeFiles/RTAsan.powerpc64.dir/asan_malloc_win.cc.o
FAILED: projects/compiler-rt/lib/asan/CMakeFiles/RTAsan.powerpc64.dir/asan_malloc_win.cc.o 
/usr/bin/c++  -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/asan -I/home/buildbots/ppc64be-clang-lnt-test/clang-ppc64be-lnt/llvm/projects/compiler-rt/lib/asan -Iinclude -I/home/buildbots/ppc64be-clang-lnt-test/clang-ppc64be-lnt/llvm/include -I/home/buildbots/ppc64be-clang-lnt-test/clang-ppc64be-lnt/llvm/projects/compiler-rt/lib/asan/.. -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -std=c++11 -Wno-unused-parameter -O2    -UNDEBUG  -m64 -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fvisibility=hidden -fno-lto -O3 -g -Wno-variadic-macros -Wno-non-virtual-dtor -fno-rtti -MD -MT projects/compiler-rt/lib/asan/CMakeFiles/RTAsan.powerpc64.dir/asan_malloc_win.cc.o -MF projects/compiler-rt/lib/asan/CMakeFiles/RTAsan.powerpc64.dir/asan_malloc_win.cc.o.d -o projects/compiler-rt/lib/asan/CMakeFiles/RTAsan.powerpc64.dir/asan_malloc_win.cc.o -c /home/buildbots/ppc64be-clang-lnt-test/clang-ppc64be-lnt/llvm/projects/compiler-rt/lib/asan/asan_malloc_win.cc
/home/buildbots/ppc64be-clang-lnt-test/clang-ppc64be-lnt/llvm/projects/compiler-rt/lib/asan/asan_malloc_win.cc:23:2: error: #error "Missing arch or unsupported platform for Windows."
 #error "Missing arch or unsupported platform for Windows."
  ^~~~~
/home/buildbots/ppc64be-clang-lnt-test/clang-ppc64be-lnt/llvm/projects/compiler-rt/lib/asan/asan_malloc_win.cc:25:10: fatal error: heapapi.h: No such file or directory
 #include <heapapi.h>
          ^~~~~~~~~~~
rnk added a comment.Jul 8 2019, 1:51 PM
In D62927#1574273, @jfb wrote:

Reverted in https://reviews.llvm.org/rL365384

because of: http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/28842/steps/build%20stage%201/logs/stdio

/home/buildbots/ppc64be-clang-lnt-test/clang-ppc64be-lnt/llvm/projects/compiler-rt/lib/asan/asan_malloc_win.cc:23:2: error: #error "Missing arch or unsupported platform for Windows."
 #error "Missing arch or unsupported platform for Windows."
  ^~~~~
/home/buildbots/ppc64be-clang-lnt-test/clang-ppc64be-lnt/llvm/projects/compiler-rt/lib/asan/asan_malloc_win.cc:25:10: fatal error: heapapi.h: No such file or directory
 #include <heapapi.h>
          ^~~~~~~~~~~

This should be easy to fix, the entire asan_malloc_win.cc file should be encapsulated in the #if SANITIZER_WINDOWS block, including this.

I would recommend setting up a build and test environment on Linux to flush out these issues prior to committing in the future, since ASan is very platform sensitive. Once you get that working, feel free to reland when you get a chance.

rnk added a comment.Jul 8 2019, 1:57 PM

I noticed the new tests also failed on the sanitizer-windows bot:
http://lab.llvm.org:8011/builders/sanitizer-windows/builds/48066/steps/stage%201%20check/logs/stdio

I haven't looked into it much, but it would be good to investigate that before trying again.

Sorry for not testing out this commit until it hit the tree and not commenting on this before. This one turned out to break MinGW builds for a few different reasons, that I'll comment on inline.

Unrelated point: Please close the review on/after committing; this is easily handled by mentioning the review in the commit message in the form Differential Revision: https://reviews.llvm.org/D62927. In this case I was surprised why this could have broken anything as the review still wasn't closed.

If fixing these cases with the issues I'm pointing out is too cumbersome, I wouldn't mind adding a large #ifndef __MINGW32__ around the new code. But the release branch is approaching fast, and I'd like to have address sanitizer at least buildable for MinGW in the release (even though issues can be fixed after the branch as well) - having these new interceptors usable is less of a deal.

compiler-rt/lib/asan/asan_malloc_win.cc
18

This needs an || defined(__i386__) for MinGW, and same with defined(__x86_64__) below. And changing the #define _X86_ into #define _X86_ 1 avoids a warning about redefinition to a different value.

25

This file implicitly includes a lot of windows specific headers. See the comment below about "Intentionally not including windows.h here"; this brings back the same issue that was fixed earlier by removing the include of windows.h here, in particular errors like these:

compiler-rt/lib/asan/asan_malloc_win.cc:84:6: error: redeclaration of 'free' cannot add 'dllexport' attribute
void free(void *ptr) {
     ^

To avoid this, we need to make sure we don't see any inline function that uses the function free up to this point, when we redefine it with new attributes.

344

In MinGW, min() isn't defined by the headers included so far. I guess that could be considered a MinGW header bug (it's not defined if __cplusplus is defined - fixing that if necessary should hopefully be doable), but the issue mentioned above about explicitly avoiding windows platform headers might also lead to it being unavailable in MSVC style builds.

407

_Frees_ptr_opt_ is not available/defined in MinGW - can it be left out here, or does that cause errors about mismatching attributes in an MSVC build?

mcgov closed this revision.Jul 12 2019, 9:01 PM
mcgov marked 8 inline comments as done.

@mrstojo no worries! I'm very new and still learning how to be a good contributor so I appreciate feedback on what I'm doing wrong. I will remove the heapapi include and replace with just the function prototypes I needed along with those constants.

I'm closing this review and will open a new one for the mingw32 changes

compiler-rt/lib/asan/asan_malloc_win.cc
18

Will just yank this, we don't need the full include.

25

Whoops! will just replace with the items I need. We should need the function prototypes so if that's correct I'll just yank them out of the header.

344

no trouble, will replace

407

Nope, will remove this. My guess is this is some static analysis macro actually that resolves to nothing anyway, I just missed this was here since it's defined somewhere in that headerm so it shouldn't hurt anything to remove.