This is an archive of the discontinued LLVM Phabricator instance.

Differential D58988

[Sanitizer] Protect UnwindFast with IsValidFrame on MIPS
Needs RevisionPublic

Authored by yln on Mar 5 2019, 11:24 AM.

Details

Reviewers
vitalybuka
eugenis
Summary

I think we changed what we protect with IsValidFrame over the course of
our refactorings. Take a look at the code before we started:
https://github.com/llvm/llvm-project/blob/2946cd701067404b99c39fb29dc9c74bd7193eb3/compiler-rt/lib/asan/asan_stack.h

Note that the code path that can potentially result in a fast unwind is
protected by IsValidFrame (for MIPS). This makes sense, since fast
unwind requires the frame info (bp, stack_top, stack_bottom) to work.
The current code only protects the slow unwind.

Diff Detail

Event Timeline

yln created this revision.Mar 5 2019, 11:24 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 5 2019, 11:24 AM
Herald added subscribers: llvm-commits, Restricted Project, jdoerfert and 4 others. · View Herald Transcript
Harbormaster completed remote builds in B28809: Diff 189375.Mar 5 2019, 11:26 AM
vitalybuka added inline comments.Mar 5 2019, 11:31 AM
compiler-rt/lib/asan/asan_stack.cc
68

Previously It protected both.
However original commit for this check mentions that MIPS use only Slow and it lacks IsValid check.

vitalybuka added inline comments.Mar 5 2019, 11:32 AM
compiler-rt/lib/asan/asan_stack.cc
68

https://reviews.llvm.org/D18690?vs=on&id=52346&whitespace=ignore-most#toc

yln marked an inline comment as done.Mar 5 2019, 12:26 PM
yln added inline comments.
compiler-rt/lib/asan/asan_stack.cc
68

Just to make sure I understand:
MIPS only uses slow unwinder, which doesn't work when !IsValidFrame, despite it not explicitly taking (bp, top, bottom) as inputs?

To me, that is extremely confusing.

Should we try to make MIPS slow unwinder work (as in: return early without error) for !IsValidFrame? Instead of working around the symptom here?

vitalybuka added inline comments.Mar 5 2019, 12:50 PM
compiler-rt/lib/asan/asan_stack.cc
68

Correct.
It would be nice if you have time for this.

vitalybuka requested changes to this revision.Apr 24 2019, 11:11 AM
This revision now requires changes to proceed.Apr 24 2019, 11:11 AM

Revision Contents

PathSize
compiler-rt/
lib/
asan/
6 lines

Diff 189375

compiler-rt/lib/asan/asan_stack.cc

Show First 20 Lines • Show All 59 Lines • ▼ Show 20 Linesvoid __sanitizer::BufferedStackTrace::UnwindImpl(
if (UNLIKELY(!asan_inited)) if (UNLIKELY(!asan_inited))
return; return;
request_fast = StackTrace::WillUseFastUnwind(request_fast); request_fast = StackTrace::WillUseFastUnwind(request_fast);
AsanThread *t = GetCurrentThread(); AsanThread *t = GetCurrentThread();
ScopedUnwinding unwind_scope(t); ScopedUnwinding unwind_scope(t);
if (!unwind_scope.CanUnwind()) if (!unwind_scope.CanUnwind())
return; return;
if (request_fast) { if (request_fast) {
if (t) { if (t && (!SANITIZER_MIPS ||
vitalybukaUnsubmitted
Not Done
ReplyInline Actions

Previously It protected both.
However original commit for this check mentions that MIPS use only Slow and it lacks IsValid check.

vitalybuka: Previously It protected both. However original commit for this check mentions that MIPS use…
vitalybukaUnsubmitted
Not Done
ReplyInline Actions

https://reviews.llvm.org/D18690?vs=on&id=52346&whitespace=ignore-most#toc

vitalybuka: https://reviews.llvm.org/D18690?vs=on&id=52346&whitespace=ignore-most#toc
ylnAuthorUnsubmitted
Done
ReplyInline Actions

Just to make sure I understand:
MIPS only uses slow unwinder, which doesn't work when !IsValidFrame, despite it not explicitly taking (bp, top, bottom) as inputs?

To me, that is extremely confusing.

Should we try to make MIPS slow unwinder work (as in: return early without error) for !IsValidFrame? Instead of working around the symptom here?

yln: Just to make sure I understand: MIPS only uses slow unwinder, which doesn't work when !
vitalybukaUnsubmitted
Not Done
ReplyInline Actions

Correct.
It would be nice if you have time for this.

vitalybuka: Correct. It would be nice if you have time for this.
IsValidFrame(bp, t->stack_top(), t->stack_bottom()))) {
Unwind(max_depth, pc, bp, nullptr, t->stack_top(), t->stack_bottom(), Unwind(max_depth, pc, bp, nullptr, t->stack_top(), t->stack_bottom(),
true); true);
} }
return; return;
} }
if (SANITIZER_MIPS && t &&
!IsValidFrame(bp, t->stack_top(), t->stack_bottom()))
return;
Unwind(max_depth, pc, bp, context, 0, 0, false); Unwind(max_depth, pc, bp, context, 0, 0, false);
} }
// ------------------ Interface -------------- {{{1 // ------------------ Interface -------------- {{{1
extern "C" { extern "C" {
SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_INTERFACE_ATTRIBUTE
void __sanitizer_print_stack_trace() { void __sanitizer_print_stack_trace() {
using namespace __asan; using namespace __asan;
PRINT_CURRENT_STACK(); PRINT_CURRENT_STACK();
} }
} // extern "C" } // extern "C"