[Sanitizers] Remove BuildId from sanitizers stacktrace on Darwin

[usama54321]

Summary

On Darwin, we do not want to show the BuildId appended at the end of stack
frames in Sanitizers. According to our understanding, the BuildId string is not being used anywhere on Darwin.

rdar://108324403

[usama54321]

There are two more calls to MaybeBuildIdToBuffer in the switch statement. I did not guard them with !SANITIZER_APPLE because they do not affect the codepath on Darwin. If we want, I can also guard them with !SANITIZER_APPLE, or just guard the function implementation itself rather than the call sites.

[yln]

Nit: can we push the conditional inside MaybeBuildIdToBuffer()? Since it's already aptly named "Maybe..."

Please also show the before/after outputs for an example ASan/TSan/UBSan bug.

[usama54321]

Feedback fixes

[usama54321]

There is no change in UBSan.

Old ASan

WRITE of size 4 at 0x000100502ee0 thread T0
    #0 0x100493ec8 in main+0x84 (a.out:arm64+0x100003ec8) (BuildId: 6b02f9ebe9513beeb808945f611316dc32000000200000000100000000000e00)
    #1 0x18937a054  (<unknown module>)

0x000100502ee0 is located 1100 bytes after 100-byte region [0x000100502a30,0x000100502a94)
allocated by thread T0 here:
    #0 0x100f22b08 in wrap_malloc+0x94 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x52b08) (BuildId: 487e1a780e8f3396a9f9da766c5e185b32000000200000000100000000000b00)
    #1 0x100493e68 in main+0x24 (a.out:arm64+0x100003e68) (BuildId: 6b02f9ebe9513beeb808945f611316dc32000000200000000100000000000e00)
    #2 0x18937a054  (<unknown module>)

SUMMARY: AddressSanitizer: heap-buffer-overflow (a.out:arm64+0x100003ec8) (BuildId: 6b02f9ebe9513beeb808945f611316dc32000000200000000100000000000e00) in main+0x84

New Asan

WRITE of size 4 at 0x60b0000004f0 thread T0
    #0 0x1008bfec8 in main+0x84 (a.out:arm64+0x100003ec8)
    #1 0x18937a054  (<unknown module>)
    #2 0x3e79fffffffffffc  (<unknown module>)

0x60b0000004f0 is located 1100 bytes after 100-byte region [0x60b000000040,0x60b0000000a4)
allocated by thread T0 here:
    #0 0x101260800 in wrap_malloc sanitizer_malloc_mac.inc:137
    #1 0x1008bfe68 in main+0x24 (a.out:arm64+0x100003e68)
    #2 0x18937a054  (<unknown module>)
    #3 0x3e79fffffffffffc  (<unknown module>)

SUMMARY: AddressSanitizer: heap-buffer-overflow (a.out:arm64+0x100003ec8) in main+0x84

Old TSan:

 Write of size 4 at 0x00010420c000 by thread T1:
    #0 Thread1 <null>:79480708 (a.out:arm64+0x100003dc8) (BuildId: 5e79074a7b753da999cf8789fa452d1432000000200000000100000000000e00)

  Previous write of size 4 at 0x00010420c000 by main thread:
    #0 main <null>:79480708 (a.out:arm64+0x100003e3c) (BuildId: 5e79074a7b753da999cf8789fa452d1432000000200000000100000000000e00)

  Location is global 'Global' at 0x00010420c000 (a.out+0x100008000)

  Thread T1 (tid=6744635, running) created by main thread at:
    #0 pthread_create <null>:79480708 (libclang_rt.tsan_osx_dynamic.dylib:arm64e+0x2ffec) (BuildId: e5ecae67005633f0b93152d2bf60e86e32000000200000000100000000000b00)
    #1 main <null>:79480708 (a.out:arm64+0x100003e34) (BuildId: 5e79074a7b753da999cf8789fa452d1432000000200000000100000000000e00)

SUMMARY: ThreadSanitizer: data race (a.out:arm64+0x100003dc8) (BuildId: 5e79074a7b753da999cf8789fa452d1432000000200000000100000000000e00) in Thread1+0x34

