[hwasan] Fix incorrect candidate matching for stack OOB.

[fmayer]

Summary

We would find an address with matching tag, only to discover in
ShowCandidate that it's very far away from [stack].

[eugenis]

LGTM

compiler-rt/lib/hwasan/hwasan_report.cpp

438–439

I'd rather check AddrIsInStack instead of num_descriptions_printed, or record the result of the stack check in a bool variable and look at that. Otherwise this depends on the order of the checks and will become confusing later if the code is moved.

439–440

rename ShowCandidate to ShowHeapOrGlobalCandidate

[fmayer]

Address comments.

compiler-rt/lib/hwasan/hwasan_report.cpp

static uptr GetGlobalSizeFromDescriptor(uptr ptr) {
   for (const hwasan_global &global :
        HwasanGlobalsFor(load_bias, phdr_begin, ehdr->e_phnum))
     if (global.addr() <= ptr && ptr < global.addr() + global.size())
       return global.size();
 
   return 0;
 }
 
-static void ShowCandidate(uptr untagged_addr, tag_t *candidate, tag_t *left,
-                          tag_t *right) {
+static void ShowHeapOrGlobalCandidate(uptr untagged_addr, tag_t *candidate,
+                                      tag_t *left, tag_t *right) {
   Decorator d;
   uptr mem = ShadowToMem(reinterpret_cast<uptr>(candidate));
   HwasanChunkView chunk = FindHeapChunkByAddress(mem);
   if (chunk.IsAllocated()) {
     uptr offset;
     const char *whence;
     if (untagged_addr < chunk.End() && untagged_addr >= chunk.Beg()) {
       offset = untagged_addr - chunk.Beg();
Printf("%s[%p,%p) is a %s %s heap chunk; "
            d.Location(),
            beg, beg + size,
            chunk.FromSmallHeap() ? "small" : "large",
            chunk.IsAllocated() ? "allocated" : "unallocated",
            size, untagged_addr - beg,
            d.Default());
   }
 
+  tag_t addr_tag = GetTagFromPointer(tagged_addr);
+
+  bool on_stack = false;
+  // Check stack first. If the address is on the stack of a live thread, we
+  // know it cannot be a heap / global overflow.
+  hwasanThreadList().VisitAllLiveThreads([&](Thread *t) {
+    if (t->AddrIsInStack(untagged_addr)) {
+      on_stack = true;
+      // TODO(fmayer): figure out how to distinguish use-after-return and
+      // stack-buffer-overflow.
+      Printf("%s", d.Error());
+      Printf("\nCause: stack tag-mismatch\n");
+      Printf("%s", d.Location());
+      Printf("Address %p is located in stack of thread T%zd\n", untagged_addr,
+             t->unique_id());
+      Printf("%s", d.Default());
+      t->Announce();
+
+      auto *sa = (t == GetCurrentThread() && current_stack_allocations)
+                     ? current_stack_allocations
+                     : t->stack_allocations();
+      PrintStackAllocations(sa, addr_tag, untagged_addr);
+      num_descriptions_printed++;
+    }
+  });
+
   // Check if this looks like a heap buffer overflow by scanning
   // the shadow left and right and looking for the first adjacent
   // object with a different memory tag. If that tag matches addr_tag,
   // check the allocator if it has a live chunk there.
-  tag_t addr_tag = GetTagFromPointer(tagged_addr);
   tag_t *tag_ptr = reinterpret_cast<tag_t*>(MemToShadow(untagged_addr));
   tag_t *candidate = nullptr, *left = tag_ptr, *right = tag_ptr;
   uptr candidate_distance = 0;
   for (; candidate_distance < 1000; candidate_distance++) {
     if (MemIsShadow(reinterpret_cast<uptr>(left)) &&
         TagsEqual(addr_tag, left)) {
       candidate = left;
       break;
     }
     --left;
     if (MemIsShadow(reinterpret_cast<uptr>(right)) &&
         TagsEqual(addr_tag, right)) {
       candidate = right;
       break;
     }
     ++right;
   }
 
   constexpr auto kCloseCandidateDistance = 1;
 
-  if (candidate && candidate_distance <= kCloseCandidateDistance) {
-    ShowCandidate(untagged_addr, candidate, left, right);
+  if (!on_stack && candidate && candidate_distance <= kCloseCandidateDistance) {
+    ShowHeapOrGlobalCandidate(untagged_addr, candidate, left, right);
     num_descriptions_printed++;
   }
 
   hwasanThreadList().VisitAllLiveThreads([&](Thread *t) {
-    if (t->AddrIsInStack(untagged_addr)) {
-      // TODO(fmayer): figure out how to distinguish use-after-return and
-      // stack-buffer-overflow.
-      Printf("%s", d.Error());
-      Printf("\nCause: stack tag-mismatch\n");
-      Printf("%s", d.Location());
-      Printf("Address %p is located in stack of thread T%zd\n", untagged_addr,
-             t->unique_id());
-      Printf("%s", d.Default());
-      t->Announce();
-
-      auto *sa = (t == GetCurrentThread() && current_stack_allocations)
-                     ? current_stack_allocations
-                     : t->stack_allocations();
-      PrintStackAllocations(sa, addr_tag, untagged_addr);
-      num_descriptions_printed++;
-    }
-  });
-
-  hwasanThreadList().VisitAllLiveThreads([&](Thread *t) {
     // Scan all threads' ring buffers to find if it's a heap-use-after-free.
     HeapAllocationRecord har;
     uptr ring_index, num_matching_addrs, num_matching_addrs_4b;
     if (FindHeapAllocation(t->heap_allocations(), tagged_addr, &har,
                            &ring_index, &num_matching_addrs,
                            &num_matching_addrs_4b)) {
       Printf("%s", d.Error());
       Printf("\nCause: use-after-free\n");
if (FindHeapAllocation(t->heap_allocations(), tagged_addr, &har,
              num_matching_addrs_4b);
 
       t->Announce();
       num_descriptions_printed++;
     }
   });
 
   if (candidate && num_descriptions_printed == 0) {
-    ShowCandidate(untagged_addr, candidate, left, right);
+    ShowHeapOrGlobalCandidate(untagged_addr, candidate, left, right);
     num_descriptions_printed++;
   }
 
   // Print the remaining threads, as an extra information, 1 line per thread.
   hwasanThreadList().VisitAllLiveThreads([&](Thread *t) { t->Announce(); });
 
   if (!num_descriptions_printed)
     // We exhausted our possibilities. Bail out.

compiler-rt/test/hwasan/TestCases/stack-oob.c

int f() {
   return p[SIZE];
 }
 
 int main() {
   f();
   // CHECK: READ of size 1 at
   // CHECK: #0 {{.*}} in f{{.*}}stack-oob.c:[[@LINE-6]]
 
+  // CHECK-NOT: Cause: global-overflow
   // CHECK: Cause: stack tag-mismatch
   // CHECK: is located in stack of threa
+  // CHECK-NOT: Cause: global-overflow
 
   // CHECK: SUMMARY: HWAddressSanitizer: tag-mismatch {{.*}} in f
 }