This is an archive of the discontinued LLVM Phabricator instance.

[libFuzzer] Report at most one crash per input.
ClosedPublic

Authored by morehouse on Apr 30 2018, 11:15 AM.

Details

Summary

Fixes https://github.com/google/sanitizers/issues/788/, a deadlock
caused by multiple crashes happening at the same time. Before printing
a crash report, we now test and set an atomic flag. If the flag was
already set, the crash handler returns immediately.

Diff Detail

Repository
rL LLVM

Event Timeline

morehouse created this revision.Apr 30 2018, 11:15 AM
kcc added inline comments.May 1 2018, 12:19 PM
compiler-rt/lib/asan/asan_report.cc
137 ↗(On Diff #144595)

Will check-asan pass with this?
__sanitizer_acquire_crash_state is weak, and is not defined w/o libFuzzer, so you should get a null deref here. No?

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp
382 ↗(On Diff #144595)

I was thinking about implementing this function in sanitizer_common, and not making it weak.

morehouse added inline comments.May 1 2018, 12:26 PM
compiler-rt/lib/asan/asan_report.cc
137 ↗(On Diff #144595)

check-asan passes. This is defined weakly in sanitizer_common.cc, so it shouldn't be a nullptr.

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp
382 ↗(On Diff #144595)

I defined it weakly in sanitizer_common.cc so that recovery mode shouldn't be affected.

kcc added inline comments.May 1 2018, 12:51 PM
compiler-rt/lib/asan/asan_report.cc
137 ↗(On Diff #144595)

I would rather define this function (non-weak) in sanitizer_common and check halt_on_error_ before calling it there.
This will make it work with the recovery mode. Right?

morehouse updated this revision to Diff 144770.May 1 2018, 1:16 PM
  • Define __sanitizer_acquire_crash_state() in sanitizer_common.
morehouse marked an inline comment as done.May 1 2018, 1:16 PM
kcc added inline comments.May 1 2018, 1:21 PM
compiler-rt/lib/sanitizer_common/sanitizer_common.cc
353 ↗(On Diff #144770)

redundant empty line

compiler-rt/lib/sanitizer_common/sanitizer_interface_internal.h
59 ↗(On Diff #144770)

Since this is used by libFuzzer, it's not an internal interface, but a public one, so move it to include/sanitizer/common_interface_defs.h
Also, make it return int, to make more C-friendly, update the comment accordingly.

compiler-rt/test/fuzzer/AcquireCrashStateTest.cpp
10 ↗(On Diff #144770)

Instead of this, include sanitizer/common_interface_defs.h

morehouse updated this revision to Diff 144776.May 1 2018, 1:44 PM
  • Add __sanitizer_acquire_crash_state to public interface.
morehouse marked 3 inline comments as done.May 1 2018, 1:46 PM
morehouse added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_interface_internal.h
59 ↗(On Diff #144770)

Still kept this declaration also, following the pattern of other public interfaces that are defined internally.

kcc accepted this revision.May 1 2018, 1:49 PM

LGTM

This revision is now accepted and ready to land.May 1 2018, 1:49 PM
This revision was automatically updated to reflect the committed changes.
morehouse marked an inline comment as done.