This is an archive of the discontinued LLVM Phabricator instance.

Differential D46459

[asan] Make GetCurrentThread RTEMS-friendly
ClosedPublic

Authored by waltl on May 4 2018, 1:48 PM.

Details

Reviewers
vitalybuka
eugenis
alekseyshl
Commits
rGead3b3487bf0: [asan] Make GetCurrentThread RTEMS-friendly
rCRT332888: [asan] Make GetCurrentThread RTEMS-friendly
rL332888: [asan] Make GetCurrentThread RTEMS-friendly
Summary

On RTEMS, system and user code all live in a single binary and address
space. There is no clean separation, and instrumented code may
execute before the ASan run-time is initialized (or after it has been
destroyed).

Currently, GetCurrentThread() may crash if it's called before ASan
run-time is initialized. Make it return nullptr instead.

Similarly, fix __asan_handle_no_return so that it gives up rather than
try something that may crash.

Diff Detail

Event Timeline

waltl created this revision.May 4 2018, 1:48 PM
Herald added a subscriber: kubamracek. · View Herald TranscriptMay 4 2018, 1:48 PM
kcc added a comment.May 4 2018, 2:32 PM

I don't like this.
It adds lots of boiler-plate code that we can not test.

I understand the limitations that you describe in the commit message, but please try to find a solution that doesn't pollute the code with platfom-specific ifs()

waltl updated this revision to Diff 145474.May 7 2018, 8:52 AM

Add guard for fake stack. This was missing in the initial commit, so
even though we may need to redo it I just want to make the patches
consistent first.

Harbormaster completed remote builds in B17778: Diff 145474.May 7 2018, 8:52 AM
waltl updated this revision to Diff 145628.May 7 2018, 8:21 PM

Rely on early init of shadow memory, so now we can remove most checks.
2 are left.

