[Darwin] Fix symbolization for recent simulator runtimes.

[delcypher]

Summary

Due to sandbox restrictions in the recent versions of the simulator runtime the
atos program is no longer able to access the task port of a parent process
without additional help.

This patch fixes this by registering a task port for the parent process
before spawning atos and also tells atos to look for this by setting
a special environment variable.

This patch is based on an Apple internal fix (rdar://problem/43693565) that
unfortunately contained a bug (rdar://problem/58789439) because it used
setenv() to set the special environment variable. This is not safe because in
certain circumstances this can trigger a call to realloc() which can fail
during symbolization leading to deadlock. A test case is included that captures
this problem.

This patch takes a different approach by

1. Calling putenv() early during process init (but late enough that

malloc/realloc works) to set a dummy value for the environment variable.

2. Just before atos is spawned the storage for the environment

variable is modified to contain the correct PID.

The issue reported in rdar://problem/58789439 manifested as a deadlock
during symbolization in the following situation:

1. Before TSan detects an issue Something calls setenv() that sets a

new environment variable that wasn't previously set. This triggers a
call to malloc() to allocate a new environment array. This uses TSan's
normal user-facing allocator. LibC stores this pointer for future use
later.

2. TSan detects an issue and tries to launch the symbolizer. When we are in the

symbolizer we switch to a different (internal allocator) and then we call
setenv() to set a new environment variable. When this happen setenv() sees
that it needs to make the environment array larger and calls realloc() on the
existing enviroment array because it remembers that it previously allocated
memory for it. Calling realloc() fails here because it is being called on a
pointer its never seen before.

The included test case closely reproduces the originally reported
problem but it doesn't replicate the `((kBlockMagic)) ==
((((u64*)addr)[0])` assertion failure exactly. This is due to the way
TSan's normal allocator allocates the environment array the first time
it is allocated. In the test program addr[0] accesses an inaccessible
page and raises SIGBUS. If TSan's SIGBUS signal handler is active, the
signal is caught and symbolication is attempted again which results in
deadlock.

In the originally reported problem the pointer is successfully derefenced but
then the assert fails due to the provided pointer not coming from the active
allocator. When the assert fails TSan tries to symbolicate the stacktrace while
already being in the middle of symbolication which results in deadlock.

rdar://problem/58789439

[delcypher]

• Detect env var being messed with gracefully.
• Small clean up.

[kubamracek]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

32

Can you drop these non-essential changes, please?

53

Can you sink this into the class please?

74

Just for readability, please move the putenv call outside of the CHECK.

98

I'd rather not call getenv here too. We can just access atos_mach_port_env_var_entry_ directly, no? Also, why not just unconditionally call updateAtosEnvVar?

110

All methods here in this class start with uppercase letter. Let's keep it that way, please.

238

Who's calling this?

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.h

38

I don't see a virtual LateInitialize in SymbolizerTool... is this supposed to not be an "override"?

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

32

Sorry clang-format changed this and I didn't notice.

74

Sure.

98

The reason is that I wanted to check our assumption is that the __check_mach_ports_lookup environment variable is still in the environment and that we still control it.

This assumption could be broken by the user's code changing the environment.

If you don't care about detecting this case so that we can fail more gracefully then I can rip this out which also means I can rip out getValueFromEnvEntry() as well.

110

Oops I missed this. I'll fix that.

238

This will be called via Symbolizer::LateInitialize()

Please see the patch (https://reviews.llvm.org/D78178) that this patch depends on.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

53

If I try to sink this into the class then this doesn't compile.

/Volumes/data/dev/llvm/monorepo_upstream/master/llvm/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:152:21: error: in-class initializer for static data member of type 'const char [26]' requires 'constexpr' specifier
  static const char kAtosEnvVar[] = "__check_mach_ports_lookup";
                    ^               ~~~~~~~~~~~~~~~~~~~~~~~~~~~
  constexpr

If I make it constexpr then we'll get linking error because we need the memory to exist at runtime.

Undefined symbols for architecture i386:
  "__sanitizer::AtosSymbolizerProcess::kAtosEnvVar", referenced from:
      __sanitizer::AtosSymbolizer::AtosSymbolizer(char const*, __sanitizer::LowLevelAllocator*) in sanitizer_symbolizer_mac.cpp.o
      __sanitizer::AtosSymbolizer::AtosSymbolizer(char const*, __sanitizer::LowLevelAllocator*) in sanitizer_symbolizer_mac.cpp.o
      __sanitizer::AtosSymbolizerProcess::StartSymbolizerSubprocess() in sanitizer_symbolizer_mac.cpp.o

To fix this I'd have initialize the static data member outside the class which means I'd have to do...

// In class
static const char kAtosEnvVar[26];

// Outside of class
const char AtosSymbolizerProcess::kAtosEnvVar[] = "__check_mach_ports_lookup";

This seems clumsy to me because I have to hardcode the 26 size.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.h

38

Please see the patch that this patch depends on: https://reviews.llvm.org/D78178

[kubamracek]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

98

Yes, let's not worry about the user overwriting that env var.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

53

Do you have a preference between the global var and the clumsy static data member?

98

Are you have for me to drop the check of the assumption?

[delcypher]

• Address first round of feedback.

[delcypher]

@kubamracek Any additional comments?

[delcypher]

Correct typo in comment.

[delcypher]

• Update radar number.

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

68

What's our stance on if(<defined-value>) vs #ifdef? In compiler-rt we seem to usually use #ifdef. Should we aim for consistency here?

69

Thanks for the explanation of why this is needed! :)

98

If you remove the unnecessary call in the constructor, then this is the only usage of the function and we can inline it.

130

I think you can get away without the defining kAtosEnvVar_ at all:
Here:
char atos_mach_port_env_var_entry_[sizeof(K_ATOS_ENV_VAR) + sizeof(pid_str_)];
Above: make it part of the format string:
internal_snprintf(.., .., K_ATOS_ENV_VAR "=%s", pid_str);

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.h

38

Just a quick question: will the principled resolution (where we modify the env only for posix_spawn, but not in our own process) require this as well. If not, is the plan to remove it again?

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

68

SANITIZER_IOSSIM is always defined to be 0 or 1. I thought it was better than this code be compiled on all paths because not everyone builds for iOS simulators in open source.

98

@yln

If you remove the unnecessary call in the constructor, then this is the only usage of the function and we can inline it.

The call in the constructor is essential. When you call putenv() Darwin's LibC checks the data is well formed so we have to initialize atos_mach_port_env_var_entry_ before giving it to putenv().

Later on we need to modify atos_mach_port_env_var_entry_ again to have it contain the right PID so it made sense to refactor this repeated action into UpdateAtosEnvVar().

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

130

@yln
Originally this was a lot cleaner. It was just this at global scope.

static const char kAtosEnvVar[] = "__check_mach_ports_lookup";

However @kubamracek wanted this sunk into the class itself. This resulted in the mess that's in this current patch. I much preferred how it was before because I didn't need to use macros at all.

The approach you've laid out is a definite improvement over what's currently in the patch but it still involves macros.

@kubamracek @yln Can we pick something? Either what @yln is suggesting or my original approach (global variable). I prefer the original approach because it doesn't involve any macros.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

98

@yln Sorry a little bit more on this. The call to UpdateAtosEnvVar() is essential but it doesn't actually need to be in the constructor. We could move it into LateInitialize() to set atos_mach_port_env_var_entry_ just before we give it to putenv(). Is that what you'd prefer?

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

98

How about the following?

Initialize to default that is good enough to use for putenv():

// Comment about '\0' and '='
char AtosSymbolizerProcess::atos_mach_port_env_var_entry_[sizeof(K_ATOS_ENV_VAR) + sizeof(pid_str_)] = K_ATOS_ENV_VAR "=0";

Do nothing in the ctor (UpdateAtosEnvVar can be inlined).

UpdateAtosEnvVar() becomes:

dst = &atos_mach_port_env_var_entry_[sizeof(K_ATOS_ENV_VAR)];
strlcpy(dst, pid_str, ...);

Nit: use less verbose name for atos_mach_port_env_var_entry_

Double check the buffer sizes/string offsets/space for =...

[delcypher]

• Adopt Julian's macro suggestions.
• Inline UpdateAtosEnvVar().
• atos_mach_port_env_var_entry_ -> mach_port_env_var_entry_

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

98

I tried doing this with constexpr but I gave up as I would need to be able to concatenate strings and I'd have to implement a bunch of infrastructure to do that. So I went with your macro approach.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.h

38

The principled resolution does not depend on https://reviews.llvm.org/D78178.
However, I'm abandoning the principled resolution due to complexity involved in copying the environment and modifying the copy. I had it all implemented but it needs a lot of clean up for adoption in open source which I don't have time to do.

[delcypher]

@yln @kubamracek Good to go?

[yln]

LGTM, with two more tiny nits.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

101

We could assert "equal" on the exact count sizeof(K_ATOS_ENV_VAR) + 1 + internal_strlen(pid_str_)

135

Is this considered good hygiene? Personally, I would drop it, but I am fine either way.

[delcypher]

• Make check on return of internal_snprintf() tighter.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

101

Done. You don't need the +1 though .

135

I consider it good hygiene for the macro to be defined for only as long as its needed.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.h

 class AtosSymbolizerProcess;
 
 class AtosSymbolizer : public SymbolizerTool {
  public:
   explicit AtosSymbolizer(const char *path, LowLevelAllocator *allocator);
 
   bool SymbolizePC(uptr addr, SymbolizedStack *stack) override;
   bool SymbolizeData(uptr addr, DataInfo *info) override;
+  void LateInitialize() override;
 
  private:
   AtosSymbolizerProcess *process_;
 };
 
 } // namespace __sanitizer
 
 #endif  // SANITIZER_MAC
 
 #endif // SANITIZER_SYMBOLIZER_MAC_H

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

 #if SANITIZER_MAC
 
 #include "sanitizer_allocator_internal.h"
 #include "sanitizer_mac.h"
 #include "sanitizer_symbolizer_mac.h"
 
 #include <dlfcn.h>
 #include <errno.h>
+#include <mach/mach.h>
 #include <stdlib.h>
 #include <sys/wait.h>
 #include <unistd.h>
 #include <util.h>
 
 namespace __sanitizer {
 
 bool DlAddrSymbolizer::SymbolizePC(uptr addr, SymbolizedStack *stack) {
   Dl_info info;
   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);
   const char *demangled = DemangleSwiftAndCXX(info.dli_sname);
   if (!demangled) return false;
   stack->info.function = internal_strdup(demangled);
   return true;
 }
 
 bool DlAddrSymbolizer::SymbolizeData(uptr addr, DataInfo *datainfo) {
   Dl_info info;
   int result = dladdr((const void *)addr, &info);
   if (!result) return false;
   const char *demangled = DemangleSwiftAndCXX(info.dli_sname);
   datainfo->name = internal_strdup(demangled);
   datainfo->start = (uptr)info.dli_saddr;
   return true;
 }
 
+#define K_ATOS_ENV_VAR "__check_mach_ports_lookup"
+
 class AtosSymbolizerProcess : public SymbolizerProcess {
  public:
   explicit AtosSymbolizerProcess(const char *path)
       : SymbolizerProcess(path, /*use_posix_spawn*/ true) {
     pid_str_[0] = '\0';
   }
 
+  void LateInitialize() {
+    if (SANITIZER_IOSSIM) {
+      // `putenv()` may call malloc/realloc so it is only safe to do this
+      // during LateInitialize() or later (i.e. we can't do this in the
+      // constructor).  We also can't do this in `StartSymbolizerSubprocess()`
+      // because in TSan we switch allocators when we're symbolizing.
+      // We use `putenv()` rather than `setenv()` so that we can later directly
+      // write into the storage without LibC getting involved to change what the
+      // variable is set to
+      int result = putenv(mach_port_env_var_entry_);
+      CHECK_EQ(result, 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());
 
+    if (SANITIZER_IOSSIM) {
+      // `atos` in the simulator is restricted in its ability to retrieve the
+      // task port for the target process (us) so we need to do extra work
+      // to pass our task port to it.
+      mach_port_t ports[]{mach_task_self()};
+      kern_return_t ret =
+          mach_ports_register(mach_task_self(), ports, /*count=*/1);
+      CHECK_EQ(ret, KERN_SUCCESS);
+
+      // Set environment variable that signals to `atos` that it should look
+      // for our task port. We can't call `setenv()` here because it might call
+      // malloc/realloc. To avoid that we instead update the
+      // `mach_port_env_var_entry_` variable with our current PID.
+      uptr count = internal_snprintf(mach_port_env_var_entry_,
+                                     sizeof(mach_port_env_var_entry_),
+                                     K_ATOS_ENV_VAR "=%s", pid_str_);
+      CHECK_GE(count, sizeof(K_ATOS_ENV_VAR) + internal_strlen(pid_str_));
+      // Document our assumption but without calling `getenv()` in normal
+      // builds.
+      DCHECK_EQ(internal_strcmp(getenv(K_ATOS_ENV_VAR), pid_str_), 0);
+    }
+
     return SymbolizerProcess::StartSymbolizerSubprocess();
   }
 
   bool ReachedEndOfOutput(const char *buffer, uptr length) const override {
     return (length >= 1 && buffer[length - 1] == '\n');
   }
 
   void GetArgV(const char *path_to_binary,
                const char *(&argv)[kArgVMax]) const override {
     int i = 0;
     argv[i++] = path_to_binary;
     argv[i++] = "-p";
     argv[i++] = &pid_str_[0];
     if (GetMacosVersion() == MACOS_VERSION_MAVERICKS) {
       // On Mavericks atos prints a deprecation warning which we suppress by
       // passing -d. The warning isn't present on other OSX versions, even the
       // newer ones.
       argv[i++] = "-d";
     }
     argv[i++] = nullptr;
   }
 
   char pid_str_[16];
+  // Space for `\0` in `kAtosEnvVar_` is reused for `=`.
+  char mach_port_env_var_entry_[sizeof(K_ATOS_ENV_VAR) + sizeof(pid_str_)] =
+      K_ATOS_ENV_VAR "=0";
 };
 
+#undef K_ATOS_ENV_VAR
+
 static bool ParseCommandOutput(const char *str, uptr addr, char **out_name,
                                char **out_module, char **out_file, uptr *line,
                                uptr *start_address) {
   // Trim ending newlines.
   char *trim;
   ExtractTokenUpToDelimiter(str, "\n", &trim);
 
   // The line from `atos` is in one of these formats:
bool AtosSymbolizer::SymbolizeData(uptr addr, DataInfo *info) {
   if (!ParseCommandOutput(buf, addr, &info->name, &info->module, nullptr,
                           nullptr, &info->start)) {
     process_ = nullptr;
     return false;
   }
   return true;
 }
 
+void AtosSymbolizer::LateInitialize() { process_->LateInitialize(); }
+
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_MAC

compiler-rt/test/tsan/Darwin/no_call_setenv_in_symbolize.cpp

+// RUN: %clangxx_tsan -O1 %s -o %t
+// `handle_sigbus=0` is required because when the rdar://problem/58789439 bug was
+// present TSan's runtime could derefence bad memory leading to SIGBUS being raised.
+// If the signal was caught TSan would deadlock because it would try to run the
+// symbolizer again.
+// RUN: %env_tsan_opts=handle_sigbus=0,symbolize=1 %run %t 2>&1 | FileCheck %s
+// RUN: %env_tsan_opts=handle_sigbus=0,symbolize=1 __check_mach_ports_lookup=some_value %run %t 2>&1 | FileCheck %s
+#include <sanitizer/common_interface_defs.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+const char *kEnvName = "__UNLIKELY_ENV_VAR_NAME__";
+
+int main() {
+  if (getenv(kEnvName)) {
+    fprintf(stderr, "Env var %s should not be set\n", kEnvName);
+    abort();
+  }
+
+  // This will set an environment variable that isn't already in
+  // the environment array. This will cause Darwin's Libc to
+  // malloc() a new array.
+  if (setenv(kEnvName, "some_value", /*overwrite=*/1)) {
+    fprintf(stderr, "Failed to set %s \n", kEnvName);
+    abort();
+  }
+
+  // rdar://problem/58789439
+  // Now trigger symbolization. If symbolization tries to call
+  // to `setenv` that adds a new environment variable, then Darwin
+  // Libc will call `realloc()` and TSan's runtime will hit
+  // an assertion failure because TSan's runtime uses a different
+  // allocator during symbolization which leads to `realloc()` being
+  // called on a pointer that the allocator didn't allocate.
+  //
+  // CHECK: #{{[0-9]}} main {{.*}}no_call_setenv_in_symbolize.cpp:[[@LINE+1]]
+  __sanitizer_print_stack_trace();
+
+  // CHECK: DONE
+  fprintf(stderr, "DONE\n");
+
+  return 0;
+}