New TSan:

  Write of size 4 at 0x0001048e4000 by thread T1:
    #0 Thread1 <null>:131073952 (a.out:arm64+0x100003dc0)

  Previous write of size 4 at 0x0001048e4000 by main thread:
    #0 main <null>:131073600 (a.out:arm64+0x100003e3c)

  Location is global 'Global' at 0x0001048e4000 (a.out+0x100008000)

  Thread T1 (tid=6745255, running) created by main thread at:
    #0 pthread_create tsan_interceptors_posix.cpp:1048 (libclang_rt.tsan_osx_dynamic.dylib:arm64+0x2d20c)
    #1 main <null>:131073952 (a.out:arm64+0x100003e24)

SUMMARY: ThreadSanitizer: data race (a.out:arm64+0x100003dc0) in Thread1+0x3c

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp

129

how about runtime sanitizer common flag which is FALSE by default for apple

[fmayer]

Can you please elaborate in the change description why "On Darwin, we do not want to show the BuildId appended at the end of stack
frames in Sanitizers."?

[thetruestblue]

My only nit is to add more description to commit message (as fmayer suggested) about why we are removing this and perhaps a link to the review that this was introduced. https://reviews.llvm.org/D114294

[usama54321]

In D150298#4354170, @fmayer wrote:

Can you please elaborate in the change description why "On Darwin, we do not want to show the BuildId appended at the end of stack
frames in Sanitizers."?

We do not want to show it since according to our understanding it is currently not being used for anything on Darwin. Can you kindly elaborate a bit about the motivation of introducing this in https://reviews.llvm.org/D114294? Thanks

[fmayer]

In D150298#4354220, @usama54321 wrote:

In D150298#4354170, @fmayer wrote:

Can you please elaborate in the change description why "On Darwin, we do not want to show the BuildId appended at the end of stack
frames in Sanitizers."?

We do not want to show it since according to our understanding it is currently not being used for anything on Darwin. Can you kindly elaborate a bit about the motivation of introducing this in https://reviews.llvm.org/D114294? Thanks

It makes it a lot easier to symbolize crashes from stripped binary, because you can find the correct version of the unstripped binary from the build id.

[fmayer]

In D150298#4354226, @fmayer wrote:

In D150298#4354220, @usama54321 wrote:

In D150298#4354170, @fmayer wrote:

Can you please elaborate in the change description why "On Darwin, we do not want to show the BuildId appended at the end of stack
frames in Sanitizers."?

We do not want to show it since according to our understanding it is currently not being used for anything on Darwin. Can you kindly elaborate a bit about the motivation of introducing this in https://reviews.llvm.org/D114294? Thanks

It makes it a lot easier to symbolize crashes from stripped binary, because you can find the correct version of the unstripped binary from the build id.

With that in mind I would suggest _not removing this_ unless you have a reason why this is causing problems. This can be useful for people.

[fmayer]

Apart from my other comment, if you want to go ahead I would suggest implementing this in a different way and only removing the MaybeBuildIdToBuffer from the implementation of 'L' in sanitizer_stacktrace_printer.cpp, then a) people can still manually get the build id if their format string uses %b, and b) the MaybeBuildIdToBuffer function doesn't unnecessarily not work on OS X, which is confusing.

[yln]

In D150298#4354287, @fmayer wrote:

Apart from my other comment, if you want to go ahead I would suggest implementing this in a different way and only removing the MaybeBuildIdToBuffer from the implementation of 'L' in sanitizer_stacktrace_printer.cpp, then a) people can still manually get the build id if their format string uses %b, and b) the MaybeBuildIdToBuffer function doesn't unnecessarily not work on OS X, which is confusing.

I think these are good suggestions. How about combining this with Vitaly's suggestion for using a flag to only change this on Apple platforms (and to enable everyone to still get the current output)?

Just thought of an even more flexible approach. Make the frame format string a configuration option, so we can specify platform-dependent defaults, allow users to get current behavior (on Apple platforms), and give them the ability to fully customize if they desire.

static const char kDefaultFormat[] = "    #%n %p %F %L";

@vitalybuka @fmayer
What do you think?

[fmayer]

In D150298#4354471, @yln wrote:

In D150298#4354287, @fmayer wrote:

Apart from my other comment, if you want to go ahead I would suggest implementing this in a different way and only removing the MaybeBuildIdToBuffer from the implementation of 'L' in sanitizer_stacktrace_printer.cpp, then a) people can still manually get the build id if their format string uses %b, and b) the MaybeBuildIdToBuffer function doesn't unnecessarily not work on OS X, which is confusing.

I think these are good suggestions. How about combining this with Vitaly's suggestion for using a flag to only change this on Apple platforms (and to enable everyone to still get the current output)?

