[PATCH] D84262: Avoid failing a CHECK in `DlAddrSymbolizer::SymbolizePC`.

[Dan Liew via Phabricator via llvm-commits]

delcypher created this revision.
delcypher added reviewers: kubamracek, yln.
Herald added subscribers: Sanitizers, kristof.beyls.
Herald added a project: Sanitizers.

It turns out the `CHECK(addr >= reinterpret_cast<upt>(info.dli_saddr)`
can fail because on armv7s on iOS 9.3 `dladdr()` returns
`info.dli_saddr` with an address larger than the address we provided.

We should avoid crashing here because crashing in the middle of reporting
an issue is very unhelpful. Instead we now try to compute a function offset
if the value we get back from `dladdr()` looks sane, otherwise we don't
set the function offset.

A test case is included. It's basically a slightly modified version of
the existing `test/sanitizer_common/TestCases/Darwin/symbolizer-function-offset-dladdr.cpp`
test case that doesn't run on iOS devices right now.

More details:

In the concrete scenario on armv7s `addr` is `0x2195c870` and the returned
`info.dli_saddr` is `0x2195c871`.

This what LLDB says when disassembling the code.

  (lldb) dis -a 0x2195c870
  libdyld.dylib`<redacted>:
      0x2195c870 <+0>: nop
      0x2195c872 <+2>: blx    0x2195c91c                ; symbol stub for: exit
      0x2195c876 <+6>: trap

The value returned by `dladdr()` doesn't make sense because it points
into the middle of a instruction.

There might also be other bugs lurking here because I noticed that the PCs we
gather during stackunwinding (before changing them with
`StackTrace::GetPreviousInstructionPc()`) look a little suspicious (e.g.  the
PC stored for the frame with fail to symbolicate is 0x2195c873) as they don't
look properly aligned. This probably warrants further investigation in the future.

rdar://problem/65621511


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84262

Files:
  compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp
  compiler-rt/test/asan/TestCases/Darwin/symbolizer-function-offset-dladdr.cpp


Index: compiler-rt/test/asan/TestCases/Darwin/symbolizer-function-offset-dladdr.cpp
===================================================================
--- /dev/null
+++ compiler-rt/test/asan/TestCases/Darwin/symbolizer-function-offset-dladdr.cpp
@@ -0,0 +1,39 @@
+// FIXME(dliew): Duplicated from `test/sanitizer_common/TestCases/Darwin/symbolizer-function-offset-dladdr.cpp`.
+// This case can be dropped once sanitizer_common tests work on iOS devices (rdar://problem/47333049).
+
+// RUN: %clangxx_asan %s -O0 -o %t
+// RUN: %env_asan_opts=verbosity=2,external_symbolizer_path=,stack_trace_format='"function_name_%f___function_offset_%q"' %run %t > %t.output 2>&1
+// RUN: FileCheck -input-file=%t.output %s
+#include <sanitizer/common_interface_defs.h>
+#include <stdio.h>
+
+void baz() {
+  printf("Do stuff in baz\n");
+  __sanitizer_print_stack_trace();
+}
+
+void bar() {
+  printf("Do stuff in bar\n");
+  baz();
+}
+
+void foo() {
+  printf("Do stuff in foo\n");
+  bar();
+}
+
+int main() {
+  printf("Do stuff in main\n");
+  foo();
+  return 0;
+}
+
+// CHECK: External symbolizer is explicitly disabled
+// CHECK: Using dladdr symbolizer
+
+// These `function_offset` patterns are designed to disallow `0x0` which is the
+// value printed for `kUnknown`.
+// CHECK: function_name_baz{{(\(\))?}}___function_offset_0x{{0*[1-9a-f][0-9a-f]*$}}
+// CHECK: function_name_bar{{(\(\))?}}___function_offset_0x{{0*[1-9a-f][0-9a-f]*$}}
+// CHECK: function_name_foo{{(\(\))?}}___function_offset_0x{{0*[1-9a-f][0-9a-f]*$}}
+// CHECK: function_name_main{{(\(\))?}}___function_offset_0x{{0*[1-9a-f][0-9a-f]*$}}
Index: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp
===================================================================
--- compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp
+++ compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp
@@ -33,8 +33,15 @@
   int result = dladdr((const void *)addr, &info);
   if (!result) return false;
 
-  CHECK(addr >= reinterpret_cast<uptr>(info.dli_saddr));
-  stack->info.function_offset = addr - reinterpret_cast<uptr>(info.dli_saddr);
+  // Compute offset if possible. `dladdr()` doesn't always ensure that `addr >=
+  // sym_addr` so only compute the offset when this holds. Failure to find the
+  // function offset is not treated as a failure because it might still be
+  // possible to get the symbol name.
+  uptr sym_addr = reinterpret_cast<uptr>(info.dli_saddr);
+  if (addr >= sym_addr) {
+    stack->info.function_offset = addr - sym_addr;
+  }
+
   const char *demangled = DemangleSwiftAndCXX(info.dli_sname);
   if (!demangled) return false;
   stack->info.function = internal_strdup(demangled);
@@ -219,10 +226,10 @@
       start_address = reinterpret_cast<uptr>(info.dli_saddr);
   }
 
-  // Only assig to `function_offset` if we were able to get the function's
-  // start address.
-  if (start_address != AddressInfo::kUnknown) {
-    CHECK(addr >= start_address);
+  // Only assign to `function_offset` if we were able to get the function's
+  // start address and we got a sensible `start_address` (dladdr doesn't always
+  // ensure that `addr >= sym_addr`).
+  if (start_address != AddressInfo::kUnknown && addr >= start_address) {
     stack->info.function_offset = addr - start_address;
   }
   return true;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D84262.279606.patch
Type: text/x-patch
Size: 3314 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200721/2263c3d3/attachment.bin>