[PATCH] asan: handle SIGABRT

[Alexey Samsonov]

  Sorry for the delayed response.


================
Comment at: lib/asan/asan_posix.cc:62
@@ +61,3 @@
+// Invoke the default signal handler.
+void InvokeDefaultSignalHandler(int signal_number) {
+  struct sigaction sig_action;
----------------
Looks like you don't "invoke" default handler, you just install it here.

================
Comment at: lib/asan/asan_posix.cc:90
@@ +89,3 @@
+  UnmapShadowMappings();
+  InvokeDefaultSignalHandler(SIGABRT);
+}
----------------
Todd Lipcon wrote:
> Alexander Potapenko wrote:
> > Once you unmap the shadow mappings, it's unsafe to invoke the default signal handler, because the instrumented code will try to access the shadow memory.
> The default signal handler, though, is in the kernel -- not user code. So, it won't run instrumented code, it'll just generate a core dump and exit with the right return code. Right?
> 
> I'm explicitly setting to SIG_DFL, not restoring whatever signal handler the user might have installed.
Ok... So you mean that after ASAN_OnSIGABRT exits, default signal handler will be called, and no more user code will be executed? 

================
Comment at: lib/asan/asan_report.cc:526
@@ -525,1 +525,3 @@
 
+void ReportSIGABRT(uptr pc, uptr sp, uptr bp) {
+  Decorator d;
----------------
Todd Lipcon wrote:
> Alexey Samsonov wrote:
> > No, generally ASan should fail the program immediately after it prints the error report - that's why all "ReportFoo" functions are noreturn and we have ScopedInErrorReport object. Among the other things, it makes sure that no two error reports are printed simultaneously, otherwise bad things may happen - the code in "ReportFoo" functions is thread-unsafe.
> The issue is that if you die with "Die()" then you don't get the core dump at all. I'd like to actually get core dumps on SEGV/ABRT so I can debug things, but I want the core dumps to be _without_ the shadow mapping.
> 
> With this patch, you can get this behavior by turning on abort_on_error -- the flow is:
> 
> - user code accesses bad memory
> -- ReportSEGV generates the report
> -- Die() calls abort(), generating a SIGABRT
> --- HandleSIGABRT gets called, reports SIGABRT, unmaps memory
> --- HandleSIGABRT re-kills self with SIGABRT after setting to SIG_DFL
> ---- kernel generates core dump (without shadow mappings)
> - user happily debugs their application from the corefile
> 
> As for ensuring thread-safety, I put that check in the signal handler -- it uses an atomic swap to make sure that only one thread will proceed into this code
Hm... but Die() already unmaps shadow at exit, doesn't it? Why do you need to do this in SIGABRT handler? Will the following strategy work?
* make SIGABRT and SIGSEGV handlers identical: print an error report, optionally set the handler to SIG_DFL, and call Die()
* Die() optionally unmaps the shadow memory
* Die() optionally calls abort(), which goes to kernel and generates a core dump.

Your atomic swap doesn't prevent two threads from entering ReportSIGABRT and ReportFreeNotMalloced simultaneously. The code in these functions is not thread-safe, for example they print symbolized stack traces, and symbolization is single-threaded.

================
Comment at: lib/asan/asan_rtl.cc:151
@@ -154,2 +150,3 @@
   f->handle_segv = ASAN_NEEDS_SEGV;
+  f->handle_abrt = ASAN_NEEDS_SEGV;
   f->allow_user_segv_handler = false;
----------------
Todd Lipcon wrote:
> Alexey Samsonov wrote:
> > Why? I don't think SIGABRT handler should be on by default.
> Why not? Unmapping the shadow memory on abort makes corefiles possible on 64-bit (and could enable us to then get rid of the other code which sets RLIMIT_CORE->0 on those systems)
> 
> My general use case is that I have an integration test build which runs all of my tests with ASAN enabled. Sometimes my tests fail due to a race which is difficult to reproduce, but a corefile would allow us to debug it the next morning. Currently it's impossible to get corefiles out of this build, but with this patch we're able to.
Your use case is legitimate, but we generally introduce new options under a flag. We need to make sure it won't break the existing users, especially if they were able to live w/o this capability.

See my comment above about generating core dumps.


http://llvm-reviews.chandlerc.com/D1123