Just thought of an even more flexible approach. Make the frame format string a configuration option, so we can specify platform-dependent defaults, allow users to get current behavior (on Apple platforms), and give them the ability to fully customize if they desire.

static const char kDefaultFormat[] = "    #%n %p %F %L";

@vitalybuka @fmayer
What do you think?

The custom strings would involve splitting the %b from the %L and putting "%L %b" as the custom string, which would lead to an extraneous space if %b is empty. We could add the space as the first character of the %b, but that's kind of ugly.

[yln]

Oh, I and just realized, we already have this option to customize the stack frame format (sanitizer common flag stack_trace_format)!
https://github.com/llvm/llvm-project/blob/ca50840b5bc0679e824850ca4fd322504d6713a0/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc#L194

So then it's just about specifying a platform-dependent default?

[yln]

Maybe something like this?

#if APPLE
static const char kDefaultFormat[] = "    #%n %p %F %xxxx<figure this out>";
#else
static const char kDefaultFormat[] = "    #%n %p %F %L";
#endif

[usama54321]

In D150298#4354481, @fmayer wrote:

In D150298#4354471, @yln wrote:

In D150298#4354287, @fmayer wrote:

Apart from my other comment, if you want to go ahead I would suggest implementing this in a different way and only removing the MaybeBuildIdToBuffer from the implementation of 'L' in sanitizer_stacktrace_printer.cpp, then a) people can still manually get the build id if their format string uses %b, and b) the MaybeBuildIdToBuffer function doesn't unnecessarily not work on OS X, which is confusing.

I think these are good suggestions. How about combining this with Vitaly's suggestion for using a flag to only change this on Apple platforms (and to enable everyone to still get the current output)?

Just thought of an even more flexible approach. Make the frame format string a configuration option, so we can specify platform-dependent defaults, allow users to get current behavior (on Apple platforms), and give them the ability to fully customize if they desire.

static const char kDefaultFormat[] = "    #%n %p %F %L";

@vitalybuka @fmayer
What do you think?

The custom strings would involve splitting the %b from the %L and putting "%L %b" as the custom string, which would lead to an extraneous space if %b is empty. We could add the space as the first character of the %b, but that's kind of ugly.

The current codeflow makes this refactoring difficult since the call to MaybeBuildIdToBuffer appears in an else if branch in both %L and %M specifiers. There is already a %b specifier present for MaybeBuildIdToBuffer but I can not just use that. I could write another specifier like %B which only calls MaybeBuildIdToBuffer with the conditions (!info->file && info->module) || (!(address & kExternalPCBit) && info->module) but that looks really ugly. Do you have any other suggestions?

[MaskRay]

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

140

This can use a regular if statement.

[fmayer]

In D150298#4361758, @usama54321 wrote:

In D150298#4354481, @fmayer wrote:

In D150298#4354471, @yln wrote:

In D150298#4354287, @fmayer wrote:

Apart from my other comment, if you want to go ahead I would suggest implementing this in a different way and only removing the MaybeBuildIdToBuffer from the implementation of 'L' in sanitizer_stacktrace_printer.cpp, then a) people can still manually get the build id if their format string uses %b, and b) the MaybeBuildIdToBuffer function doesn't unnecessarily not work on OS X, which is confusing.

I think these are good suggestions. How about combining this with Vitaly's suggestion for using a flag to only change this on Apple platforms (and to enable everyone to still get the current output)?

Just thought of an even more flexible approach. Make the frame format string a configuration option, so we can specify platform-dependent defaults, allow users to get current behavior (on Apple platforms), and give them the ability to fully customize if they desire.

static const char kDefaultFormat[] = "    #%n %p %F %L";

@vitalybuka @fmayer
What do you think?

The custom strings would involve splitting the %b from the %L and putting "%L %b" as the custom string, which would lead to an extraneous space if %b is empty. We could add the space as the first character of the %b, but that's kind of ugly.

The current codeflow makes this refactoring difficult since the call to MaybeBuildIdToBuffer appears in an else if branch in both %L and %M specifiers. There is already a %b specifier present for MaybeBuildIdToBuffer but I can not just use that. I could write another specifier like %B which only calls MaybeBuildIdToBuffer with the conditions (!info->file && info->module) || (!(address & kExternalPCBit) && info->module) but that looks really ugly. Do you have any other suggestions?

Could you please explain why these build ids do harm? Otherwise it would really be preferable to just have consistency between platforms.

