[asan] Reify ErrorDeadlySignal

[filcab]

Summary

Keep reifying other errors.

[filcab]

Updating with current tree.

[vitalybuka]

lib/asan/asan_errors.h

23 ↗

(On Diff #69809)

Please move it back into cc file

lib/asan/asan_report.cc

324 ↗

(On Diff #69809)

can you avoid nolint? maybe = {}

[filcab]

As for the NOLINT on the constructor: We have two Error* constructors doing that.
Do you want me to change them to parenthesized initialization too, so we don't need the NOLINT?

lib/asan/asan_errors.h

23 ↗

(On Diff #69809)

I had taken it out because it's a simple function which would now be needed on two .cc files (for a while), and we don't really want to export the symbol.
Will put it back and export it, though.

lib/asan/asan_report.cc

324 ↗

(On Diff #69809)

I can use parenthesis, but we might want to start thinking about updating the cpplint.py script. I guess there are newer versions that might be able to deal with this.

[filcab]

Addressed code review

[vitalybuka]

lib/asan/asan_errors.h

23 ↗

(On Diff #70336)

I see no problem with symbol
Also we already have PrintGlobalNameIfASCII and PrintGlobalLocation, so this function could sit next to them in asan_report.cc and h
Please move it there.

136 ↗

(On Diff #70336)

Let's keep NOLINT here for consistency,

lib/asan/asan_report.cc

324–325 ↗

(On Diff #70336)

I see, it's about ";" after "}"
Feel free to keep NOLINT here as this code does a lot of this already,

[filcab]

Updated with review feedback.

[filcab]

Add a missing header include

[vitalybuka]

LGTM

compiler-rt/trunk/lib/asan/asan_errors.h

 //
 // ASan-private header for error structures.
 //===----------------------------------------------------------------------===//
 #ifndef ASAN_ERRORS_H
 #define ASAN_ERRORS_H
 
 #include "asan_descriptions.h"
 #include "asan_scariness_score.h"
+#include "sanitizer_common/sanitizer_common.h"
 
 namespace __asan {
 
 struct ErrorBase {
   ScarinessScoreBase scariness;
 };
 
 struct ErrorStackOverflow : ErrorBase {
ErrorStackOverflow(const SignalContext &sig, u32 tid_)
         sp(sig.sp),
         context(sig.context) {
     scariness.Clear();
     scariness.Scare(10, "stack-overflow");
   }
   void Print();
 };
 
+struct ErrorDeadlySignal : ErrorBase {
+  u32 tid;
+  uptr addr, pc, bp, sp;
+  int signo;
+  SignalContext::WriteFlag write_flag;
+  bool is_memory_access;
+  // ErrorDeadlySignal never owns the context.
+  void *context;
+  // VS2013 doesn't implement unrestricted unions, so we need a trivial default
+  // constructor
+  ErrorDeadlySignal() = default;
+  ErrorDeadlySignal(int signo_, const SignalContext &sig, u32 tid_)
+      : tid(tid_),
+        addr(sig.addr),
+        pc(sig.pc),
+        bp(sig.bp),
+        sp(sig.sp),
+        signo(signo_),
+        write_flag(sig.write_flag),
+        is_memory_access(sig.is_memory_access),
+        context(sig.context) {
+    scariness.Clear();
+    if (is_memory_access) {
+      if (addr < GetPageSizeCached()) {
+        scariness.Scare(10, "null-deref");
+      } else if (addr == pc) {
+        scariness.Scare(60, "wild-jump");
+      } else if (write_flag == SignalContext::WRITE) {
+        scariness.Scare(30, "wild-addr-write");
+      } else if (write_flag == SignalContext::READ) {
+        scariness.Scare(20, "wild-addr-read");
+      } else {
+        scariness.Scare(25, "wild-addr");
+      }
+    } else {
+      scariness.Scare(10, "signal");
+    }
+  }
+  void Print();
+};
+
 struct ErrorDoubleFree : ErrorBase {
   u32 tid;
   HeapAddressDescription addr_description;
   // ErrorDoubleFree doesn't own the stack trace.
   BufferedStackTrace *second_free_stack;
   // VS2013 doesn't implement unrestricted unions, so we need a trivial default
   // constructor
   ErrorDoubleFree() = default;
ErrorNewDeleteSizeMismatch(uptr addr, u32 tid_, uptr delete_size_,
     scariness.Scare(10, "new-delete-type-mismatch");
   }
   void Print();
 };
 
 enum ErrorKind {
   kErrorKindInvalid = 0,
   kErrorKindStackOverflow,
+  kErrorKindDeadlySignal,
   kErrorKindDoubleFree,
   kErrorKindNewDeleteSizeMismatch,
 };
 
 struct ErrorDescription {
   ErrorKind kind;
   // We're using a tagged union because it allows us to have a trivially
   // copiable type and use the same structures as the public interface.
   //
   // We can add a wrapper around it to make it "more c++-like", but that would
   // add a lot of code and the benefit wouldn't be that big.
   union {
     ErrorStackOverflow stack_overflow;
+    ErrorDeadlySignal deadly_signal;
     ErrorDoubleFree double_free;
     ErrorNewDeleteSizeMismatch new_delete_size_mismatch;
   };
   ErrorDescription() { internal_memset(this, 0, sizeof(*this)); }
   ErrorDescription(const ErrorStackOverflow &e)  // NOLINT
       : kind(kErrorKindStackOverflow),
         stack_overflow(e) {}
+  ErrorDescription(const ErrorDeadlySignal &e)  // NOLINT
+      : kind(kErrorKindDeadlySignal),
+        deadly_signal(e) {}
   ErrorDescription(const ErrorDoubleFree &e)  // NOLINT
       : kind(kErrorKindDoubleFree),
         double_free(e) {}
   ErrorDescription(const ErrorNewDeleteSizeMismatch &e)  // NOLINT
       : kind(kErrorKindNewDeleteSizeMismatch),
         new_delete_size_mismatch(e) {}
 
   bool IsValid() { return kind != kErrorKindInvalid; }
   void Print() {
     switch (kind) {
       case kErrorKindStackOverflow:
         stack_overflow.Print();
         return;
+      case kErrorKindDeadlySignal:
+        deadly_signal.Print();
+        return;
       case kErrorKindDoubleFree:
         double_free.Print();
         return;
       case kErrorKindNewDeleteSizeMismatch:
         new_delete_size_mismatch.Print();
         return;
       case kErrorKindInvalid:
         CHECK(0);
     }
     CHECK(0);
   }
 };
 
 }  // namespace __asan
 
 #endif  // ASAN_ERRORS_H

compiler-rt/trunk/lib/asan/asan_errors.cc

 //===-- asan_errors.cc ------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of AddressSanitizer, an address sanity checker.
 //
 // ASan implementation for error structures.
 //===----------------------------------------------------------------------===//
 
 #include "asan_errors.h"
+#include <signal.h>
 #include "asan_descriptions.h"
+#include "asan_report.h"
 #include "asan_stack.h"
 
 namespace __asan {
 
 void ErrorStackOverflow::Print() {
   Decorator d;
   Printf("%s", d.Warning());
   Report(
       "ERROR: AddressSanitizer: stack-overflow on address %p"
       " (pc %p bp %p sp %p T%d)\n",
       (void *)addr, (void *)pc, (void *)bp, (void *)sp, tid);
   Printf("%s", d.EndWarning());
   scariness.Print();
   BufferedStackTrace stack;
   GetStackTraceWithPcBpAndContext(&stack, kStackTraceMax, pc, bp, context,
                                   common_flags()->fast_unwind_on_fatal);
   stack.Print();
   ReportErrorSummary("stack-overflow", &stack);
 }
 
+static void MaybeDumpInstructionBytes(uptr pc) {
+  if (!flags()->dump_instruction_bytes || (pc < GetPageSizeCached())) return;
+  InternalScopedString str(1024);
+  str.append("First 16 instruction bytes at pc: ");
+  if (IsAccessibleMemoryRange(pc, 16)) {
+    for (int i = 0; i < 16; ++i) {
+      PrintMemoryByte(&str, "", ((u8 *)pc)[i], /*in_shadow*/ false, " ");
+    }
+    str.append("\n");
+  } else {
+    str.append("unaccessible\n");
+  }
+  Report("%s", str.data());
+}
+
+void ErrorDeadlySignal::Print() {
+  Decorator d;
+  Printf("%s", d.Warning());
+  const char *description = DescribeSignalOrException(signo);
+  Report(
+      "ERROR: AddressSanitizer: %s on unknown address %p (pc %p bp %p sp %p "
+      "T%d)\n",
+      description, (void *)addr, (void *)pc, (void *)bp, (void *)sp, tid);
+  Printf("%s", d.EndWarning());
+  if (pc < GetPageSizeCached()) Report("Hint: pc points to the zero page.\n");
+  if (is_memory_access) {
+    const char *access_type =
+        write_flag == SignalContext::WRITE
+            ? "WRITE"
+            : (write_flag == SignalContext::READ ? "READ" : "UNKNOWN");
+    Report("The signal is caused by a %s memory access.\n", access_type);
+    if (addr < GetPageSizeCached())
+      Report("Hint: address points to the zero page.\n");
+  }
+  scariness.Print();
+  BufferedStackTrace stack;
+  GetStackTraceWithPcBpAndContext(&stack, kStackTraceMax, pc, bp, context,
+                                  common_flags()->fast_unwind_on_fatal);
+  stack.Print();
+  MaybeDumpInstructionBytes(pc);
+  Printf("AddressSanitizer can not provide additional info.\n");
+  ReportErrorSummary(description, &stack);
+}
+
 void ErrorDoubleFree::Print() {
   Decorator d;
   Printf("%s", d.Warning());
   char tname[128];
   Report(
       "ERROR: AddressSanitizer: attempting double-free on %p in "
       "thread T%d%s:\n",
       addr_description.addr, tid,

compiler-rt/trunk/lib/asan/asan_internal.h

 class AsanThread;
 using __sanitizer::StackTrace;
 
 void AsanInitFromRtl();
 
 // asan_win.cc
 void InitializePlatformExceptionHandlers();
 
+// asan_win.cc / asan_posix.cc
+const char *DescribeSignalOrException(int signo);
+
 // asan_rtl.cc
 void NORETURN ShowStatsAndAbort();
 
 // asan_malloc_linux.cc / asan_malloc_mac.cc
 void ReplaceSystemMalloc();
 
 // asan_linux.cc / asan_mac.cc / asan_win.cc
 void *AsanDoesNotSupportStaticLinkage();

compiler-rt/trunk/lib/asan/asan_posix.cc

 #include <signal.h>
 #include <stdlib.h>
 #include <sys/time.h>
 #include <sys/resource.h>
 #include <unistd.h>
 
 namespace __asan {
 
+const char *DescribeSignalOrException(int signo) {
+  switch (signo) {
+    case SIGFPE:
+      return "FPE";
+    case SIGILL:
+      return "ILL";
+    default:
+      return "SEGV";
+  }
+}
+
 void AsanOnDeadlySignal(int signo, void *siginfo, void *context) {
   ScopedDeadlySignal signal_scope(GetCurrentThread());
   int code = (int)((siginfo_t*)siginfo)->si_code;
   // Write the first message using fd=2, just in case.
   // It may actually fail to write in case stderr is closed.
   internal_write(2, "ASAN:DEADLYSIGNAL\n", 18);
   SignalContext sig = SignalContext::Create(siginfo, context);
 
#if __powerpc__
   }
 #endif // __powerpc__
 
   // We also check si_code to filter out SEGV caused by something else other
   // then hitting the guard page or unmapped memory, like, for example,
   // unaligned memory access.
   if (IsStackAccess && (code == si_SEGV_MAPERR || code == si_SEGV_ACCERR))
     ReportStackOverflow(sig);
-  else if (signo == SIGFPE)
-    ReportDeadlySignal("FPE", sig);
-  else if (signo == SIGILL)
-    ReportDeadlySignal("ILL", sig);
-  else if (signo == SIGABRT)
-    ReportDeadlySignal("ABRT", sig);
   else
-    ReportDeadlySignal("SEGV", sig);
+    ReportDeadlySignal(signo, sig);
 }
 
 // ---------------------- TSD ---------------- {{{1
 
 static pthread_key_t tsd_key;
 static bool tsd_key_inited = false;
 void AsanTSDInit(void (*destructor)(void *tsd)) {
   CHECK(!tsd_key_inited);

compiler-rt/trunk/lib/asan/asan_report.h

 // them to "globals" array.
 int GetGlobalsForAddress(uptr addr, __asan_global *globals, u32 *reg_sites,
                          int max_globals);
 
 const char *MaybeDemangleGlobalName(const char *name);
 void PrintGlobalNameIfASCII(InternalScopedString *str, const __asan_global &g);
 void PrintGlobalLocation(InternalScopedString *str, const __asan_global &g);
 
+void PrintMemoryByte(InternalScopedString *str, const char *before, u8 byte,
+                     bool in_shadow, const char *after = "\n");
+
 // The following functions prints address description depending
 // on the memory type (shadow/heap/stack/global).
 bool ParseFrameDescription(const char *frame_descr,
                            InternalMmapVector<StackVarDescr> *vars);
 
 // Different kinds of error reports.
 void ReportGenericError(uptr pc, uptr bp, uptr sp, uptr addr, bool is_write,
                         uptr access_size, u32 exp, bool fatal);
 void ReportStackOverflow(const SignalContext &sig);
-void ReportDeadlySignal(const char *description, const SignalContext &sig);
+void ReportDeadlySignal(int signo, const SignalContext &sig);
 void ReportNewDeleteSizeMismatch(uptr addr, uptr delete_size,
                                  BufferedStackTrace *free_stack);
 void ReportDoubleFree(uptr addr, BufferedStackTrace *free_stack);
 void ReportFreeNotMalloced(uptr addr, BufferedStackTrace *free_stack);
 void ReportAllocTypeMismatch(uptr addr, BufferedStackTrace *free_stack,
                              AllocType alloc_type,
                              AllocType dealloc_type);
 void ReportMallocUsableSizeNotOwned(uptr addr, BufferedStackTrace *stack);

compiler-rt/trunk/lib/asan/asan_report.cc

internal_strncpy(error_message_buffer + error_message_buffer_pos,
                    buffer, remaining);
   error_message_buffer[kErrorMessageBufferSize - 1] = '\0';
   // FIXME: reallocate the buffer instead of truncating the message.
   error_message_buffer_pos += Min(remaining, length);
 }
 
 // ---------------------- Helper functions ----------------------- {{{1
 
-static void PrintMemoryByte(InternalScopedString *str, const char *before,
-    u8 byte, bool in_shadow, const char *after = "\n") {
+void PrintMemoryByte(InternalScopedString *str, const char *before, u8 byte,
+                     bool in_shadow, const char *after) {
   Decorator d;
   str->append("%s%s%x%x%s%s", before,
               in_shadow ? d.ShadowByte(byte) : d.MemoryByte(),
               byte >> 4, byte & 15,
               in_shadow ? d.EndShadowByte() : d.EndMemoryByte(), after);
 }
 
 static void PrintShadowByte(InternalScopedString *str, const char *before,
PrintShadowByte(str, "  Array cookie:            ",
                   kAsanArrayCookieMagic);
   PrintShadowByte(str, "  Intra object redzone:    ",
                   kAsanIntraObjectRedzone);
   PrintShadowByte(str, "  ASan internal:           ", kAsanInternalHeapMagic);
   PrintShadowByte(str, "  Left alloca redzone:     ", kAsanAllocaLeftMagic);
   PrintShadowByte(str, "  Right alloca redzone:    ", kAsanAllocaRightMagic);
 }
 
-void MaybeDumpInstructionBytes(uptr pc) {
-  if (!flags()->dump_instruction_bytes || (pc < GetPageSizeCached()))
-    return;
-  InternalScopedString str(1024);
-  str.append("First 16 instruction bytes at pc: ");
-  if (IsAccessibleMemoryRange(pc, 16)) {
-    for (int i = 0; i < 16; ++i) {
-      PrintMemoryByte(&str, "", ((u8 *)pc)[i], /*in_shadow*/false, " ");
-    }
-    str.append("\n");
-  } else {
-    str.append("unaccessible\n");
-  }
-  Report("%s", str.data());
-}
-
 static void PrintShadowMemoryForAddress(uptr addr) {
   if (!AddrIsInMem(addr)) return;
   uptr shadow_addr = MemToShadow(addr);
   const uptr n_bytes_per_row = 16;
   uptr aligned_shadow = shadow_addr & ~(n_bytes_per_row - 1);
   InternalScopedString str(4096 * 8);
   str.append("Shadow bytes around the buggy address:\n");
   for (int i = -5; i <= 5; i++) {

 ErrorDescription ScopedInErrorReport::current_error_;
 
 void ReportStackOverflow(const SignalContext &sig) {
   ScopedInErrorReport in_report(/*report*/ nullptr, /*fatal*/ true);
   ErrorStackOverflow error{sig, GetCurrentTidOrInvalid()};  // NOLINT
   in_report.ReportError(error);
 }
 
-void ReportDeadlySignal(const char *description, const SignalContext &sig) {
+void ReportDeadlySignal(int signo, const SignalContext &sig) {
   ScopedInErrorReport in_report(/*report*/ nullptr, /*fatal*/ true);
-  Decorator d;
-  Printf("%s", d.Warning());
-  Report(
-      "ERROR: AddressSanitizer: %s on unknown address %p"
-      " (pc %p bp %p sp %p T%d)\n",
-      description, (void *)sig.addr, (void *)sig.pc, (void *)sig.bp,
-      (void *)sig.sp, GetCurrentTidOrInvalid());
-  Printf("%s", d.EndWarning());
-  ScarinessScore SS;
-  if (sig.pc < GetPageSizeCached())
-    Report("Hint: pc points to the zero page.\n");
-  if (sig.is_memory_access) {
-    const char *access_type =
-        sig.write_flag == SignalContext::WRITE
-            ? "WRITE"
-            : (sig.write_flag == SignalContext::READ ? "READ" : "UNKNOWN");
-    Report("The signal is caused by a %s memory access.\n", access_type);
-    if (sig.addr < GetPageSizeCached()) {
-      Report("Hint: address points to the zero page.\n");
-      SS.Scare(10, "null-deref");
-    } else if (sig.addr == sig.pc) {
-      SS.Scare(60, "wild-jump");
-    } else if (sig.write_flag == SignalContext::WRITE) {
-      SS.Scare(30, "wild-addr-write");
-    } else if (sig.write_flag == SignalContext::READ) {
-      SS.Scare(20, "wild-addr-read");
-    } else {
-      SS.Scare(25, "wild-addr");
-    }
-  } else {
-    SS.Scare(10, "signal");
-  }
-  SS.Print();
-  GET_STACK_TRACE_SIGNAL(sig);
-  stack.Print();
-  MaybeDumpInstructionBytes(sig.pc);
-  Printf("AddressSanitizer can not provide additional info.\n");
-  ReportErrorSummary(description, &stack);
+  ErrorDeadlySignal error(signo, sig, GetCurrentTidOrInvalid());
+  in_report.ReportError(error);
 }
 
 void ReportDoubleFree(uptr addr, BufferedStackTrace *free_stack) {
   ScopedInErrorReport in_report;
   ErrorDoubleFree error{addr, GetCurrentTidOrInvalid(), free_stack};  // NOLINT
   in_report.ReportError(error);
 }
 

compiler-rt/trunk/lib/asan/asan_win.cc

switch (code) {
     case EXCEPTION_ILLEGAL_INSTRUCTION: {
       return common_flags()->handle_sigill;
     }
   }
   return false;
 }
 
 // Return the textual name for this exception.
-static const char *DescribeDeadlyException(unsigned code) {
+const char *DescribeSignalOrException(int signo) {
+  unsigned code = signo;
+  // Get the string description of the exception if this is a known deadly
+  // exception.
   switch (code) {
     case EXCEPTION_ACCESS_VIOLATION:
       return "access-violation";
     case EXCEPTION_IN_PAGE_ERROR:
       return "in-page-error";
     case EXCEPTION_BREAKPOINT:
       return "breakpoint";
     case EXCEPTION_ILLEGAL_INSTRUCTION:
       return "illegal-instruction";
   }
   return nullptr;
 }
 
 static long WINAPI SEHHandler(EXCEPTION_POINTERS *info) {
   EXCEPTION_RECORD *exception_record = info->ExceptionRecord;
   CONTEXT *context = info->ContextRecord;
 
   if (ShouldReportDeadlyException(exception_record->ExceptionCode)) {
-    // Get the string description of the exception if this is a known deadly
-    // exception.
-    const char *description =
-        DescribeDeadlyException(exception_record->ExceptionCode);
     SignalContext sig = SignalContext::Create(exception_record, context);
-    ReportDeadlySignal(description, sig);
+    ReportDeadlySignal(exception_record->ExceptionCode, sig);
   }
 
   // FIXME: Handle EXCEPTION_STACK_OVERFLOW here.
 
   return default_seh_handler(info);
 }
 
 // We want to install our own exception handler (EH) to print helpful reports