[NFC][compiler-rt][hwasan] Refactor hwasan functions

[leonardchan]

Summary

This moves the implementations for HandleTagMismatch, __hwasan_tag_mismatch4, and HwasanAtExit from hwasan_linux.cpp to hwasan.cpp and declares them in hwasan.h. This way, calls to those functions can be shared with the fuchsia implementation without duplicating code.

[vitalybuka]

compiler-rt/lib/hwasan/hwasan.cpp

192

please clang-format the patch

226

__hwasan:: redundant

[vitalybuka]

compiler-rt/lib/hwasan/hwasan.cpp

205

maybe we should pop everything up to "pc" to avoid issues with nested calls?

For most users hwasan frames are not very useful. However if you work on sanitizer, some frames can be a useful info. So I don't mind we just relax test cases to accommodate this nesting.

cc @smd

[smd]

Hi folks,

I've been working on support hwasan for risc-v and I believe I've found an issue with the existing lit tests this commit causes.
Tests stack-{oob,uar,uas}.c check for correct backtrace being printed. From the code and comments the idea is to not to print any hwasan related frames(see the code and comments below).

void HandleTagMismatch(AccessInfo ai, uptr pc, uptr frame, void *uc,                                
                       uptr *registers_frame) {                                                     
  InternalMmapVector<BufferedStackTrace> stack_buffer(1);                                           
  BufferedStackTrace *stack = stack_buffer.data();                                                  
  stack->Reset();                                                                                   
  stack->Unwind(pc, frame, uc, common_flags()->fast_unwind_on_fatal);                               
                                                                                                    
  // The second stack frame contains the failure __hwasan_check function, as                        
  // we have a stack frame for the registers saved in __hwasan_tag_mismatch that                    
  // we wish to ignore. This (currently) only occurs on AArch64, as x64                      
  // implementations use SIGTRAP to implement the failure, and thus do not go                       
  // through the stack saver.                                                                       
  if (registers_frame && stack->trace && stack->size > 0) {                                         
    stack->trace++;                                                                              
    stack->size--;                                                                               
  }

Before this commit the return address and frame pointer to were taken directly from hwasan_tag_mismatch4, while after the commit those addresses are calculated after another function being called from hwasan_tag_mismatch4 (the HwasanTagMismatch one).
So, if I understand it correctly, now it looks like 2 stack frames must be ignored(for hwasan_tag_mismatch4 and HwasanTagMismatch) to get a proper backtrace.
What do you think?

Thanks

[fmayer]

In D103562#3702962, @smd wrote:

Hi folks,

I've been working on support hwasan for risc-v and I believe I've found an issue with the existing lit tests this commit causes.
Tests stack-{oob,uar,uas}.c check for correct backtrace being printed. From the code and comments the idea is to not to print any hwasan related frames(see the code and comments below).

void HandleTagMismatch(AccessInfo ai, uptr pc, uptr frame, void *uc,                                
                       uptr *registers_frame) {                                                     
  InternalMmapVector<BufferedStackTrace> stack_buffer(1);                                           
  BufferedStackTrace *stack = stack_buffer.data();                                                  
  stack->Reset();                                                                                   
  stack->Unwind(pc, frame, uc, common_flags()->fast_unwind_on_fatal);                               
                                                                                                    
  // The second stack frame contains the failure __hwasan_check function, as                        
  // we have a stack frame for the registers saved in __hwasan_tag_mismatch that                    
  // we wish to ignore. This (currently) only occurs on AArch64, as x64                      
  // implementations use SIGTRAP to implement the failure, and thus do not go                       
  // through the stack saver.                                                                       
  if (registers_frame && stack->trace && stack->size > 0) {                                         
    stack->trace++;                                                                              
    stack->size--;                                                                               
  }

Before this commit the return address and frame pointer to were taken directly from hwasan_tag_mismatch4, while after the commit those addresses are calculated after another function being called from hwasan_tag_mismatch4 (the HwasanTagMismatch one).
So, if I understand it correctly, now it looks like 2 stack frames must be ignored(for hwasan_tag_mismatch4 and HwasanTagMismatch) to get a proper backtrace.
What do you think?

Thanks

Yes, but I am not sure we can *rely* on that being the case as is. LTO could conceivably inline this – in which case it would be one, right?

compiler-rt/lib/hwasan/hwasan.cpp

205

This is probably for another patch though, right? This is already like this on the LHS.

205

nevermind. i accidentally had this left, sent https://reviews.llvm.org/D131279 for that.

[fmayer]

compiler-rt/lib/hwasan/hwasan.cpp

205

Sorry, please disregard everything I said here (incl the link). I confused myself where this was posted, and this is all nonsense.

[smd]

compiler-rt/lib/hwasan/hwasan.cpp

205

@vitalybuka thanks for the reply.

However if you work on sanitizer, some frames can be a useful info

From my experience most of the time while I'm working on sanitizers I spend in gdb, so I don't actually care about what backtrace is being printed. So it might be a good idea to avoid confusing regular hwasan user and skip printing service hwasan frames.

Maybe we just could keep the current scheme, but get return address and frame pointer in __hwasan_tag_mismatch4 like it was before? Something like:

void HwasanTagMismatch(uptr addr, uptr pc, uptr frame, uptr access_info, uptr *registers_frame, size_t outsize) {
  __hwasan::AccessInfo ai;
  ai.is_store = access_info & 0x10;
  ai.is_load = !ai.is_store;
  ai.recover = access_info & 0x20;
  ai.addr = addr;
  if ((access_info & 0xf) == 0xf)
    ai.size = outsize;
  else
    ai.size = 1 << (access_info & 0xf);

  HandleTagMismatch(ai, pc, frame, nullptr, registers_frame);
}
...
void __hwasan_tag_mismat(uptr)__builtin_frame_address(0)ch4(uptr addr, uptr access_info, uptr *registers_frame,
                            size_t outsize) {
  uptr ra = (uptr)__builtin_return_address(0);
  uptr fp = (uptr)__builtin_frame_address(0);
  __hwasan::HwasanTagMismatch(addr, ra, fp, access_info, registers_frame, outsize);
}

@fmayer

LTO could conceivably inline this – in which case it would be one, right?

Yes, you're totally right.

compiler-rt/lib/hwasan/hwasan.h

 
 #define GET_FATAL_STACK_TRACE_PC_BP(pc, bp)              \
   BufferedStackTrace stack;                              \
   if (hwasan_inited)                                     \
     stack.Unwind(pc, bp, nullptr, common_flags()->fast_unwind_on_fatal)
 
 void HwasanTSDInit();
 void HwasanTSDThreadInit();
+void HwasanAtExit();
 
 void HwasanOnDeadlySignal(int signo, void *info, void *context);
 
 void UpdateMemoryUsage();
 
 void AppendToErrorMessageBuffer(const char *buffer);
 
 void AndroidTestTlsSlot();
 
+// This is a compiler-generated struct that can be shared between hwasan
+// implementations.
+struct AccessInfo {
+  uptr addr;
+  uptr size;
+  bool is_store;
+  bool is_load;
+  bool recover;
+};
+
+// Given access info and frame information, unwind the stack and report the tag
+// mismatch.
+void HandleTagMismatch(AccessInfo ai, uptr pc, uptr frame, void *uc,
+                       uptr *registers_frame = nullptr);
+
+// This dispatches to HandleTagMismatch but sets up the AccessInfo, program
+// counter, and frame pointer.
+void HwasanTagMismatch(uptr addr, uptr access_info, uptr *registers_frame,
+                       size_t outsize);
+
 }  // namespace __hwasan
 
 #define HWASAN_MALLOC_HOOK(ptr, size)       \
   do {                                    \
     if (&__sanitizer_malloc_hook) {       \
       __sanitizer_malloc_hook(ptr, size); \
     }                                     \
     RunMallocHooks(ptr, size);            \

compiler-rt/lib/hwasan/hwasan.cpp

void UpdateMemoryUsage() {
   HwasanFormatMemoryUsage(s);
   internal_strncpy(memory_usage_buffer, s.data(), kMemoryUsageBufferSize - 1);
   memory_usage_buffer[kMemoryUsageBufferSize - 1] = '\0';
 }
 #else
 void UpdateMemoryUsage() {}
 #endif
 
+void HwasanAtExit() {
+  if (common_flags()->print_module_map)
+    DumpProcessMap();
+  if (flags()->print_stats && (flags()->atexit || hwasan_report_count > 0))
+    ReportStats();
+  if (hwasan_report_count > 0) {
+    // ReportAtExitStatistics();
+    if (common_flags()->exitcode)
+      internal__exit(common_flags()->exitcode);
+  }
+}
+
+void HandleTagMismatch(AccessInfo ai, uptr pc, uptr frame, void *uc,
+                       uptr *registers_frame) {
+  InternalMmapVector<BufferedStackTrace> stack_buffer(1);
+  BufferedStackTrace *stack = stack_buffer.data();
+  stack->Reset();
+  stack->Unwind(pc, frame, uc, common_flags()->fast_unwind_on_fatal);
+
+  // The second stack frame contains the failure __hwasan_check function, as
+  // we have a stack frame for the registers saved in __hwasan_tag_mismatch that
+  // we wish to ignore. This (currently) only occurs on AArch64, as x64
+  // implementations use SIGTRAP to implement the failure, and thus do not go
+  // through the stack saver.
+  if (registers_frame && stack->trace && stack->size > 0) {
+    stack->trace++;
+    stack->size--;
+  }
+
+  bool fatal = flags()->halt_on_error || !ai.recover;
+  ReportTagMismatch(stack, ai.addr, ai.size, ai.is_store, fatal,
+                    registers_frame);
+}
+
+void HwasanTagMismatch(uptr addr, uptr access_info, uptr *registers_frame,
+                       size_t outsize) {
+  __hwasan::AccessInfo ai;
+  ai.is_store = access_info & 0x10;
+  ai.is_load = !ai.is_store;
+  ai.recover = access_info & 0x20;
+  ai.addr = addr;
+  if ((access_info & 0xf) == 0xf)
+    ai.size = outsize;
+  else
+    ai.size = 1 << (access_info & 0xf);
+
+  HandleTagMismatch(ai, (uptr)__builtin_return_address(0),
+                    (uptr)__builtin_frame_address(0), nullptr, registers_frame);
+  __builtin_unreachable();
+}
+
 } // namespace __hwasan
 
 using namespace __hwasan;
 
 void __sanitizer::BufferedStackTrace::UnwindImpl(
     uptr pc, uptr bp, void *context, bool request_fast, u32 max_depth) {
   Thread *t = GetCurrentThread();
   if (!t) {

compiler-rt/lib/hwasan/hwasan_linux.cpp

 // Memory outside the alias range has non-zero tags.
 #  if !defined(HWASAN_ALIASING_MODE)
   CHECK(GetTagFromPointer(p) == 0);
 #  endif
 
   return p >= kHighMemStart || (p >= kLowMemStart && p <= kLowMemEnd);
 }
 
-static void HwasanAtExit(void) {
-  if (common_flags()->print_module_map)
-    DumpProcessMap();
-  if (flags()->print_stats && (flags()->atexit || hwasan_report_count > 0))
-    ReportStats();
-  if (hwasan_report_count > 0) {
-    // ReportAtExitStatistics();
-    if (common_flags()->exitcode)
-      internal__exit(common_flags()->exitcode);
-  }
-}
-
 void InstallAtExitHandler() {
   atexit(HwasanAtExit);
 }
 
 // ---------------------- TSD ---------------- {{{1
 
 extern "C" void __hwasan_thread_enter() {
   hwasanThreadList().CreateCurrentThread()->InitRandomState();

 Thread *GetCurrentThread() {
   uptr *ThreadLongPtr = GetCurrentThreadLongPtr();
   if (UNLIKELY(*ThreadLongPtr == 0))
     return nullptr;
   auto *R = (StackAllocationsRingBuffer *)ThreadLongPtr;
   return hwasanThreadList().GetThreadByBufferAddress((uptr)R->Next());
 }
 
-struct AccessInfo {
-  uptr addr;
-  uptr size;
-  bool is_store;
-  bool is_load;
-  bool recover;
-};
-
 static AccessInfo GetAccessInfo(siginfo_t *info, ucontext_t *uc) {
   // Access type is passed in a platform dependent way (see below) and encoded
   // as 0xXY, where X&1 is 1 for store, 0 for load, and X&2 is 1 if the error is
   // recoverable. Valid values of Y are 0 to 4, which are interpreted as
   // log2(access_size), and 0xF, which means that access size is passed via
   // platform dependent register (see below).
 #if defined(__aarch64__)
   // Access type is encoded in BRK immediate as 0x900 + 0xXY. For Y == 0xF,

 
 #else
 # error Unsupported architecture
 #endif
 
   return AccessInfo{addr, size, is_store, !is_store, recover};
 }
 
-static void HandleTagMismatch(AccessInfo ai, uptr pc, uptr frame,
-                              ucontext_t *uc, uptr *registers_frame = nullptr) {
-  InternalMmapVector<BufferedStackTrace> stack_buffer(1);
-  BufferedStackTrace *stack = stack_buffer.data();
-  stack->Reset();
-  stack->Unwind(pc, frame, uc, common_flags()->fast_unwind_on_fatal);
-
-  // The second stack frame contains the failure __hwasan_check function, as
-  // we have a stack frame for the registers saved in __hwasan_tag_mismatch that
-  // we wish to ignore. This (currently) only occurs on AArch64, as x64
-  // implementations use SIGTRAP to implement the failure, and thus do not go
-  // through the stack saver.
-  if (registers_frame && stack->trace && stack->size > 0) {
-    stack->trace++;
-    stack->size--;
-  }
-
-  bool fatal = flags()->halt_on_error || !ai.recover;
-  ReportTagMismatch(stack, ai.addr, ai.size, ai.is_store, fatal,
-                    registers_frame);
-}
-
 static bool HwasanOnSIGTRAP(int signo, siginfo_t *info, ucontext_t *uc) {
   AccessInfo ai = GetAccessInfo(info, uc);
   if (!ai.is_store && !ai.is_load)
     return false;
 
   SignalContext sig{info, uc};
   HandleTagMismatch(ai, StackTrace::GetNextInstructionPc(sig.pc), sig.bp, uc);
 

 
 
 } // namespace __hwasan
 
 // Entry point for interoperability between __hwasan_tag_mismatch (ASM) and the
 // rest of the mismatch handling code (C++).
 void __hwasan_tag_mismatch4(uptr addr, uptr access_info, uptr *registers_frame,
                             size_t outsize) {
-  __hwasan::AccessInfo ai;
-  ai.is_store = access_info & 0x10;
-  ai.is_load = !ai.is_store;
-  ai.recover = access_info & 0x20;
-  ai.addr = addr;
-  if ((access_info & 0xf) == 0xf)
-    ai.size = outsize;
-  else
-    ai.size = 1 << (access_info & 0xf);
-
-  __hwasan::HandleTagMismatch(ai, (uptr)__builtin_return_address(0),
-                              (uptr)__builtin_frame_address(0), nullptr,
-                              registers_frame);
-  __builtin_unreachable();
+  __hwasan::HwasanTagMismatch(addr, access_info, registers_frame, outsize);
 }
 
 #endif // SANITIZER_FREEBSD || SANITIZER_LINUX || SANITIZER_NETBSD