Page MenuHomePhabricator

[asan] Make GetCurrentThread RTEMS-friendly
ClosedPublic

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

Details

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
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.

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.

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
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: Restricted Project. · View Herald Transcript