Page MenuHomePhabricator

Improve crash unwinding on Fuchsia
ClosedPublic

Authored by aarongreen on Jun 22 2018, 4:53 PM.

Details

Summary

Fuchsia doesn't have signals; instead it expects processes to have a dedicated exception thread that binds to the process' exception port and waits for exception packets to be delivered. On the other hand, libFuzzer and sanitizer_common use expect to collect crash information via libunwind from the same thread that caused the exception.

The long term fix is to improve support for remote unwinding in libunbwind, plumb this through sanitizer_common and libFuzzer, and handle the exception exclusively on the exception thread. In the meantime, this revision has the exception thread "resurrect" the crashing thread by:

  • saving its general purpose register state onto the crashing thread's stack,
  • setting the crashing thread's program counter to an assembly trampoline with the CFI information needed by libunwind, and
  • resuming the crashed thread.

Diff Detail

Event Timeline

aarongreen created this revision.Jun 22 2018, 4:53 PM
Herald added subscribers: Restricted Project, llvm-commits, chrib. · View Herald Transcript
phosek added inline comments.Jun 25 2018, 5:51 PM
lib/fuzzer/FuzzerUtilFuchsia.cpp
82–83

Do you care about atexit hooks being executed? I think this should probably use _exit instead.

187

I'd use an explicit #elif defined(__aarch64__) and #error in the #else case. You never know when someone tries to compile this on some other architecture.

aarongreen marked 2 inline comments as done.

Addressed Petr's comment, and modified the visibility of CrashTrampolineAsm and MakeTrampoline to suppress their warnings.

Almost there, but some nits.

lib/fuzzer/FuzzerUtilFuchsia.cpp
45

Clarify in the comment that this appears to have external linkage to C++ and that's why it's not in the anonymous namespace, but the assembly definition inside MakeTrampoline(), below, actually defines the symbol with internal linkage only.

63

You don't want this printf for the success case in production code.

84

typo: s/and/an/

86

typo: trailing slash should be period

91

Alternatively, Fuchsia may in future actually implement basic signal handling for the machine trap signals.

144

lr is not special, it's just an alias for x30 so I'd use OP_NUM(30) here.
Only sp (and pc) are really special.

159

This can use offsetof(..., r[num]) instead of local arithmetic. It's not standard but it works in Clang and GCC and always will.

165

Make this static (no extern "C") and pass it into the asm.

175

Clarify that this function is never really used as such and that's why the attribute is necessary.
It's just a container around the asm so it can use operands for compile-time computed constants.

178

No need for a variable here.

183

This can be ".cfi_startproc simple" because you supply all the details explicitly.

189

This can be "call %c[StaticCrashHandler]" with the operand being "i' (StaticCrashHandler).

196

This can be "bl %[StaticCrashHandler]" with the operand being "i' (StaticCrashHandler).
Make this the last operand and it solves to comma-eating problem too.

209

Just use "i" (0) here.

216

Can use initialized member decls and default constructor.
I'd make a ScopedHandle instead and just have two of them.

265

This should use __unsanitized_memcpy from <zircon/sanitizer.h>.
Otherwise it's theoretically possible for this thread to reenter the sanitizer runtime and that could go badly.

271

Also needs rounded down to modulo 16.

310–312

typo: s/its/it's/

aarongreen marked 18 inline comments as done.

Addressed Roland's comments.

mcgrathr accepted this revision.Jul 14 2018, 12:30 AM

Looks great!

lib/fuzzer/FuzzerUtilFuchsia.cpp
182

superfluous ;

This revision is now accepted and ready to land.Jul 14 2018, 12:30 AM
aarongreen marked an inline comment as done.

Scrubbed superfluous semicolon.

This revision was automatically updated to reflect the committed changes.