From bcffddf04b818412d52b46fd02a5205be5d96251 Mon Sep 17 00:00:00 2001 From: Dan Liew Date: Mon, 6 Apr 2020 18:57:54 -0700 Subject: [PATCH] [Darwin] Fix a bug where the symbolizer would examine the wrong process. Summary: 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.dylib: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. Reviewers: kubamracek, yln Subscribers: #sanitizers, llvm-commits Tags: #sanitizers Differential Revision: https://reviews.llvm.org/D77623 (cherry picked from commit 8efc3ccaf808caeba395f71449524830f7fe1d09) --- .../sanitizer_symbolizer_mac.cpp | 13 ++-- ...-stack-trace-in-code-loaded-after-fork.cpp | 60 +++++++++++++++++++ 2 files changed, 68 insertions(+), 5 deletions(-) create mode 100644 compiler-rt/test/sanitizer_common/TestCases/Darwin/print-stack-trace-in-code-loaded-after-fork.cpp diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp index f26efe5c50b55..1379ed82d03d1 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp @@ -52,16 +52,19 @@ bool DlAddrSymbolizer::SymbolizeData(uptr addr, DataInfo *datainfo) { 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 @@ static bool ParseCommandOutput(const char *str, uptr addr, char **out_name, } 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; diff --git a/compiler-rt/test/sanitizer_common/TestCases/Darwin/print-stack-trace-in-code-loaded-after-fork.cpp b/compiler-rt/test/sanitizer_common/TestCases/Darwin/print-stack-trace-in-code-loaded-after-fork.cpp new file mode 100644 index 0000000000000..db4cdf8f9b6da --- /dev/null +++ b/compiler-rt/test/sanitizer_common/TestCases/Darwin/print-stack-trace-in-code-loaded-after-fork.cpp @@ -0,0 +1,60 @@ +// 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 + +#ifdef SHARED_LIB +#include + +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 +#include +#include +#include +#include + +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); + // We load new code into the child process that isn't loaded into the parent. + // When we symbolize in `PrintStack` if the symbolizer is told to symbolize + // the parent (an old bug) rather than the child then symbolization will + // fail. + 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