GWP-ASan: Change double free crash analysis logic

Currently, the crash analyzer determines if a crash is related to
GWP-ASan by checking to see if the exception address was in the GWP-ASan
region. This can lead to some messy logic, such as requiring a double
free to intentionally access the invalid allocation to crash which can
be racy. Currently if two threads race a double free Thread 1 marks an
allocation freed but not yet inaccessible and Thread 2 can fail to crash
on accessing the page.

Instead, update the allocator to store the double free address if a
double free occurs and trap and have the crash handler to use that
address as the source of the crash.

Bug: 925447
Change-Id: Ia34a10c53d3d4a78c5bf82db671e3d0bb87ea4b9
Reviewed-on: https://chromium-review.googlesource.com/c/1435984
Auto-Submit: Vlad Tsyrklevich <vtsyrklevich@chromium.org>
Reviewed-by: Vitaly Buka <vitalybuka@chromium.org>
Commit-Queue: Vlad Tsyrklevich <vtsyrklevich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626367}

components/gwp_asan/client/guarded_page_allocator.cc

@@ -7,7 +7,6 @@
 #include <iterator>
 #include <memory>
 
-#include "base/atomicops.h"
 #include "base/bits.h"
 #include "base/no_destructor.h"
 #include "base/process/process_metrics.h"
@@ -106,12 +105,7 @@ void GuardedPageAllocator::Deallocate(void* ptr) {
   DCHECK_EQ(addr, slots_[slot].alloc_ptr);
   // Check for double free.
   if (slots_[slot].dealloc.trace_collected) {
-    state_.double_free_detected = true;
-    base::subtle::MemoryBarrier();
-    // Trigger an exception by writing to the inaccessible free()d allocation.
-    // We want to crash by accessing the offending allocation in order to signal
-    // to the crash handler that this crash is caused by that allocation.
-    *reinterpret_cast<char*>(ptr) = 'X';
+    state_.double_free_address = addr;
     __builtin_trap();
   }
 

components/gwp_asan/common/allocator_state.cc

@@ -90,7 +90,7 @@ AllocatorState::ErrorType AllocatorState::GetErrorType(uintptr_t addr,
                                                        bool deallocated) const {
   if (!allocated)
     return ErrorType::kUnknown;
-  if (double_free_detected)
+  if (double_free_address)
     return ErrorType::kDoubleFree;
   if (deallocated)
     return ErrorType::kUseAfterFree;

components/gwp_asan/common/allocator_state.h

@@ -133,8 +133,8 @@ class AllocatorState {
   // present.)
   uintptr_t slot_metadata = 0;
 
-  // Set to true if a double free has occurred.
-  bool double_free_detected = false;
+  // Set to the address of a double freed allocation if a double free occurred.
+  uintptr_t double_free_address = false;
 
   DISALLOW_COPY_AND_ASSIGN(AllocatorState);
 };

components/gwp_asan/crash_handler/crash_analyzer.cc

@@ -53,15 +53,11 @@ GwpAsanCrashAnalysisResult CrashAnalyzer::GetExceptionInfo(
   if (exception->Context()->Is64Bit() != is_64_bit)
     return GwpAsanCrashAnalysisResult::kErrorMismatchedBitness;
 
-  crashpad::VMAddress crash_addr = GetAccessAddress(*exception);
-  if (!crash_addr)
-    return GwpAsanCrashAnalysisResult::kUnrelatedCrash;
-
   if (!process_snapshot.Memory())
     return GwpAsanCrashAnalysisResult::kErrorNullProcessMemory;
 
-  return AnalyzeCrashedAllocator(*process_snapshot.Memory(), gpa_ptr,
-                                 crash_addr, proto);
+  return AnalyzeCrashedAllocator(*process_snapshot.Memory(), *exception,
+                                 gpa_ptr, proto);
 }
 
 crashpad::VMAddress CrashAnalyzer::GetAllocatorAddress(
@@ -91,8 +87,8 @@ crashpad::VMAddress CrashAnalyzer::GetAllocatorAddress(
 
 GwpAsanCrashAnalysisResult CrashAnalyzer::AnalyzeCrashedAllocator(
     const crashpad::ProcessMemory& memory,
+    const crashpad::ExceptionSnapshot& exception,
     crashpad::VMAddress gpa_addr,
-    crashpad::VMAddress exception_addr,
     gwp_asan::Crash* proto) {
   AllocatorState unsafe_state;
   if (!memory.Read(gpa_addr, sizeof(unsafe_state), &unsafe_state)) {
@@ -106,6 +102,13 @@ GwpAsanCrashAnalysisResult CrashAnalyzer::AnalyzeCrashedAllocator(
   }
   const AllocatorState& valid_state = unsafe_state;
 
+  crashpad::VMAddress exception_addr = GetAccessAddress(exception);
+  if (valid_state.double_free_address)
+    exception_addr = valid_state.double_free_address;
+
+  if (!exception_addr)
+    return GwpAsanCrashAnalysisResult::kUnrelatedCrash;
+
   uintptr_t slot_address;
   auto ret = valid_state.GetMetadataForAddress(exception_addr, &slot_address);
   if (ret == AllocatorState::GetMetadataReturnType::kErrorBadSlot) {

components/gwp_asan/crash_handler/crash_analyzer.h

@@ -76,12 +76,12 @@ class CrashAnalyzer {
 
   // This method implements the underlying logic for GetExceptionInfo(). It
   // analyzes the GuardedPageAllocator of the crashing process, and if the
-  // exception address is in the GWP-ASan region it fills out the protobuf
+  // exception occurred in the GWP-ASan region it fills out the protobuf
   // parameter and returns kGwpAsanCrash.
   static GwpAsanCrashAnalysisResult AnalyzeCrashedAllocator(
       const crashpad::ProcessMemory& memory,
+      const crashpad::ExceptionSnapshot& exception,
       crashpad::VMAddress gpa_addr,
-      crashpad::VMAddress exception_addr,
       gwp_asan::Crash* proto);
 
   // This method fills out an AllocationInfo protobuf from a