[PATCH] D77623: [Darwin] Fix a bug where the symbolizer would examine the wrong process.

[Dan Liew via Phabricator via llvm-commits]

delcypher created this revision.
delcypher added reviewers: kubamracek, yln.
Herald added a project: Sanitizers.
Herald added a subscriber: Sanitizers.
delcypher added a parent revision: D77622: [Sanitizer Common] Show command used to launch symbolizer process at high verbosity level..

Previously `AtosSymbolizer` would set the PID to examine in the
constructor which is called early on during sanitizer init. This can
lead to incorrect behaviour in the case of a fork() because if the
symbolizer is launched in the child it will be told examine the parent
process rather than the child.

To fix this the PID is determined just before the symbolizer is
launched.

A test case is included that triggers the buggy behaviour that existed
prior to this patch. The test observes the PID that `atos` was called
on. It also examines the symbolized stacktrace. Prior to this patch
`atos` failed to symbolize the stacktrace giving output that looked
like...

  #0 0x100fc3bb5 in __sanitizer_print_stack_trace asan_stack.cpp:86
  #1 0x10490dd36 in PrintStack+0x56 (/path/to/print-stack-trace-in-code-loaded-after-fork.cpp.tmp_shared_lib:x86_64+0xd36)
  #2 0x100f6f986 in main+0x4a6 (path/to/print-stack-trace-in-code-loaded-after-fork.cpp.tmp_loader:x86_64+0x100001986)
  #3 0x7fff714f1cc8 in start+0x0 (/usr/lib/system/libdyld.dylib:x86_64+0x1acc8)

After this patch stackframes `#1` and `#2` are fully symbolized.

This patch is also a pre-requisite refactor for rdar://problem/58789439.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77623

Files:
  compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp
  compiler-rt/test/sanitizer_common/TestCases/Darwin/print-stack-trace-in-code-loaded-after-fork.cpp


Index: compiler-rt/test/sanitizer_common/TestCases/Darwin/print-stack-trace-in-code-loaded-after-fork.cpp
===================================================================
--- /dev/null
+++ compiler-rt/test/sanitizer_common/TestCases/Darwin/print-stack-trace-in-code-loaded-after-fork.cpp
@@ -0,0 +1,56 @@
+// RUN: %clangxx %s -g -DSHARED_LIB -shared -o %t_shared_lib.dylib
+// RUN: %clangxx %s -g -USHARED_LIB -o %t_loader
+// RUN: %env_tool_opts=verbosity=3 %run %t_loader %t_shared_lib.dylib > %t_loader_output.txt 2>&1
+// RUN: FileCheck -input-file=%t_loader_output.txt %s
+// RUN: FileCheck -check-prefix=CHECK-STACKTRACE -input-file=%t_loader_output.txt %s
+
+#include <stdio.h>
+
+#ifdef SHARED_LIB
+#include <sanitizer/common_interface_defs.h>
+
+extern "C" void PrintStack() {
+  fprintf(stderr, "Calling __sanitizer_print_stack_trace\n");
+  // CHECK-STACKTRACE: #0{{( *0x.* *in *)?}}  __sanitizer_print_stack_trace
+  // CHECK-STACKTRACE: #1{{( *0x.* *in *)?}} PrintStack {{.*}}print-stack-trace-in-code-loaded-after-fork.cpp:[[@LINE+1]]
+  __sanitizer_print_stack_trace();
+}
+#else
+#include <assert.h>
+#include <dlfcn.h>
+#include <stdlib.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+typedef void (*PrintStackFnPtrTy)(void);
+
+int main(int argc, char **argv) {
+  assert(argc == 2);
+  pid_t pid = fork();
+  if (pid != 0) {
+    // Parent
+    pid_t parent_pid = getpid();
+    fprintf(stderr, "parent: %d\n", parent_pid);
+    int status = 0;
+    pid_t child = waitpid(pid, &status, /*options=*/0);
+    assert(pid == child);
+    bool clean_exit = WIFEXITED(status) && WEXITSTATUS(status) == 0;
+    return !clean_exit;
+  }
+  // Child.
+  pid = getpid();
+  // CHECK: child: [[CHILD_PID:[0-9]+]]
+  fprintf(stderr, "child: %d\n", pid);
+  const char *library_to_load = argv[1];
+  void *handle = dlopen(library_to_load, RTLD_NOW | RTLD_LOCAL);
+  assert(handle);
+  PrintStackFnPtrTy PrintStackFnPtr = (PrintStackFnPtrTy)dlsym(handle, "PrintStack");
+  assert(PrintStackFnPtr);
+  // Check that the symbolizer is told examine the child process.
+  // CHECK: Launching Symbolizer process: {{.+}}atos -p [[CHILD_PID]]
+  // CHECK-STACKTRACE: #2{{( *0x.* *in *)?}} main {{.*}}print-stack-trace-in-code-loaded-after-fork.cpp:[[@LINE+1]]
+  PrintStackFnPtr();
+  return 0;
+}
+
+#endif
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
@@ -52,16 +52,19 @@
 
 class AtosSymbolizerProcess : public SymbolizerProcess {
  public:
-  explicit AtosSymbolizerProcess(const char *path, pid_t parent_pid)
+  explicit AtosSymbolizerProcess(const char *path)
       : SymbolizerProcess(path, /*use_posix_spawn*/ true) {
-    // Put the string command line argument in the object so that it outlives
-    // the call to GetArgV.
-    internal_snprintf(pid_str_, sizeof(pid_str_), "%d", parent_pid);
+    pid_str_[0] = '\0';
   }
 
  private:
   bool StartSymbolizerSubprocess() override {
     // Configure sandbox before starting atos process.
+
+    // Put the string command line argument in the object so that it outlives
+    // the call to GetArgV.
+    internal_snprintf(pid_str_, sizeof(pid_str_), "%d", internal_getpid());
+
     return SymbolizerProcess::StartSymbolizerSubprocess();
   }
 
@@ -138,7 +141,7 @@
 }
 
 AtosSymbolizer::AtosSymbolizer(const char *path, LowLevelAllocator *allocator)
-    : process_(new(*allocator) AtosSymbolizerProcess(path, getpid())) {}
+    : process_(new (*allocator) AtosSymbolizerProcess(path)) {}
 
 bool AtosSymbolizer::SymbolizePC(uptr addr, SymbolizedStack *stack) {
   if (!process_) return false;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D77623.255569.patch
Type: text/x-patch
Size: 3782 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200407/64a4033e/attachment.bin>