At any rate, if you have to: however you do the change, please do not make`'%b` a no-op, and don't make MaybeBuildIdToBuffer a no-op. If the easiest way is to put the two calls in %M and %L into an ifdef, then I'd begrudgingly accept that.

[usama54321]

There are a couple of problems with Build IDs on Darwin. Firstly, Build IDs/UUIDs are 16 bytes on Darwin. I put up a patch at https://reviews.llvm.org/D152309 to fix this. Secondly, a stack trace is not the right place to show Build Ids as multiple frames from the same library lead to repetition. This purpose is better served by the print_module_map sanitizer flag which dumps the Build Ids of all the libraries without repetition. I have followed your recommendations and made the %b specifier and the MaybeBuildIdToBuffer method functional again. @fmayer

[fmayer]

I still don't like the inconsistency between platforms, and I don't think a few bytes of repeated build id matter, but fine.

[thakis]

Could this have broken check-hwasan? http://45.33.8.238/linux/109046/step_10.txt

[fmayer]

In D150298#4401580, @thakis wrote:

Could this have broken check-hwasan? http://45.33.8.238/linux/109046/step_10.txt

Looks like it.

[thakis]

Please take a look and revert for now if it takes a while to fix.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp

221

sanitizer_platform.h does

#else
#  define SANITIZER_APPLE 0

so it's defined on non-apple too. I think that's why this breaks tests.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace_printer.cpp

if (!internal_strcmp(function, "__libc_thr_sigsetmask"))
     return "pthread_sigmask";
 #endif
 
   return function;
 }
 
 static void MaybeBuildIdToBuffer(const AddressInfo &info, bool PrefixSpace,
                                  InternalScopedString *buffer) {
   if (info.uuid_size) {
     if (PrefixSpace)
       buffer->append(" ");
     buffer->append("(BuildId: ");
     for (uptr i = 0; i < info.uuid_size; ++i) {
       buffer->append("%02x", info.uuid[i]);
     }
     buffer->append(")");
   }
case 'L':
       // Source location, or module location.
       if (info->file) {
         RenderSourceLocation(buffer, info->file, info->line, info->column,
                              vs_style, strip_path_prefix);
       } else if (info->module) {
         RenderModuleLocation(buffer, info->module, info->module_offset,
                              info->module_arch, strip_path_prefix);
 
+#if !SANITIZER_APPLE
         MaybeBuildIdToBuffer(*info, /*PrefixSpace=*/true, buffer);
+#endif
       } else {
         buffer->append("(<unknown module>)");
       }
       break;
     case 'M':
       // Module basename and offset, or PC.
       if (address & kExternalPCBit) {
         // There PCs are not meaningful.
       } else if (info->module) {
         // Always strip the module name for %M.
         RenderModuleLocation(buffer, StripModuleName(info->module),
                              info->module_offset, info->module_arch, "");
+#if !SANITIZER_APPLE
         MaybeBuildIdToBuffer(*info, /*PrefixSpace=*/true, buffer);
+#endif
       } else {
         buffer->append("(%p)", (void *)address);
       }
       break;
     default:
       Report("Unsupported specifier in stack frame format: %c (%p)!\n", *p,
              (void *)p);
       Die();

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

TEST(SanitizerStacktracePrinter, RenderFrame) {
 
   info.uuid_size = 2;
   info.uuid[0] = 0x55;
   info.uuid[1] = 0x66;
 
   RenderFrame(&str, "%M", frame_no, info.address, &info, false);
   EXPECT_NE(nullptr, internal_strstr(str.data(), "(module+0x"));
   EXPECT_NE(nullptr, internal_strstr(str.data(), "200"));
+#if SANITIZER_APPLE
+  EXPECT_EQ(nullptr, internal_strstr(str.data(), "BuildId: 5566"));
+#else
   EXPECT_NE(nullptr, internal_strstr(str.data(), "BuildId: 5566"));
+#endif
   str.clear();
 
   RenderFrame(&str, "%L", frame_no, info.address, &info, false);
+#if SANITIZER_APPLE
+  EXPECT_STREQ("(/path/to/module+0x200)", str.data());
+#else
   EXPECT_STREQ("(/path/to/module+0x200) (BuildId: 5566)", str.data());
+#endif
   str.clear();
 
   RenderFrame(&str, "%b", frame_no, info.address, &info, false);
   EXPECT_STREQ("(BuildId: 5566)", str.data());
   str.clear();
 
   info.function = internal_strdup("my_function");
   RenderFrame(&str, "%F", frame_no, info.address, &info, false);