This is an archive of the discontinued LLVM Phabricator instance.

Work around crashes in `__sanitizer_malloc_hook()` under Mac OSX.
ClosedPublic

Authored by delcypher on May 18 2016, 4:55 PM.

Details

Summary

[LibFuzzer]
Work around crashes in __sanitizer_malloc_hook() under Mac OSX.

Under Mac OSX we intercept calls to malloc before thread local
storage is initialised leading to a crash when accessing
AllocTracer. To workaround this AllocTracer is only accessed
in the hook under Linux. For symmetry __sanitizer_free_hook()
is also modified in the same way.

To support this change a set of new macros
LIBFUZZER_LINUX and LIBFUZZER_APPLE has been defined which can be
used to check the target being compiled for.

Diff Detail

Repository
rL LLVM

Event Timeline

delcypher updated this revision to Diff 57709.May 18 2016, 4:55 PM
delcypher retitled this revision from to Try to fix libFuzzer running on Mac OSX.
delcypher updated this object.
delcypher added reviewers: kcc, aizatsky.
delcypher added subscribers: llvm-commits, kcc, aizatsky.
kcc edited edge metadata.May 18 2016, 6:32 PM

This is very, very sad.
I intentionally made this thread local so that we don't use atomics in a very hot place (and malloc is often a very hot place).
Besides, I am only collecting counts from the main thread.
Need to find another solution, e.g. to not touch AllocTracer before we called Start() on it.

@kcc

This is very, very sad.
I intentionally made this thread local so that we don't use atomics in a very hot place (and malloc is often a very hot place).

Okay. My implementation isn't very good then

Besides, I am only collecting counts from the main thread.

Okay, so that was intentional. So my implementation isn't just bad, it's also wrong. Is the reason you only want to collect counts from the main thread for the reason I gave in my earlier comment? Or is there another reason?

Need to find another solution, e.g. to not touch AllocTracer before we called Start() on it.

Well there's a really hacky way to do that. There could be a global bool guarding access to AllocTracer that we set to true once we call `Start()`. Not sure if that's ideal though as technically would could race on the global bool if there are multiple threads.

Just a side though. The LeakSanitizer currently doesn't work on Mac OSX. AFAICT it only works right now on Linux that is not Android on x86_64, aarch64 or MIPS (based on reading compiler-rt/lib/lsan/lsan_common.h where CAN_SANITIZE_LEAKS is set). Whether or not the CAN_SANITIZE_LEAKS macro was enabled (and consequently if leak sanitization can be performed) does not seem to be exposed in the Leak Sanitizer's interface. There is the __lsan_is_turned_off() function but that is not affected by the value of the CAN_SANITIZE_LEAKS macro. Ideally LibFuzzer shouldn't try to do anything related to leak checking if the leak sanitizer is not actually functional. If __lsan_is_turned_off()` was fixed to respect CAN_SANITIZE_LEAKS we could guard some of the code in LibFuzzer that supports running the LeakSanitizer.

kcc added a comment.May 18 2016, 10:31 PM

Is the reason you only want to collect counts from the main thread for the reason I gave in my earlier comment? Or is there another reason?

Mostly performance. Note that this is not necessary a good heuristic -- in case we malloc in one thread and free() in another thread
we will get the counts wrong and call lsan too often (and then stop calling it at all).

But, 99% of targets we've tested with libFuzzer are single-threaded, so we simply don't have enough experience with multi-threaded targets.

Need to find another solution, e.g. to not touch AllocTracer before we called Start() on it.

Well there's a really hacky way to do that. There could be a global bool guarding access to AllocTracer that we set to true once we call `Start()`. Not sure if that's ideal though as technically would could race on the global bool if there are multiple threads.

Or we can have a global that we set just once at init time and never unset.
This will solve the problem, right? Then there will be no race.

Just a side though. The LeakSanitizer currently doesn't work on Mac OSX.

AFIACT -- yes.

AFAICT it only works right now on Linux that is not Android on x86_64, aarch64 or MIPS (based on reading compiler-rt/lib/lsan/lsan_common.h where CAN_SANITIZE_LEAKS is set). Whether or not the CAN_SANITIZE_LEAKS macro was enabled (and consequently if leak sanitization can be performed) does not seem to be exposed in the Leak Sanitizer's interface.

True.

There is the __lsan_is_turned_off() function but that is not affected by the value of the CAN_SANITIZE_LEAKS macro. Ideally LibFuzzer shouldn't try to do anything related to leak checking if the leak sanitizer is not actually functional. If __lsan_is_turned_off()` was fixed to respect CAN_SANITIZE_LEAKS we could guard some of the code in LibFuzzer that supports running the LeakSanitizer.

__lsan_is_turned_off is a different thing:

// The user may optionally provide this function to disallow leak checking
// for the program it is linked into (if the return value is non-zero). This
// function must be defined as returning a constant value; any behavior beyond
// that is unsupported.

Maybe for now just guard the code in sanitizer_malloc_hook with if (LIBFUZZER_LINUX)?

delcypher updated this object.May 19 2016, 11:55 AM
delcypher edited edge metadata.
delcypher updated this revision to Diff 57835.May 19 2016, 12:15 PM
delcypher retitled this revision from Try to fix libFuzzer running on Mac OSX to Work around crashes in `__sanitizer_malloc_hook()` under Mac OSX..
delcypher updated this object.

@kcc
Is this new version of the patch acceptable? If so I'll commit it.

My other patches ( http://reviews.llvm.org/D20410 and http://reviews.llvm.org/D20409) will depend on the newly added platform macros in this patch so I will fix those once this patch is committed.

aizatsky added inline comments.May 19 2016, 1:24 PM
lib/Fuzzer/FuzzerLoop.cpp
446 ↗(On Diff #57835)

Maybe if (!LIBFUZZER_APPLY) would be a better condition?

kcc added a comment.May 19 2016, 2:20 PM

Yes, this looks ok...

lib/Fuzzer/FuzzerLoop.cpp
446 ↗(On Diff #57835)

Yea, maybe..
if (!LIBFUZZER_APPLE)

Also, this code tends to not use {} for simple if statements. Same below.

delcypher updated this revision to Diff 57858.May 19 2016, 2:42 PM

Make changes suggest by @kcc and @aizatsky.

Is this good enough now?

kcc accepted this revision.May 19 2016, 2:43 PM
kcc edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.May 19 2016, 2:43 PM
aizatsky accepted this revision.May 19 2016, 2:52 PM
aizatsky edited edge metadata.
This revision was automatically updated to reflect the committed changes.