Harbormaster completed remote builds in B17823: Diff 145628.May 7 2018, 8:21 PM
vitalybuka added inline comments.May 17 2018, 1:31 PM
compiler-rt/lib/asan/asan_fake_stack.cc
197 ↗(On Diff #145628)

GetTLSFakeStack probably will return 0 anyway?

compiler-rt/lib/asan/asan_rtl.cc
535 ↗(On Diff #145628)

Same about GetCurrentThread

waltl added inline comments.May 17 2018, 6:01 PM
compiler-rt/lib/asan/asan_fake_stack.cc
197 ↗(On Diff #145628)

GetTLSFakeStack is currently not enabled for RTEMS. But regardless, the problem is that thread locals don't work for instrumented code during booting, before ASan compiler-rt has been initialized. Both this entry point and the one in __asan_handle_no_return eventually access TLS (either through GetTLSFakeStack or GetCurrentThread) which would crash the program.

Maybe I should put the check in GetCurrentThread so that it returns NULL instead of crash when !asan_inited? I can put together a patch based on that.

waltl updated this revision to Diff 147439.May 17 2018, 9:39 PM

Make GetCurrentThread RTEMS-friendly.

waltl retitled this revision from [asan] On RTEMS, checks for asan_inited before entering ASan run-time to [asan] Make GetCurrentThread RTEMS-friendly.May 17 2018, 9:45 PM
waltl edited the summary of this revision. (Show Details)
waltl marked 2 inline comments as done.
waltl added inline comments.
compiler-rt/lib/asan/asan_fake_stack.cc
197 ↗(On Diff #145628)

I uploaded the proposed patch. Please let me know what you think.

alekseyshl accepted this revision.May 18 2018, 10:23 AM
This revision is now accepted and ready to land.May 18 2018, 10:23 AM
vitalybuka added inline comments.May 18 2018, 10:26 AM
compiler-rt/lib/asan/asan_thread.cc
394 ↗(On Diff #147439)

What if you just put here and we avoid platform specific branch

if (UNLIKELY(!asan_inited))
  AsanInitFromRtl();

!asan_inited is not expected here on any other platform

alekseyshl added inline comments.May 18 2018, 10:31 AM
compiler-rt/lib/asan/asan_thread.cc
394 ↗(On Diff #147439)

But it still be a check, still some code executed, even if it is unlikely. With SANITIZER_RTEMS, it will be removed on all platforms, but RTEMS.

The semantics is also wrong, this way it creates an impression that it can happen on all platforms.

vitalybuka added inline comments.May 18 2018, 10:55 AM
compiler-rt/lib/asan/asan_thread.cc
394 ↗(On Diff #147439)

With first part I agree, we don't want additional check here as this can be called quite frequently. So it makes sense to move AsanInitFromRtl() into __asan_handle_no_return

For semantics, I'd rather have as much as possible same function on all platform, even if it handles cases which are not necessary on some of them.

alekseyshl added inline comments.May 18 2018, 11:15 AM
compiler-rt/lib/asan/asan_thread.cc
394 ↗(On Diff #147439)

From the patch comment it can be called "after ASan run-time has been destroyed". Why would we want to call AsanInitFromRtl again?

waltl marked an inline comment as done.May 18 2018, 11:17 AM
waltl added inline comments.
compiler-rt/lib/asan/asan_thread.cc
394 ↗(On Diff #147439)

Unfortunately it's not safe to call AsanInitFromRtl() under these conditions. It could be before pthreads have been initialized.

waltl added inline comments.May 18 2018, 11:24 AM
compiler-rt/lib/asan/asan_thread.cc
394 ↗(On Diff #147439)

And yes, the issue also applies after run-time destruction.

vitalybuka accepted this revision.May 21 2018, 1:44 PM
Closed by commit rL332888: [asan] Make GetCurrentThread RTEMS-friendly (authored by waltl). · Explain WhyMay 21 2018, 1:48 PM
Closed by commit rCRT332888: [asan] Make GetCurrentThread RTEMS-friendly (authored by waltl). · Explain Why
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: delcypher. · View Herald TranscriptMay 21 2018, 1:48 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Revision Contents

PathSize
lib/
asan/
3 lines
3 lines

Diff 147863

lib/asan/asan_rtl.cc

Show First 20 Lines • Show All 530 Lines • ▼ Show 20 Linesvoid NOINLINE __asan_handle_no_return() {
int local_stack; int local_stack;
AsanThread *curr_thread = GetCurrentThread(); AsanThread *curr_thread = GetCurrentThread();
uptr PageSize = GetPageSizeCached(); uptr PageSize = GetPageSizeCached();
uptr top, bottom; uptr top, bottom;
if (curr_thread) { if (curr_thread) {
top = curr_thread->stack_top(); top = curr_thread->stack_top();
bottom = ((uptr)&local_stack - PageSize) & ~(PageSize - 1); bottom = ((uptr)&local_stack - PageSize) & ~(PageSize - 1);
} else if (SANITIZER_RTEMS) {
// Give up On RTEMS.
return;
} else { } else {
CHECK(!SANITIZER_FUCHSIA); CHECK(!SANITIZER_FUCHSIA);
// If we haven't seen this thread, try asking the OS for stack bounds. // If we haven't seen this thread, try asking the OS for stack bounds.
uptr tls_addr, tls_size, stack_size; uptr tls_addr, tls_size, stack_size;
GetThreadStackAndTls(/*main=*/false, &bottom, &stack_size, &tls_addr, GetThreadStackAndTls(/*main=*/false, &bottom, &stack_size, &tls_addr,
&tls_size); &tls_size);
top = bottom + stack_size; top = bottom + stack_size;
} }
Show All 33 Lines

lib/asan/asan_thread.cc

Show First 20 Lines • Show All 388 Lines • ▼ Show 20 Linesstatic bool ThreadStackContainsAddress(ThreadContextBase *tctx_base,
if (!t) return false; if (!t) return false;
if (t->AddrIsInStack((uptr)addr)) return true; if (t->AddrIsInStack((uptr)addr)) return true;
if (t->has_fake_stack() && t->fake_stack()->AddrIsInFakeStack((uptr)addr)) if (t->has_fake_stack() && t->fake_stack()->AddrIsInFakeStack((uptr)addr))
return true; return true;
return false; return false;
} }
AsanThread *GetCurrentThread() { AsanThread *GetCurrentThread() {
if (SANITIZER_RTEMS && !asan_inited)
return nullptr;
AsanThreadContext *context = AsanThreadContext *context =
reinterpret_cast<AsanThreadContext *>(AsanTSDGet()); reinterpret_cast<AsanThreadContext *>(AsanTSDGet());
if (!context) { if (!context) {
if (SANITIZER_ANDROID) { if (SANITIZER_ANDROID) {
// On Android, libc constructor is called _after_ asan_init, and cleans up // On Android, libc constructor is called _after_ asan_init, and cleans up
// TSD. Try to figure out if this is still the main thread by the stack // TSD. Try to figure out if this is still the main thread by the stack
// address. We are not entirely sure that we have correct main thread // address. We are not entirely sure that we have correct main thread
// limits, so only do this magic on Android, and only if the found thread // limits, so only do this magic on Android, and only if the found thread
▲ Show 20 Lines • Show All 122 Lines • Show Last 20 Lines