[Darwin] Teach `AtosSymbolizerProcess` to work on a copy of the environment.

[delcypher]

Summary

This refactor is a pre-requisite to fixing rdar://problem/58789439. In
future patches we will modify this copy to pass a different environment
to atos. Due to working on a copy of the existing environment it is
expected that this patch will produce no functional change in most
cases.

The storage size for the copy of the environment is set statically so we
avoid doing a heap allocation at symbolization time where we might be
unable to allocate heap memory (e.g. when an out-of-memory situation has
been hit). Note that although AtosSymbolizerProcess is itself heap
allocated it is done so during early init where we should have enough
available heap memory.

In the case that the current process has more environment variables than
can be stored (currently 511 + 1 for null terminator) then a warning is
produced and the copy of the environment is truncated. A test case is
included for this scenario which should hopefully be rare.

[yln]

LGTM with nits

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

54

Can be moved inside private section of class: it's a private implementation detail of the class.

59

I wouldn't do this (initialize members). My argument is *not* performance or that it is unnecessary, but rather that it makes it seem as if AtosSymbolizerProcess is fully usable after being constructed. Good C++ objects are fully initialized/usable after construction. AtosSymbolizerProcess is not like that. It's very special: you have to call its methods in a specific order for it to work. Stray of that path and it won't work correctly. And we can't fix that, not even by initializing its members with good (empty) defaults.

124

Should this also be setup in StartSymbolizerSubprocess for consistency? Or the other way around: should pid_str_ be populated in GetArgV? But then we would probably introduce an unwanted order dependency between GetEnvP and GetArgV. (These are just questions, not suggestions.)

compiler-rt/test/sanitizer_common/TestCases/Darwin/symbolizer-large-env.cpp

2

I think this is unnecessary

23

I think the most important aspect to test would be to check that the started symbolizer process has all the env vars that we would expect it to have. I thought about how to test this but don't have a good idea, so I am happy to move forward without it.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

54

Seems reasonable. I thought there was some issue with C++ wanting this to be constexpr when I tried this last time but it doesn't seem to be happening now so I'll make this change.

59

The point of this initialization is so that if methods are some how called in the wrong order we get deterministic behaviour. If we just leave garbage in this array and pass this to posix_spawn by accident then we'd likely get unpredictable crashes. Technically this initialization is unnecessary so if you feel very strongly about this then I'll remove it. Do you still want this removed?

124

Should what also be setup in StartSymbolizerSubprocess?

Also please bear in mind the subsequent patch (https://reviews.llvm.org/D77706) where the semantics of CopyEnv change to allocate space for a new environment variable.
Although looking at this.. all of this could actually be moved in StartSymbolizerSubprocess that would actually be way cleaner because we can keep the branching logic for SANITIZER_IOSSIM contained to a single method.

I'll refactor this.

compiler-rt/test/sanitizer_common/TestCases/Darwin/symbolizer-large-env.cpp

2

This was originally here because I was hitting some weird issue where atos was reporting stale line locations. I'll remove it.

[delcypher]

Address first round of feedback.

[yln]

LGTM

[delcypher]

Tweak.

[kubamracek]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

95

Can this be moved into sanitizer_mac.cpp or other non-symbolizer file? Make it an actual useful helper that takes the destination buffer as argument, its size, returns true/false.

[kubamracek]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

115

Can you briefly describe how tricky would it be to not have a hardcoded limit? Could an InternalMmapVector be used here instead?

117

Is it actually correct to not do a "deep copy" and only copy the pointers? I mean, who "owns" these strings? Are they guaranteed to live forever?

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

95

No. The semantics of this function will change in the next patch that tie it to the needs of this class. To avoid discussing this in two places let's discuss the change of semantics in https://reviews.llvm.org/D77706.

115

It's not tricky but I don't think it's a good idea. We should avoid trying to do allocation during symbolication because we could be in an out-of-memory situation. The only way to avoid doing allocations at symbolization time is to do it earlier. This patch allocates at the time the symbolizer class is created (because the statically declared array is an instance member) which is during early process init where we should have plenty of memory (if we don't then we have bigger problems).

