[asan] Provide bug descriptions for all reports (not just ErrorGeneric)

[kubamracek]

Summary

ErrorGeneric report type has a const char *bug_descr, which describes the type of bug that is detected. Most non-generic bugs (e.g. double-free, null pointer dereference) also already have a short keyword used in printed reports (e.g. "double-free", "null-deref"). This patch moves the bug_descr field to ErrorBase and makes sure it's populated by all constructors.

[rnk]

Isn't this somewhat redundant with the buffer in ScarinessScoreBase?

struct ScarinessScoreBase {
  ...
  int score;
  char descr[1024];
};

[filcab]

In D27012#604558, @rnk wrote:

Isn't this somewhat redundant with the buffer in ScarinessScoreBase?

struct ScarinessScoreBase {
  ...
  int score;
  char descr[1024];
};

I was going to ask the same. And we have more information in the scariness score (e.g: in the param-overlap, and generic errors).
Maybe converting all the scattered bug_descr to use the scariness score would be better?

[kubamracek]

Isn't this somewhat redundant with the buffer in ScarinessScoreBase?
Maybe converting all the scattered bug_descr to use the scariness score would be better?

Makes sense. Updating the patch to use the description from scariness.

[rnk]

In D27012#604582, @filcab wrote:

In D27012#604558, @rnk wrote:

Isn't this somewhat redundant with the buffer in ScarinessScoreBase?

struct ScarinessScoreBase {
  ...
  int score;
  char descr[1024];
};

I was going to ask the same. And we have more information in the scariness score (e.g: in the param-overlap, and generic errors).
Maybe converting all the scattered bug_descr to use the scariness score would be better?

Well, it looks like it's not that redundant. If you look at the logic in ErrorGeneric, you see multiple calls to scariness.Scare, and that's where the complexity arises. I take it back, let's keep this separate.

[kubamracek]

Well, it looks like it's not that redundant. If you look at the logic in ErrorGeneric, you see multiple calls to scariness.Scare, and that's where the complexity arises. I take it back, let's keep this separate.

ErrorGeneric constructs scariness strings like "8-byte-write-heap-buffer-overflow-far-from-bounds", which is the description of this bug instance, whereas what I'm looking for is a description of a whole class, for the purposes of categorization, e.g. "heap-buffer-overflow", "double-free", "odr-violation" and such. So I slightly agree with you that this is a separate thing from the scariness description, but there's a lot of overlap.

So the previous patch looks good to you?

[filcab]

If ErrorGeneric is the only special one, why not use the scariness in general, but make that one a special case?
It's already special, so it's not really a big change.

[kubamracek]

In D27012#604649, @filcab wrote:

If ErrorGeneric is the only special one, why not use the scariness in general, but make that one a special case?
It's already special, so it's not really a big change.

That's what the current patch does.

[filcab]

Ah yes. Sorry about that.
Current patch LGTM!

[kubamracek]

Thanks. Reid?

[rnk]

lgtm, I see, ErrorGeneric still does ReportErrorSummary(bug_descr, &stack); Thanks!

[kubamracek]

Thanks for the review!

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

