[GWP-ASan] Fix up bad report for in-page underflow w/ UaF

[hctim]

Summary

Complex scenario, but reports when there's both a use-after-free and
buffer-underflow that is in-page (i.e. doesn't touch the guard page)
ended up generating a pretty bad report:

'Use After Free at 0x7ff392e88fef (18446744073709551615 bytes into a
1-byte allocation at 0x7ff392e88ff0) by thread 3836722 here:'

(note the 2^64-bytes-into-alloc, very cool and good!)

Fix up that case, and add a diagnostic about when you have both a
use-after-free and a buffer-overflow that it's probably a bogus report
(assuming the developer didn't *really* screw up and have a uaf+overflow
bug at the same time).

[hctim]

Make it so overflow+uaf reports uaf first, which is probably the more important bit of information, and makes the reporting more consistent.

[hctim]

Rebased. vitalybuka / eugenis, PTAL

[vitalybuka]

compiler-rt/lib/gwp_asan/crash_handler.cpp

56–58

not a fan of unnecessary local vars, can be source of bugs
e.g. use after scope

Some more reasoning: https://abseil.io/tips/161

58

{} use is inconsistent

[vitalybuka]

compiler-rt/lib/gwp_asan/crash_handler.cpp

72

here another one with the same name, reader can be confused which one is used
but this one is hard to remove.

[hctim]

Touch up a local variable.

compiler-rt/lib/gwp_asan/crash_handler.cpp

56–58

yeah, especially as it shadows the same name in the scope below. thanks.

compiler-rt/lib/gwp_asan/crash_handler.cpp

