This is an archive of the discontinued LLVM Phabricator instance.

[hwasan] Record and display stack history in stack-based reports.
ClosedPublic

Authored by eugenis on Sep 18 2018, 2:35 PM.

Details

Summary

Display a list of recent stack frames (not a stack trace!) when
tag-mismatch is detected on a stack address.

The implementation uses alignment tricks to get both the address of
the history buffer, and the base address of the shadow with a single
8-byte load. See the comment in hwasan_thread_list.h for more
details.

Developed in collaboration with Kostya Serebryany.

Diff Detail

Event Timeline

eugenis created this revision.Sep 18 2018, 2:35 PM
eugenis updated this revision to Diff 166039.Sep 18 2018, 2:40 PM

add missing file

kcc accepted this revision.Sep 18 2018, 3:41 PM

LGTM with some nits (feel free to ignore any of them, if they are wrong)

compiler-rt/lib/hwasan/hwasan_interceptors.cc
229 ↗(On Diff #166039)

do you need this change?

compiler-rt/lib/hwasan/hwasan_report.cc
152 ↗(On Diff #166039)

why not just sa->size()?

compiler-rt/lib/hwasan/hwasan_thread_list.h
21 ↗(On Diff #166039)

Is that

starting at (2**kShadowBaseAlignment+1)

?

27 ↗(On Diff #166039)

is this

(a + sizeof(uptr))) & ~mask ?

(~ missing)

compiler-rt/lib/sanitizer_common/sanitizer_ring_buffer.h
81 ↗(On Diff #166039)

short comment?

This revision is now accepted and ready to land.Sep 18 2018, 3:41 PM
eugenis updated this revision to Diff 166204.Sep 19 2018, 5:34 PM
eugenis marked 2 inline comments as done.

.

I've moved initialization code around a little.

compiler-rt/lib/hwasan/hwasan_report.cc
152 ↗(On Diff #166039)

sa->size() is a multiple of 512, the user might prefer less history in their reports

compiler-rt/lib/hwasan/hwasan_thread_list.h
21 ↗(On Diff #166039)

No, the align-up code is only wrong the ringbuffer address == shadow address, which is clearly impossible. I'll remove the -1, it is confusing.

27 ↗(On Diff #166039)

yes, will fix

kcc accepted this revision.Sep 21 2018, 5:46 PM

LGTM with nits.

compiler-rt/lib/hwasan/hwasan_report.cc
39 ↗(On Diff #166579)

plz comment why we need this

compiler-rt/lib/hwasan/hwasan_thread_list.h
22 ↗(On Diff #166579)

can N actually be 0?
If so, please add a run-time test, although I would rather say N is [1,8)

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
718 ↗(On Diff #166579)

10, not 12?

eugenis updated this revision to Diff 166589.Sep 21 2018, 6:15 PM
eugenis marked an inline comment as done.

fixed shift size

compiler-rt/lib/hwasan/hwasan_thread_list.h
22 ↗(On Diff #166579)

(2**0)*4096 == 4096, sure that will work.

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
718 ↗(On Diff #166579)

ouch

eugenis updated this revision to Diff 166758.Sep 24 2018, 2:39 PM

Fixed instrumentation on aarch64-linux-unknown and 1 test case.

This revision was automatically updated to reflect the committed changes.
eugenis reopened this revision.Sep 24 2018, 4:03 PM
This revision is now accepted and ready to land.Sep 24 2018, 4:03 PM
eugenis updated this revision to Diff 166779.Sep 24 2018, 4:04 PM

Tentative fix: s/1UL/1ULL/

This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: Restricted Project. · View Herald TranscriptSep 24 2018, 4:07 PM