struct ErrorMallocUsableSizeNotOwned : ErrorBase {
   // VS2013 doesn't implement unrestricted unions, so we need a trivial default
   // constructor
   ErrorMallocUsableSizeNotOwned() = default;
   ErrorMallocUsableSizeNotOwned(u32 tid, BufferedStackTrace *stack_, uptr addr)
       : ErrorBase(tid),
         stack(stack_),
         addr_description(addr, /*shouldLockThreadRegistry=*/false) {
     scariness.Clear();
+    scariness.Scare(10, "bad-malloc_usable_size");
   }
   void Print();
 };
 
 struct ErrorSanitizerGetAllocatedSizeNotOwned : ErrorBase {
   // ErrorSanitizerGetAllocatedSizeNotOwned doesn't own the stack trace.
   const BufferedStackTrace *stack;
   AddressDescription addr_description;
   // VS2013 doesn't implement unrestricted unions, so we need a trivial default
   // constructor
   ErrorSanitizerGetAllocatedSizeNotOwned() = default;
   ErrorSanitizerGetAllocatedSizeNotOwned(u32 tid, BufferedStackTrace *stack_,
                                          uptr addr)
       : ErrorBase(tid),
         stack(stack_),
         addr_description(addr, /*shouldLockThreadRegistry=*/false) {
     scariness.Clear();
+    scariness.Scare(10, "bad-__sanitizer_get_allocated_size");
   }
   void Print();
 };
 
 struct ErrorStringFunctionMemoryRangesOverlap : ErrorBase {
   // ErrorStringFunctionMemoryRangesOverlap doesn't own the stack trace.
   const BufferedStackTrace *stack;
   uptr length1, length2;
ErrorBadParamsToAnnotateContiguousContainer(u32 tid,
                                               BufferedStackTrace *stack_,
                                               uptr beg_, uptr end_,
                                               uptr old_mid_, uptr new_mid_)
       : ErrorBase(tid),
         stack(stack_),
         beg(beg_),
         end(end_),
         old_mid(old_mid_),
-        new_mid(new_mid_) {}
+        new_mid(new_mid_) {
+    scariness.Clear();
+    scariness.Scare(10, "bad-__sanitizer_annotate_contiguous_container");
+  }
   void Print();
 };
 
 struct ErrorODRViolation : ErrorBase {
   __asan_global global1, global2;
   u32 stack_id1, stack_id2;
   // VS2013 doesn't implement unrestricted unions, so we need a trivial default
   // constructor
   ErrorODRViolation() = default;
   ErrorODRViolation(u32 tid, const __asan_global *g1, u32 stack_id1_,
                     const __asan_global *g2, u32 stack_id2_)
       : ErrorBase(tid),
         global1(*g1),
         global2(*g2),
         stack_id1(stack_id1_),
-        stack_id2(stack_id2_) {}
+        stack_id2(stack_id2_) {
+    scariness.Clear();
+    scariness.Scare(10, "odr-violation");
+  }
   void Print();
 };
 
 struct ErrorInvalidPointerPair : ErrorBase {
   uptr pc, bp, sp;
   AddressDescription addr1_description;
   AddressDescription addr2_description;
   // VS2013 doesn't implement unrestricted unions, so we need a trivial default
   // constructor
   ErrorInvalidPointerPair() = default;
   ErrorInvalidPointerPair(u32 tid, uptr pc_, uptr bp_, uptr sp_, uptr p1,
                           uptr p2)
       : ErrorBase(tid),
         pc(pc_),
         bp(bp_),
         sp(sp_),
         addr1_description(p1, 1, /*shouldLockThreadRegistry=*/false),
-        addr2_description(p2, 1, /*shouldLockThreadRegistry=*/false) {}
+        addr2_description(p2, 1, /*shouldLockThreadRegistry=*/false)  {
+    scariness.Clear();
+    scariness.Scare(10, "invalid-pointer-pair");
+  }
   void Print();
 };
 
 struct ErrorGeneric : ErrorBase {
   AddressDescription addr_description;
   uptr pc, bp, sp;
   uptr access_size;
   const char *bug_descr;

 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 {
+    ErrorBase Base;
     ASAN_FOR_EACH_ERROR_KIND(ASAN_ERROR_DESCRIPTION_MEMBER)
   };
 
   ErrorDescription() { internal_memset(this, 0, sizeof(*this)); }
   ASAN_FOR_EACH_ERROR_KIND(ASAN_ERROR_DESCRIPTION_CONSTRUCTOR)
 
   bool IsValid() { return kind != kErrorKindInvalid; }
   void Print() {

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

 #include "sanitizer_common/sanitizer_stackdepot.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",
+      "ERROR: AddressSanitizer: %s on address %p"
+      " (pc %p bp %p sp %p T%d)\n", scariness.GetDescription(),
       (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);
+  ReportErrorSummary(scariness.GetDescription(), &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) {
void ErrorDeadlySignal::Print() {
   ReportErrorSummary(description, &stack);
 }
 
 void ErrorDoubleFree::Print() {
   Decorator d;
   Printf("%s", d.Warning());
   char tname[128];
   Report(
-      "ERROR: AddressSanitizer: attempting double-free on %p in "
+      "ERROR: AddressSanitizer: attempting %s on %p in "
       "thread T%d%s:\n",
-      addr_description.addr, tid,
+      scariness.GetDescription(), addr_description.addr, tid,
       ThreadNameWithParenthesis(tid, tname, sizeof(tname)));
   Printf("%s", d.EndWarning());
   scariness.Print();
   GET_STACK_TRACE_FATAL(second_free_stack->trace[0],
                         second_free_stack->top_frame_bp);
   stack.Print();
   addr_description.Print();
-  ReportErrorSummary("double-free", &stack);
+  ReportErrorSummary(scariness.GetDescription(), &stack);
 }
 
 void ErrorNewDeleteSizeMismatch::Print() {
   Decorator d;
   Printf("%s", d.Warning());
   char tname[128];
   Report(
-      "ERROR: AddressSanitizer: new-delete-type-mismatch on %p in thread "
+      "ERROR: AddressSanitizer: %s on %p in thread "
       "T%d%s:\n",
-      addr_description.addr, tid,
+      scariness.GetDescription(), addr_description.addr, tid,
       ThreadNameWithParenthesis(tid, tname, sizeof(tname)));
   Printf("%s  object passed to delete has wrong type:\n", d.EndWarning());
   Printf(
       "  size of the allocated type:   %zd bytes;\n"
       "  size of the deallocated type: %zd bytes.\n",
       addr_description.chunk_access.chunk_size, delete_size);
   CHECK_GT(free_stack->size, 0);
   scariness.Print();
   GET_STACK_TRACE_FATAL(free_stack->trace[0], free_stack->top_frame_bp);
   stack.Print();
   addr_description.Print();
-  ReportErrorSummary("new-delete-type-mismatch", &stack);
+  ReportErrorSummary(scariness.GetDescription(), &stack);
   Report(
       "HINT: if you don't care about these errors you may set "
       "ASAN_OPTIONS=new_delete_type_mismatch=0\n");
 }
 
 void ErrorFreeNotMalloced::Print() {
   Decorator d;
   Printf("%s", d.Warning());
   char tname[128];
   Report(
       "ERROR: AddressSanitizer: attempting free on address "
       "which was not malloc()-ed: %p in thread T%d%s\n",
       addr_description.Address(), tid,
       ThreadNameWithParenthesis(tid, tname, sizeof(tname)));
   Printf("%s", d.EndWarning());
   CHECK_GT(free_stack->size, 0);
   scariness.Print();
   GET_STACK_TRACE_FATAL(free_stack->trace[0], free_stack->top_frame_bp);
   stack.Print();
   addr_description.Print();
-  ReportErrorSummary("bad-free", &stack);
+  ReportErrorSummary(scariness.GetDescription(), &stack);
 }
 
 void ErrorAllocTypeMismatch::Print() {
   static const char *alloc_names[] = {"INVALID", "malloc", "operator new",
                                       "operator new []"};
   static const char *dealloc_names[] = {"INVALID", "free", "operator delete",
                                         "operator delete []"};
   CHECK_NE(alloc_type, dealloc_type);
   Decorator d;
   Printf("%s", d.Warning());
-  Report("ERROR: AddressSanitizer: alloc-dealloc-mismatch (%s vs %s) on %p\n",
+  Report("ERROR: AddressSanitizer: %s (%s vs %s) on %p\n",
+         scariness.GetDescription(),
          alloc_names[alloc_type], dealloc_names[dealloc_type],
          addr_description.addr);
   Printf("%s", d.EndWarning());
   CHECK_GT(dealloc_stack->size, 0);
   scariness.Print();
   GET_STACK_TRACE_FATAL(dealloc_stack->trace[0], dealloc_stack->top_frame_bp);
   stack.Print();
   addr_description.Print();
-  ReportErrorSummary("alloc-dealloc-mismatch", &stack);
+  ReportErrorSummary(scariness.GetDescription(), &stack);
   Report(
       "HINT: if you don't care about these errors you may set "
       "ASAN_OPTIONS=alloc_dealloc_mismatch=0\n");
 }
 
 void ErrorMallocUsableSizeNotOwned::Print() {
   Decorator d;
   Printf("%s", d.Warning());
   Report(
       "ERROR: AddressSanitizer: attempting to call malloc_usable_size() for "
       "pointer which is not owned: %p\n",
       addr_description.Address());
   Printf("%s", d.EndWarning());
   stack->Print();
   addr_description.Print();
-  ReportErrorSummary("bad-malloc_usable_size", stack);
+  ReportErrorSummary(scariness.GetDescription(), stack);
 }
 
 void ErrorSanitizerGetAllocatedSizeNotOwned::Print() {
   Decorator d;
   Printf("%s", d.Warning());
   Report(
       "ERROR: AddressSanitizer: attempting to call "
       "__sanitizer_get_allocated_size() for pointer which is not owned: %p\n",
       addr_description.Address());
   Printf("%s", d.EndWarning());
   stack->Print();
   addr_description.Print();
-  ReportErrorSummary("bad-__sanitizer_get_allocated_size", stack);
+  ReportErrorSummary(scariness.GetDescription(), stack);
 }
 
 void ErrorStringFunctionMemoryRangesOverlap::Print() {
   Decorator d;
   char bug_type[100];
   internal_snprintf(bug_type, sizeof(bug_type), "%s-param-overlap", function);
   Printf("%s", d.Warning());
   Report(
       "ERROR: AddressSanitizer: %s: memory ranges [%p,%p) and [%p, %p) "
       "overlap\n",
       bug_type, addr1_description.Address(),
       addr1_description.Address() + length1, addr2_description.Address(),
       addr2_description.Address() + length2);
   Printf("%s", d.EndWarning());
   scariness.Print();
   stack->Print();
   addr1_description.Print();
   addr2_description.Print();
   ReportErrorSummary(bug_type, stack);
 }
 
 void ErrorStringFunctionSizeOverflow::Print() {
   Decorator d;
   Printf("%s", d.Warning());
-  const char *bug_type = "negative-size-param";
-  Report("ERROR: AddressSanitizer: %s: (size=%zd)\n", bug_type, size);
+  Report("ERROR: AddressSanitizer: %s: (size=%zd)\n",
+         scariness.GetDescription(), size);
   Printf("%s", d.EndWarning());
   scariness.Print();
   stack->Print();
   addr_description.Print();
-  ReportErrorSummary(bug_type, stack);
+  ReportErrorSummary(scariness.GetDescription(), stack);
 }
 
 void ErrorBadParamsToAnnotateContiguousContainer::Print() {
   Report(
       "ERROR: AddressSanitizer: bad parameters to "
       "__sanitizer_annotate_contiguous_container:\n"
       "      beg     : %p\n"
       "      end     : %p\n"
       "      old_mid : %p\n"
       "      new_mid : %p\n",
       beg, end, old_mid, new_mid);
   uptr granularity = SHADOW_GRANULARITY;
   if (!IsAligned(beg, granularity))
     Report("ERROR: beg is not aligned by %d\n", granularity);
   stack->Print();
-  ReportErrorSummary("bad-__sanitizer_annotate_contiguous_container", stack);
+  ReportErrorSummary(scariness.GetDescription(), stack);
 }
 
 void ErrorODRViolation::Print() {
   Decorator d;
   Printf("%s", d.Warning());
-  Report("ERROR: AddressSanitizer: odr-violation (%p):\n", global1.beg);
+  Report("ERROR: AddressSanitizer: %s (%p):\n", scariness.GetDescription(),
+         global1.beg);
   Printf("%s", d.EndWarning());
   InternalScopedString g1_loc(256), g2_loc(256);
   PrintGlobalLocation(&g1_loc, global1);
   PrintGlobalLocation(&g2_loc, global2);
   Printf("  [1] size=%zd '%s' %s\n", global1.size,
          MaybeDemangleGlobalName(global1.name), g1_loc.data());
   Printf("  [2] size=%zd '%s' %s\n", global2.size,
          MaybeDemangleGlobalName(global2.name), g2_loc.data());
   if (stack_id1 && stack_id2) {
     Printf("These globals were registered at these points:\n");
     Printf("  [1]:\n");
     StackDepotGet(stack_id1).Print();
     Printf("  [2]:\n");
     StackDepotGet(stack_id2).Print();
   }
   Report(
       "HINT: if you don't care about these errors you may set "
       "ASAN_OPTIONS=detect_odr_violation=0\n");
   InternalScopedString error_msg(256);
-  error_msg.append("odr-violation: global '%s' at %s",
+  error_msg.append("%s: global '%s' at %s", scariness.GetDescription(),
                    MaybeDemangleGlobalName(global1.name), g1_loc.data());
   ReportErrorSummary(error_msg.data());
 }
 
 void ErrorInvalidPointerPair::Print() {
-  const char *bug_type = "invalid-pointer-pair";
   Decorator d;
   Printf("%s", d.Warning());
-  Report("ERROR: AddressSanitizer: invalid-pointer-pair: %p %p\n",
+  Report("ERROR: AddressSanitizer: %s: %p %p\n", scariness.GetDescription(),
          addr1_description.Address(), addr2_description.Address());
   Printf("%s", d.EndWarning());
   GET_STACK_TRACE_FATAL(pc, bp);
   stack.Print();
   addr1_description.Print();
   addr2_description.Print();
-  ReportErrorSummary(bug_type, &stack);
+  ReportErrorSummary(scariness.GetDescription(), &stack);
 }
 
 static bool AdjacentShadowValuesAreFullyPoisoned(u8 *s) {
   return s[-1] > 127 && s[1] > 127;
 }
 
 ErrorGeneric::ErrorGeneric(u32 tid, uptr pc_, uptr bp_, uptr sp_, uptr addr,
                            bool is_write_, uptr access_size_)

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

 void __asan_describe_address(uptr addr) {
   // Thread registry must be locked while we're describing an address.
   asanThreadRegistry().Lock();
   PrintAddressDescription(addr, 1, "");
   asanThreadRegistry().Unlock();
 }
 
 int __asan_report_present() {
-  return ScopedInErrorReport::CurrentError().kind == kErrorKindGeneric;
+  return ScopedInErrorReport::CurrentError().kind != kErrorKindInvalid;
 }
 
 uptr __asan_get_report_pc() {
   if (ScopedInErrorReport::CurrentError().kind == kErrorKindGeneric)
     return ScopedInErrorReport::CurrentError().Generic.pc;
   return 0;
 }
 
 uptr __asan_get_report_bp() {
   if (ScopedInErrorReport::CurrentError().kind == kErrorKindGeneric)
     return ScopedInErrorReport::CurrentError().Generic.bp;
   return 0;
 }
 
 uptr __asan_get_report_sp() {
   if (ScopedInErrorReport::CurrentError().kind == kErrorKindGeneric)
     return ScopedInErrorReport::CurrentError().Generic.sp;
   return 0;
 }
 
 uptr __asan_get_report_address() {
-  if (ScopedInErrorReport::CurrentError().kind == kErrorKindGeneric)
-    return ScopedInErrorReport::CurrentError()
-        .Generic.addr_description.Address();
+  ErrorDescription &err = ScopedInErrorReport::CurrentError();
+  if (err.kind == kErrorKindGeneric)
+    return err.Generic.addr_description.Address();
+  else if (err.kind == kErrorKindDoubleFree)
+    return err.DoubleFree.addr_description.addr;
   return 0;
 }
 
 int __asan_get_report_access_type() {
   if (ScopedInErrorReport::CurrentError().kind == kErrorKindGeneric)
     return ScopedInErrorReport::CurrentError().Generic.is_write;
   return 0;
 }
 
 uptr __asan_get_report_access_size() {
   if (ScopedInErrorReport::CurrentError().kind == kErrorKindGeneric)
     return ScopedInErrorReport::CurrentError().Generic.access_size;
   return 0;
 }
 
 const char *__asan_get_report_description() {
   if (ScopedInErrorReport::CurrentError().kind == kErrorKindGeneric)
     return ScopedInErrorReport::CurrentError().Generic.bug_descr;
-  return nullptr;
+  return ScopedInErrorReport::CurrentError().Base.scariness.GetDescription();
 }
 
 extern "C" {
 SANITIZER_INTERFACE_ATTRIBUTE
 void __sanitizer_ptr_sub(void *a, void *b) {
   CheckForInvalidPointerPair(a, b);
 }
 SANITIZER_INTERFACE_ATTRIBUTE

compiler-rt/trunk/test/asan/TestCases/debug_double_free.cc

+// RUN: %clangxx_asan -O0 %s -o %t && not %run %t 2>&1 | FileCheck %s
+
+#include <sanitizer/asan_interface.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+char *heap_ptr;
+
+int main() {
+  heap_ptr = (char *)malloc(10);
+  fprintf(stderr, "heap_ptr: %p\n", heap_ptr);
+  // CHECK: heap_ptr: 0x[[ADDR:[0-9a-f]+]]
+
+  free(heap_ptr);
+  free(heap_ptr);  // BOOM
+  return 0;
+}
+
+void __asan_on_error() {
+  int present = __asan_report_present();
+  void *addr = __asan_get_report_address();
+  const char *description = __asan_get_report_description();
+
+  fprintf(stderr, "%s\n", (present == 1) ? "report present" : "");
+  // CHECK: report present
+  fprintf(stderr, "addr: %p\n", addr);
+  // CHECK: addr: {{0x0*}}[[ADDR]]
+  fprintf(stderr, "description: %s\n", description);
+  // CHECK: description: double-free
+}
+
+// CHECK: AddressSanitizer: attempting double-free on {{0x0*}}[[ADDR]] in thread T0