__gwp_asan_diagnose_error(const gwp_asan::AllocatorState *State,
                           const gwp_asan::AllocationMetadata *Metadata,
                           uintptr_t ErrorPtr) {
   if (!__gwp_asan_error_is_mine(State, ErrorPtr))
     return Error::UNKNOWN;
 
   if (State->FailureType != Error::UNKNOWN)
     return State->FailureType;
 
-  // Let's try and figure out what the source of this error is.
+  // Check for use-after-free.
+  if (addrToMetadata(State, Metadata, ErrorPtr)->IsDeallocated)
+    return Error::USE_AFTER_FREE;
+
+  // Check for buffer-overflow. Because of allocation alignment or left/right
+  // page placement, we can have buffer-overflows that don't touch a guarded
+  // page, but these are not possible to detect unless it's also a
+  // use-after-free, which is handled above.
   if (State->isGuardPage(ErrorPtr)) {
     size_t Slot = State->getNearestSlot(ErrorPtr);
     const AllocationMetadata *SlotMeta =
         addrToMetadata(State, Metadata, State->slotToAddr(Slot));
 
     // Ensure that this slot was allocated once upon a time.
     if (!SlotMeta->Addr)
       return Error::UNKNOWN;
 
     if (SlotMeta->Addr < ErrorPtr)
       return Error::BUFFER_OVERFLOW;
     return Error::BUFFER_UNDERFLOW;
   }
 
-  // Access wasn't a guard page, check for use-after-free.
-  const AllocationMetadata *SlotMeta =
-      addrToMetadata(State, Metadata, ErrorPtr);
-  if (SlotMeta->IsDeallocated) {
-    return Error::USE_AFTER_FREE;
-  }
-
   // If we have reached here, the error is still unknown.
   return Error::UNKNOWN;
 }
 
 const gwp_asan::AllocationMetadata *
 __gwp_asan_get_metadata(const gwp_asan::AllocatorState *State,
                         const gwp_asan::AllocationMetadata *Metadata,
                         uintptr_t ErrorPtr) {

compiler-rt/lib/gwp_asan/optional/segv_handler_posix.cpp

 void printHeader(Error E, uintptr_t AccessPtr,
                  const gwp_asan::AllocationMetadata *Metadata,
                  Printf_t Printf) {
   // Print using intermediate strings. Platforms like Android don't like when
   // you print multiple times to the same line, as there may be a newline
   // appended to a log file automatically per Printf() call.
   constexpr size_t kDescriptionBufferLen = 128;
   char DescriptionBuffer[kDescriptionBufferLen] = "";
+
+  bool AccessWasInBounds = false;
   if (E != Error::UNKNOWN && Metadata != nullptr) {
     uintptr_t Address = __gwp_asan_get_allocation_address(Metadata);
     size_t Size = __gwp_asan_get_allocation_size(Metadata);
-    if (E == Error::USE_AFTER_FREE) {
-      snprintf(DescriptionBuffer, kDescriptionBufferLen,
-               "(%zu byte%s into a %zu-byte allocation at 0x%zx) ",
-               AccessPtr - Address, (AccessPtr - Address == 1) ? "" : "s", Size,
-               Address);
-    } else if (AccessPtr < Address) {
+    if (AccessPtr < Address) {
       snprintf(DescriptionBuffer, kDescriptionBufferLen,
                "(%zu byte%s to the left of a %zu-byte allocation at 0x%zx) ",
                Address - AccessPtr, (Address - AccessPtr == 1) ? "" : "s", Size,
                Address);
     } else if (AccessPtr > Address) {
       snprintf(DescriptionBuffer, kDescriptionBufferLen,
                "(%zu byte%s to the right of a %zu-byte allocation at 0x%zx) ",
                AccessPtr - Address, (AccessPtr - Address == 1) ? "" : "s", Size,
                Address);
-    } else {
+    } else if (E == Error::DOUBLE_FREE) {
       snprintf(DescriptionBuffer, kDescriptionBufferLen,
                "(a %zu-byte allocation) ", Size);
+    } else {
+      AccessWasInBounds = true;
+      snprintf(DescriptionBuffer, kDescriptionBufferLen,
+               "(%zu byte%s into a %zu-byte allocation at 0x%zx) ",
+               AccessPtr - Address, (AccessPtr - Address == 1) ? "" : "s", Size,
+               Address);
     }
   }
 
   // Possible number of digits of a 64-bit number: ceil(log10(2^64)) == 20. Add
   // a null terminator, and round to the nearest 8-byte boundary.
   uint64_t ThreadID = gwp_asan::getThreadID();
   constexpr size_t kThreadBufferLen = 24;
   char ThreadBuffer[kThreadBufferLen];
   if (ThreadID == gwp_asan::kInvalidThreadID)
     snprintf(ThreadBuffer, kThreadBufferLen, "<unknown>");
   else
     snprintf(ThreadBuffer, kThreadBufferLen, "%" PRIu64, ThreadID);
 
-  Printf("%s at 0x%zx %sby thread %s here:\n", gwp_asan::ErrorToString(E),
-         AccessPtr, DescriptionBuffer, ThreadBuffer);
+  const char *OutOfBoundsAndUseAfterFreeWarning = "";
+  if (E == Error::USE_AFTER_FREE && !AccessWasInBounds) {
+    OutOfBoundsAndUseAfterFreeWarning =
+        " (warning: buffer overflow/underflow detected on a free()'d "
+        "allocation. This either means you have a buffer-overflow and a "
+        "use-after-free at the same time, or you have a long-lived "
+        "use-after-free bug where the allocation/deallocation metadata below "
+        "has already been overwritten and is likely bogus)";
+  }
+
+  Printf("%s%s at 0x%zx %sby thread %s here:\n", gwp_asan::ErrorToString(E),
+         OutOfBoundsAndUseAfterFreeWarning, AccessPtr, DescriptionBuffer,
+         ThreadBuffer);
 }
 
 void dumpReport(uintptr_t ErrorPtr, const gwp_asan::AllocatorState *State,
                 const gwp_asan::AllocationMetadata *Metadata,
                 SegvBacktrace_t SegvBacktrace, Printf_t Printf,
                 PrintBacktrace_t PrintBacktrace, void *Context) {
   assert(State && "dumpReport missing Allocator State.");
   assert(Metadata && "dumpReport missing Metadata.");

compiler-rt/test/gwp_asan/free_then_overflow.cpp

+// REQUIRES: gwp_asan
+// RUN: %clangxx_gwp_asan %s -o %t
+// RUN: %expect_crash %run %t 2>&1 | FileCheck %s
+
+// RUN: %clangxx_gwp_asan %s -o %t -DTOUCH_GUARD_PAGE
+// RUN: %expect_crash %run %t 2>&1 | FileCheck %s
+
+// CHECK: GWP-ASan detected a memory error
+// CHECK: Use After Free
+// CHECK-SAME: warning: buffer overflow/underflow detected on a free()'d allocation
+// CHECK-SAME: at 0x{{[a-f0-9]+}} ({{[0-9]+}} byte{{s?}} to the right
+
+#include <cstdlib>
+
+#include "page_size.h"
+
+int main() {
+  unsigned malloc_size = 1;
+#ifdef TOUCH_GUARD_PAGE
+  malloc_size = pageSize();
+#endif // TOUCH_GUARD_PAGE
+  char *Ptr = reinterpret_cast<char *>(malloc(malloc_size));
+  free(Ptr);
+  volatile char x = *(Ptr + malloc_size);
+  return 0;
+}

compiler-rt/test/gwp_asan/free_then_underflow.cpp

+// REQUIRES: gwp_asan
+// RUN: %clangxx_gwp_asan %s -o %t
+// RUN: %expect_crash %run %t 2>&1 | FileCheck %s
+
+// RUN: %clangxx_gwp_asan %s -o %t -DTOUCH_GUARD_PAGE
+// RUN: %expect_crash %run %t 2>&1 | FileCheck %s
+
+// CHECK: GWP-ASan detected a memory error
+// CHECK: Use After Free
+// CHECK-SAME: warning: buffer overflow/underflow detected on a free()'d allocation
+// CHECK-SAME: at 0x{{[a-f0-9]+}} (1 byte to the left
+
+#include <cstdlib>
+
+#include "page_size.h"
+
+int main() {
+  unsigned malloc_size = 1;
+#ifdef TOUCH_GUARD_PAGE
+  malloc_size = pageSize();
+#endif // TOUCH_GUARD_PAGE
+  char *Ptr = reinterpret_cast<char *>(malloc(malloc_size));
+  free(Ptr);
+  volatile char x = *(Ptr - 1);
+  return 0;
+}

compiler-rt/test/gwp_asan/heap_buffer_overflow.cpp

 // REQUIRES: gwp_asan
 // RUN: %clangxx_gwp_asan %s -o %t
 // RUN: %expect_crash %run %t 2>&1 | FileCheck %s
 
 // CHECK: GWP-ASan detected a memory error
 // CHECK: Buffer Overflow at 0x{{[a-f0-9]+}} ({{[1-9][0-9]*}} bytes to the right
 // CHECK-SAME: of a {{[1-9][0-9]*}}-byte allocation
 
 #include <cstdlib>
 
 #include "page_size.h"
 
 int main() {
-  char *Ptr =
-      reinterpret_cast<char *>(malloc(pageSize()));
+  char *Ptr = reinterpret_cast<char *>(malloc(pageSize()));
   volatile char x = *(Ptr + pageSize());
   return 0;
 }

compiler-rt/test/gwp_asan/heap_buffer_underflow.cpp

 // REQUIRES: gwp_asan
 // RUN: %clangxx_gwp_asan %s -o %t
 // RUN: %expect_crash %run %t 2>&1 | FileCheck %s
 
 // CHECK: GWP-ASan detected a memory error
 // CHECK: Buffer Underflow at 0x{{[a-f0-9]+}} (1 byte to the left
 // CHECK-SAME: of a {{[1-9][0-9]*}}-byte allocation
 
 #include <cstdlib>
 
 #include "page_size.h"
 
 int main() {
-  char *Ptr =
-      reinterpret_cast<char *>(malloc(pageSize()));
+  char *Ptr = reinterpret_cast<char *>(malloc(pageSize()));
   volatile char x = *(Ptr - 1);
   return 0;
 }