[asan] Add flag (-external_symbolizer_path_from_binary) to find llvm-symbolizer relative to the binary's directory.

[akhuang]

Summary

We want way to set a path to llvm-symbolizer that isn't relative
to the current working directory; this change adds a flag that takes a path
relative to the current binary.
This approach came from comments in https://reviews.llvm.org/D93070

[rnk]

Please add a lit test for this. The test could actually copy llvm-symbolizer into a temporary directory, and you could embed the option in __asan_default_options similar to what we want to do in Chromium. I think it's also reasonable to test that if llvm-symbolizer doesn't exist, we get a warning, not an error.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

404

If both options are set, we should prefer the external_symbolizer_path. ClusterFuzz for example sets it explicitly.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp

294

ditto

[akhuang]

-Add lit test, although it uses a hard-coded path
-Make -external_symbolizer_path take precedence

[rnk]

Super nits

@eugenis @vitalybuka, are you OK with this option?

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

404

nit, maybe just !path

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp

294

ditto, !user_path

[eugenis]

LGTM

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

404

+1 for just !path

compiler-rt/test/asan/TestCases/external-symbolizer-path.cpp

5 ↗

(On Diff #316765)

consider changing directory before running to make sure that this path is not a valid relative path from $PWD

17 ↗

(On Diff #316765)

external-symbolizer-path

[vitalybuka]

LGTM

compiler-rt/lib/sanitizer_common/sanitizer_flags.inc

27 ↗

(On Diff #316765)

as-is priority of these flags is not clear. Maybe add check that only one can be !null.

I guess this one can be relative to the current dir.
Maybe something like internal var: external_symbolizer_path=@BINARY_DIR/my/relative/path ?

[akhuang]

compiler-rt/lib/sanitizer_common/sanitizer_flags.inc

27 ↗

(On Diff #316765)

That seems cleaner I guess, something like replacing "@BINARY_DIR" with path to binary?

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_flags.inc

27 ↗

(On Diff #316765)

yes
@eugenis @rnk WDYT?

[rnk]

compiler-rt/lib/sanitizer_common/sanitizer_flags.inc

27 ↗

(On Diff #316765)

Works for me. Should it be supported anywhere in the string, or only as a prefix? I lean towards only supporting it as a prefix.

[akhuang]

compiler-rt/lib/sanitizer_common/sanitizer_flags.inc

27 ↗

(On Diff #316765)

Yeah, I was thinking just as a prefix.

[akhuang]

-switch to using @BINARY_DIR
-update test case (add directory change, fix name)

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_flags.inc

27 ↗

(On Diff #316765)

I have no opinion on prefix vs any position.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

403

static const char kPrefix[] = "@BINARY_DIR";
f (internal_strncmp(path, kPrefix, ARRAY_SIZE(kPrefix) - 1) == 0) {

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

403

Also I don't know if @ it the best char.
we definitely should not use $ or % to avoid complex escaping in shell scripts.
If you know LLVM uses similar placeholders maybe we should consider similar style.

[akhuang]

clean up string code

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

405

internal_strlen needs to scan string
ARRAY_SIZE() - 1 is constant.

compiler-rt/test/asan/TestCases/external-symbolizer-path.cpp

1 ↗

(On Diff #317989)

// REQUIRES: shell ?

11 ↗

(On Diff #317989)

we are changing sanitizer_common so the test probably should be there

There __sanitizer_symbolize_pc which can be used to trigger symbolizer from any sanitizer.

[akhuang]

-change string len to use ARRAY_SIZE
-move test to sanitizer_common

[akhuang]

compiler-rt/test/asan/TestCases/external-symbolizer-path.cpp

11 ↗

(On Diff #317989)

I copied the test to sanitizer_common and it seems to be working as is - is __sanitizer_symbolize_pc needed?

[vitalybuka]

I was going to advise to move substitution into flags.cpp and spotted SubstituteForFlagValue
But it does not work as-is, I didn't yet checked why.

If so, it would be still useful to fix it and finish the test.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

411

we leak this internal_strdup?

compiler-rt/test/sanitizer_common/TestCases/external_symbolizer_path.cpp

2

"REQUIRES: linux" should not be needed with "// REQUIRES: shell"

20

we need msan_default_options, ubsan_default_options etc.
sanitizer_common tests triggered by run "ninja check-sanitizer", I guess everything but asan fail this way.

28

this may not trigge any error with other sanitizers
just add #include <sanitizer/common_interface_defs.h> and call __sanitizer_symbolize_pc, you can find examples in test/sanitizer_common/TestCases/symbolize_pc.cpp

[akhuang]

address comments

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

411

oh, right, guess we don't need that

compiler-rt/test/sanitizer_common/TestCases/external_symbolizer_path.cpp

28

for some reason the tests with __sanitizer_symbolize_pc are hanging when I try to run them - do you know why that might be happening?

[vitalybuka]

compiler-rt/test/sanitizer_common/TestCases/external_symbolizer_path.cpp

28

No. Which sanitizer? It should be possible to extract the command line from test and run under gdb.

alternative is to raise(SIGSEGV); it should trigger stack trace with all sanitizers

[akhuang]

Update test

compiler-rt/test/sanitizer_common/TestCases/external_symbolizer_path.cpp

28

oh, sorry, my code was just broken, everything works now.

[vitalybuka]

In D94563#2517330, @vitalybuka wrote:

I was going to advise to move substitution into flags.cpp and spotted SubstituteForFlagValue
But it does not work as-is, I didn't yet checked why.

have you looked at this? Maybe we need tiny fix to make existing stuff.

[akhuang]

In D94563#2533379, @vitalybuka wrote:

In D94563#2517330, @vitalybuka wrote:

I was going to advise to move substitution into flags.cpp and spotted SubstituteForFlagValue
But it does not work as-is, I didn't yet checked why.

have you looked at this? Maybe we need tiny fix to make existing stuff.

Just looked and it seems like SubstituteForFlagValue only runs for include/include_if_exists flags? I could do a similar thing for ParseString, don't know if we want to do that? Then I guess the @BINARY_DIR substitution would work for all the flags.

[vitalybuka]

In D94563#2534663, @akhuang wrote:

In D94563#2533379, @vitalybuka wrote:

In D94563#2517330, @vitalybuka wrote:

I was going to advise to move substitution into flags.cpp and spotted SubstituteForFlagValue
But it does not work as-is, I didn't yet checked why.

have you looked at this? Maybe we need tiny fix to make existing stuff.

Just looked and it seems like SubstituteForFlagValue only runs for include/include_if_exists flags? I could do a similar thing for ParseString, don't know if we want to do that? Then I guess the @BINARY_DIR substitution would work for all the flags.

I think we should handle symbolizer path next to RegisterIncludeFlags in a similar way.
Having that we have %b we should not add @BINARY_DIR with a same meaning.

[akhuang]

I think we should handle symbolizer path next to RegisterIncludeFlags in a similar way.
Having that we have %b we should not add @BINARY_DIR with a same meaning.

%b is not quite the same, though, it's just the name of the binary without the path. And looks like the include flags are handled differently from things like the symbolizer_path flag. We could use a similar naming scheme, like %d or something?

[vitalybuka]

In D94563#2535063, @akhuang wrote:

I think we should handle symbolizer path next to RegisterIncludeFlags in a similar way.
Having that we have %b we should not add @BINARY_DIR with a same meaning.

%b is not quite the same, though, it's just the name of the binary without the path. And looks like the include flags are handled differently from things like the symbolizer_path flag. We could use a similar naming scheme, like %d or something?

%d lgtm

compiler-rt/test/sanitizer_common/TestCases/external_symbolizer_path.cpp

36

you can avoid __*_default_options using %env_tool_opts=external_symbolizer_path..." in RUN: line

[akhuang]

Use %env_tools_opt in test case, change to %d for consistency

[akhuang]

I ended up not moving things into SubstituteForFlagValue because there are apparently other flags that can contain things like '%p' that shouldn't be substituted.

I changed it from @BINARY_DIR to %d. Thinking about it more, maybe @BINARY_DIR is clearer? And for now it's only being used for external_symbolizer_path. But both seem fine to me.

[vitalybuka]

In D94563#2538075, @akhuang wrote:

I ended up not moving things into SubstituteForFlagValue because there are apparently other flags that can contain things like '%p' that shouldn't be substituted.

I changed it from @BINARY_DIR to %d. Thinking about it more, maybe @BINARY_DIR is clearer? And for now it's only being used for external_symbolizer_path. But both seem fine to me.

SubstituteForFlagValue is not yet applied to all flags, just only to two "include" flags. I assumed you are going to add new RegisterHandler, but then you need to split flag declaration in class and actual handler registration and also there are direct assignment like "cf.external_symbolizer_path = GetEnv("HWASAN_SYMBOLIZER_PATH");" which miss the parser.
So I am fine with substitution in ChooseSymbolizerTools but please still use SubstituteForFlagValue() and add %d there so we have a single point which responsible for that.
And I still prefer a little bit "%d" over "@BIN_DIR" just for consistency.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp

296

I would prefer you still use RegisterHandler, but then you need to split flag declaration in class and actual handler registration.
If still like to make substitution here please still use SubstituteForFlagValue() and add %d there so we
have single point which responsible for that.

[akhuang]

Move substitution into SubstituteForFlagValue.

[akhuang]

Sounds good, I added the substitution to SubstituteForFlagValue, and am still calling that from the symbolizer code.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_flags.cpp

68–78

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

404–408

Substituting only if the first is % looks inconsistent, please do it always.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp

294

same

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp

1052–1069

[akhuang]

address comments

[akhuang]

fix some typos

[vitalybuka]

LGTM we few comments

compiler-rt/lib/sanitizer_common/sanitizer_flags.cpp

68–78

not done?

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp

294

user_path?

[akhuang]

address comments

[akhuang]

thanks for helping / reviewing!

[RKSimon]

@akhuang This buildbot is still red after these changes - please can you take a look?

http://lab.llvm.org:8011/#/builders/112/builds/3626
...
http://lab.llvm.org:8011/#/builders/112/builds/3809

[akhuang]

I'm not entirely sure why the buildbots are failing - I think I'll just disable the test everywhere for now.

[vitalybuka]

This bot uses -DBUILD_SHARED_LIBS=ON So it fails to load libLLVMOption.so which was not copied with llvm-symbolizer. I guess we don't want to copy so files.
Should be fixed with

REQUIRES: static-libs

[RKSimon]

Thanks - the bot is green again.

compiler-rt/lib/sanitizer_common/sanitizer_common.h

 const char *StripPathPrefix(const char *filepath,
                             const char *strip_file_prefix);
 // Strip the directories from the module name.
 const char *StripModuleName(const char *module);
 
 // OS
 uptr ReadBinaryName(/*out*/char *buf, uptr buf_len);
 uptr ReadBinaryNameCached(/*out*/char *buf, uptr buf_len);
+uptr ReadBinaryDir(/*out*/ char *buf, uptr buf_len);
 uptr ReadLongProcessName(/*out*/ char *buf, uptr buf_len);
 const char *GetProcessName();
 void UpdateProcessName();
 void CacheBinaryName();
 void DisableCoreDumperIfNecessary();
 void DumpProcessMap();
 const char *GetEnv(const char *name);
 bool SetEnv(const char *name, const char *value);

compiler-rt/lib/sanitizer_common/sanitizer_common.cpp

uptr ReadBinaryNameCached(/*out*/char *buf, uptr buf_len) {
   name_len = (name_len < buf_len - 1) ? name_len : buf_len - 1;
   if (buf_len == 0)
     return 0;
   internal_memcpy(buf, binary_name_cache_str, name_len);
   buf[name_len] = '\0';
   return name_len;
 }
 
+uptr ReadBinaryDir(/*out*/ char *buf, uptr buf_len) {
+  ReadBinaryNameCached(buf, buf_len);
+  const char *exec_name_pos = StripModuleName(buf);
+  uptr name_len = exec_name_pos - buf;
+  buf[name_len] = '\0';
+  return name_len;
+}
+
 #if !SANITIZER_GO
 void PrintCmdline() {
   char **argv = GetArgv();
   if (!argv) return;
   Printf("\nCommand: ");
   for (uptr i = 0; argv[i]; ++i)
     Printf("%s ", argv[i]);
   Printf("\n\n");

compiler-rt/lib/sanitizer_common/sanitizer_flags.cpp

 
 void CommonFlags::CopyFrom(const CommonFlags &other) {
   internal_memcpy(this, &other, sizeof(*this));
 }
 
 // Copy the string from "s" to "out", making the following substitutions:
 // %b = binary basename
 // %p = pid
+// %d = binary directory
 void SubstituteForFlagValue(const char *s, char *out, uptr out_size) {
   char *out_end = out + out_size;
   while (*s && out < out_end - 1) {
     if (s[0] != '%') {
       *out++ = *s++;
       continue;
     }
     switch (s[1]) {
switch (s[1]) {
           *--buf_pos = (pid % 10) + '0';
           pid /= 10;
         } while (pid);
         while (buf_pos < buf + 32 && out < out_end - 1)
           *out++ = *buf_pos++;
         s += 2; // skip "%p"
         break;
       }
+      case 'd': {
+        uptr len = ReadBinaryDir(out, out_end - out);
+        out += len;
+        s += 2;  // skip "%d"
+        break;
+      }
       default:
         *out++ = *s++;
         break;
     }
   }
   CHECK(out < out_end - 1);
   *out = '\0';
 }
 
 class FlagHandlerInclude final : public FlagHandlerBase {
   FlagParser *parser_;
   bool ignore_missing_;
   const char *original_path_;

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

 #endif  // SANITIZER_SUPPORTS_WEAK_HOOKS
 
 const char *Symbolizer::PlatformDemangle(const char *name) {
   return DemangleSwiftAndCXX(name);
 }
 
 static SymbolizerTool *ChooseExternalSymbolizer(LowLevelAllocator *allocator) {
   const char *path = common_flags()->external_symbolizer_path;
+
+  if (path && internal_strchr(path, '%')) {
+    char *new_path = (char *)InternalAlloc(kMaxPathLength);
+    SubstituteForFlagValue(path, new_path, kMaxPathLength);
+    path = new_path;
+  }
+
   const char *binary_name = path ? StripModuleName(path) : "";
   if (path && path[0] == '\0') {
     VReport(2, "External symbolizer is explicitly disabled.\n");
     return nullptr;
   } else if (!internal_strcmp(binary_name, "llvm-symbolizer")) {
     VReport(2, "Using llvm-symbolizer at user-specified path: %s\n", path);
     return new(*allocator) LLVMSymbolizer(path, allocator);
   } else if (!internal_strcmp(binary_name, "atos")) {
 #if SANITIZER_MAC
     VReport(2, "Using atos at user-specified path: %s\n", path);

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp

 
 static void ChooseSymbolizerTools(IntrusiveList<SymbolizerTool> *list,
                                   LowLevelAllocator *allocator) {
   if (!common_flags()->symbolize) {
     VReport(2, "Symbolizer is disabled.\n");
     return;
   }
 
-  // Add llvm-symbolizer in case the binary has dwarf.
+  // Add llvm-symbolizer.
   const char *user_path = common_flags()->external_symbolizer_path;
+
+  if (user_path && internal_strchr(user_path, '%')) {
+    char *new_path = (char *)InternalAlloc(kMaxPathLength);
+    SubstituteForFlagValue(user_path, new_path, kMaxPathLength);
+    user_path = new_path;
+  }
+
   const char *path =
       user_path ? user_path : FindPathToBinary("llvm-symbolizer.exe");
   if (path) {
     VReport(2, "Using llvm-symbolizer at %spath: %s\n",
             user_path ? "user-specified " : "", path);
     list->push_back(new(*allocator) LLVMSymbolizer(path, allocator));
   } else {
     if (user_path && user_path[0] == '\0') {

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp

case EXCEPTION_INT_DIVIDE_BY_ZERO:
       return "int-divide-by-zero";
     case EXCEPTION_INT_OVERFLOW:
       return "int-overflow";
   }
   return "unknown exception";
 }
 
 uptr ReadBinaryName(/*out*/char *buf, uptr buf_len) {
-  // FIXME: Actually implement this function.
-  CHECK_GT(buf_len, 0);
-  buf[0] = 0;
+  if (buf_len == 0)
     return 0;
+
+  // Get the UTF-16 path and convert to UTF-8.
+  wchar_t binname_utf16[kMaxPathLength];
+  int binname_utf16_len =
+      GetModuleFileNameW(NULL, binname_utf16, ARRAY_SIZE(binname_utf16));
+  if (binname_utf16_len == 0) {
+    buf[0] = '\0';
+    return 0;
+  }
+  int binary_name_len = ::WideCharToMultiByte(
+      CP_UTF8, 0, binname_utf16, binname_utf16_len, buf, buf_len, NULL, NULL);
+  if ((unsigned)binary_name_len == buf_len)
+    --binary_name_len;
+  buf[binary_name_len] = '\0';
+  return binary_name_len;
 }
 
 uptr ReadLongProcessName(/*out*/char *buf, uptr buf_len) {
   return ReadBinaryName(buf, buf_len);
 }
 
 void CheckVMASize() {
   // Do nothing.
 }

compiler-rt/lib/sanitizer_common/tests/sanitizer_common_test.cpp

TEST(SanitizerCommon, ReservedAddressRangeUnmap) {
   address_range.Unmap(new_start, PageSize);
 
   #endif
 
   // Unmapping in the middle of the ReservedAddressRange should fail.
   EXPECT_DEATH(address_range.Unmap(base_addr + (PageSize * 2), PageSize), ".*");
 }
 
-// Windows has no working ReadBinaryName.
-#if !SANITIZER_WINDOWS
 TEST(SanitizerCommon, ReadBinaryNameCached) {
   char buf[256];
   EXPECT_NE((uptr)0, ReadBinaryNameCached(buf, sizeof(buf)));
 }
-#endif
 
 }  // namespace __sanitizer

compiler-rt/test/sanitizer_common/TestCases/external_symbolizer_path.cpp

+// REQUIRES: shell
+// RUN: rm -rf %t.bin
+// RUN: mkdir %t.bin
+// RUN: cp $(which llvm-symbolizer) %t.bin
+// RUN: rm -rf %t.dir
+// RUN: mkdir %t.dir
+// RUN: %clangxx -O0 %s -o %t && cd %t.dir
+// RUN: %env_tool_opts=external_symbolizer_path=%d/external_symbolizer_path.cpp.tmp.bin/llvm-symbolizer \
+// RUN:   %run %t 2>&1 | FileCheck %s --check-prefix=FOUND
+// RUN: rm -rf %t.bin/llvm-symbolizer
+// RUN: cd ..
+// RUN: %clangxx -O0 %s -o %t && cd %t.dir
+// RUN: %env_tool_opts=external_symbolizer_path=%d/external_symbolizer_path.cpp.tmp.bin/llvm-symbolizer \
+// RUN:   %run %t 2>&1 | FileCheck %s --check-prefix=NOT-FOUND
+
+#include <sanitizer/common_interface_defs.h>
+#include <stdio.h>
+
+static void Symbolize() {
+  char buffer[100];
+  __sanitizer_symbolize_pc(__builtin_return_address(0), "%p %F %L", buffer,
+                           sizeof(buffer));
+  printf("%s\n", buffer);
+}
+
+int main() {
+  // FOUND: {{0x.* in main}}
+  // NOT-FOUND: WARNING: invalid path to external symbolizer!
+  Symbolize();
+}