117

Darwin's Libc setenv implementation owns these strings. When users call setenv(...) it makes copies of them which it keeps track of. There are technically some circumstances where this is unsafe.

• setenv() is called on an existing environment variable in the environment array. In some cases Libc will call realloc() on the pointer to the existing string. In the right circumstances this could potentially leave a dangling pointer in our copy of the environment array.
• unsetenv() is called on an existing environment variable in the environment array. This will cause the string to be free'd. Again this could leave a dangling pointer in our copy of the environment array.

So no. They are not guaranteed to live for ever.

Possible solutions:

• Do nothing.
• Implement interceptors to prevent environment changes being made while symbolizer process is being launched. Technically this will never be sufficient because there are APIs to directly access the environment array directly.

Deep copying the strings on its own is actually not a good solution because it suffers exactly from the same problem we have already. If the environment gets changed while we're in the middle of copying the strings we could dereference free'd memory.

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

117

I am assuming that posix_spawn makes a deep copy of the passed-in environment when it starts the new process. Dan, do you know if this is indeed the case?

If so, I think we should ignore these issues for the sake of simplicity because the windows for things to go wrong (time between StartSymbolizerSubprocess() and posix_spawn) is small.

[delcypher]

Use new EnvArray type.

[delcypher]

Abandoning this change in favour of:

• https://reviews.llvm.org/D78178
• https://reviews.llvm.org/D78179

compiler-rt/lib/sanitizer_common/sanitizer_mac.h

 //===-- sanitizer_mac.h -----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //

 };
 
 MacosVersion GetMacosVersion();
 
 char **GetEnviron();
 
 void RestrictMemoryToMaxAddress(uptr max_address);
 
+// Get number of entries in environment array (excludes null terminator entry).
+uptr GetNumEnvironEntries(char **envp);
+
 }  // namespace __sanitizer
 
 extern "C" {
 static char __crashreporter_info_buff__[__sanitizer::kErrorMessageBufferSize] =
   {};
 static const char *__crashreporter_info__ __attribute__((__used__)) =
   &__crashreporter_info_buff__[0];
 asm(".desc ___crashreporter_info__, 0x10");

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

 //===-- sanitizer_mac.cpp -------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
if (!env_ptr) {
     CHECK(env_ptr);
   }
   char **environ = *env_ptr;
 #endif
   CHECK(environ);
   return environ;
 }
 
