[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

53

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

60

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.

107

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

1

I think this is unnecessary

22

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

53

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.

60

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?

107

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

1

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

78

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

100

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?

129

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

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

78

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.

100

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.

129

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).

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp

100

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_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_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;
 }
 
+static const uptr kMaxNumEnvPEntries = 512;
+
 class AtosSymbolizerProcess : public SymbolizerProcess {
  public:
   explicit AtosSymbolizerProcess(const char *path)
       : SymbolizerProcess(path, /*use_posix_spawn*/ true) {
     pid_str_[0] = '\0';
+    internal_memset(custom_env_, 0, sizeof(custom_env_));
   }
 
  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();
   }
 
   bool ReachedEndOfOutput(const char *buffer, uptr length) const override {
     return (length >= 1 && buffer[length - 1] == '\n');
   }
 
+  void CopyEnv(char **current_env) {
+    // Count number of elements (excluding nullptr entry).
+    uptr count = 0;
+    for (; current_env[count] != nullptr; ++count) {
+    }
+
+    // +1 for nullptr terminator.
+    uptr custom_env_required_entries = count + 1;
+    uptr entries_to_copy = count;
+
+    if (custom_env_required_entries > kMaxNumEnvPEntries) {
+      // Will have to truncate the copy of the environment.
+      Report(
+          "WARNING: Truncating symbolizer environment. Needed %llu entries but "
+          "only %llu are available.\n",
+          custom_env_required_entries, kMaxNumEnvPEntries);
+      entries_to_copy = kMaxNumEnvPEntries - 1;
+      custom_env_required_entries = kMaxNumEnvPEntries;
+    }
+
+    // Copy over existing entries
+    CHECK_LE(entries_to_copy, kMaxNumEnvPEntries);
+    internal_memcpy(custom_env_, current_env, entries_to_copy * sizeof(uptr));
+    // Null terminate the array.
+    custom_env_[custom_env_required_entries - 1] = nullptr;
+  }
+
+  char **GetEnvP() override {
+    char **current_env = SymbolizerProcess::GetEnvP();
+    CopyEnv(current_env);
+    return custom_env_;
+  }
+
   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];
+  // Use statically allocated array to avoid allocation at symbolization time.
+  char *custom_env_[kMaxNumEnvPEntries];
 };
 
 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);
 

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

+// RUN: rm -rf %t %t.dSYM
+// 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 = 512;
+
+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);
+  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);
+
+  // Trigger symbolization.
+  // CHECK: WARNING: Truncating symbolizer environment. Needed 513 entries but only 512 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;
+}