+uptr GetNumEnvironEntries(char **envp) {
+  uptr count = 0;
+  for (; envp[count]; ++count) {
+  }
+  return count;
+}
+
 const char *GetEnv(const char *name) {
   char **env = GetEnviron();
   uptr name_len = internal_strlen(name);
   while (*env != 0) {
     uptr len = internal_strlen(*env);
     if (len > name_len) {
       const char *p = *env;
       if (!internal_memcmp(p, name, name_len) &&

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

 //===-- sanitizer_symbolizer_mac.cpp --------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is shared between various sanitizers' runtime libraries.
 //
 // Implementation of Mac-specific "atos" symbolizer.
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_platform.h"
 #if SANITIZER_MAC
 
-#include "sanitizer_allocator_internal.h"
-#include "sanitizer_mac.h"
-#include "sanitizer_symbolizer_mac.h"
-
 #include <dlfcn.h>
 #include <errno.h>
 #include <stdlib.h>
 #include <sys/wait.h>
 #include <unistd.h>
 #include <util.h>
 
+#include "sanitizer_allocator_internal.h"
+#include "sanitizer_env_array.h"
+#include "sanitizer_mac.h"
+#include "sanitizer_symbolizer_mac.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));
bool DlAddrSymbolizer::SymbolizeData(uptr addr, DataInfo *datainfo) {
   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;
 }
 
 class AtosSymbolizerProcess : public SymbolizerProcess {
  public:
   explicit AtosSymbolizerProcess(const char *path)
       : SymbolizerProcess(path, /*use_posix_spawn*/ true) {
     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());
 
+    // Prepare the environment.
+    custom_env_.reset();
+
+    // Copy the existing environment.
+    // FIXME(dliew): This isn't quite right. There's a race here because we
+    // shallow copy the strings which we don't own which could be free'd between
+    // now and the time we launch the process. Deep copying wouldn't help much
+    // because the exact same race exists. Really we need to intercept `setenv`
+    // and friends and take a lock until we finish launching the process.
+    uptr entries_copied = custom_env_.copyFromRawEnv(
+        GetEnviron(), /*shallow_copy=*/true, /*allow_heap_allocations=*/false,
+        /*overwrite=*/true);
+    uptr num_entries = GetNumEnvironEntries(GetEnviron());
+    if (entries_copied < num_entries) {
+      Report(
+          "WARNING: Truncating symbolizer environment. Needed %llu entries but "
+          "only %llu are available.\n",
+          num_entries, custom_env_.getNumEnvEntries());
+    }
     return SymbolizerProcess::StartSymbolizerSubprocess();
   }
 
   bool ReachedEndOfOutput(const char *buffer, uptr length) const override {
     return (length >= 1 && buffer[length - 1] == '\n');
   }
 
+  char **GetEnvP() override { return custom_env_.getStorage().data(); }
+
   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];
+  EnvArray<512, 1> custom_env_;
 };
 
 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:
   //   myfunction (in library.dylib) (sourcefile.c:17)
   //   myfunction (in library.dylib) + 0x1fe
   //   myfunction (in library.dylib) + 15
   //   0xdeadbeef (in library.dylib) + 0x1fe
   //   0xdeadbeef (in library.dylib) + 15
   //   0xdeadbeef (in library.dylib)
   //   0xdeadbeef
 
   const char *rest = trim;

compiler-rt/test/sanitizer_common/TestCases/Darwin/symbolizer-large-env.cpp

+// RUN: %clangxx %s -g -O0 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+#include <assert.h>
+#include <sanitizer/common_interface_defs.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+extern "C" char ***_NSGetEnviron(void);
+
+size_t get_num_env_entries() {
+  char **envp = *_NSGetEnviron();
+  size_t count = 0;
+  for (; envp[count]; ++count) {
+  }
+  return count;
+}
+
+const size_t kMaxNumEnvPEntries = 511;
+
+int main() {
+  size_t current_entries = get_num_env_entries();
+  fprintf(stderr, "current_entries: %lu\n", current_entries);
+  size_t entries_to_add = (kMaxNumEnvPEntries - current_entries) + 1;
+  fprintf(stderr, "Adding %lu entries\n", entries_to_add);
+
+  // Add a large number of entries. In particular more than the sanitizer
+  // run time allows for.
+  char buffer[256];
+  for (int count = 0; count < entries_to_add; ++count) {
+    int chars_written = snprintf(buffer, sizeof(buffer), "__UNLIKELY_ENV_VAR__%d", count);
+    assert(chars_written);
+    if (setenv(buffer, "some_value", /*overwrite*/ 1)) {
+      fprintf(stderr, "Failed to write \"%s\" to env\n", buffer);
+      return 1;
+    }
+  }
+  current_entries = get_num_env_entries();
+  assert(current_entries == kMaxNumEnvPEntries + 1);
+
+  // Trigger symbolization.
+  // CHECK: WARNING: Truncating symbolizer environment. Needed 512 entries but only 511 are available.
+  // CHECK: #0{{( *0x.* *in *)?}}  __sanitizer_print_stack_trace
+  // CHECK: #1{{( *0x.* *in *)?}} main {{.*}}symbolizer-large-env.cpp:[[@LINE+1]]
+  __sanitizer_print_stack_trace();
+
+  return 0;
+}