[ubsan-minimal] Report the address of an error

[ikudrin]

Summary

This implements a FIXME in the runtime library and adds printing the address at the end of the message as "by 0x123abc". The buffer for the message is allocated on the stack in a handler, so the stack memory consumption is slightly increased. No additional external dependencies are added.

[ikudrin]

Unfortunately, I have no idea how to check the new output. Will be grateful for any suggestions.

[ikudrin]

Ping.

[vitalybuka]

Can you add/update few tests?

compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp

23–26

looks unrelated, can you land it separately, no review is needed

54

why do we need noinline here?

59

please don't skip them and match %p style
sanitizers usually don't skip 0s

65

you can avoid updating mask with:
unsigned int nibble = (caller >> shift) & 0xf;

97

Can we have "@" -> "at 0x"?

[vitalybuka]

In D131914#3724102, @ikudrin wrote:

Unfortunately, I have no idea how to check the new output. Will be grateful for any suggestions.

Sorry, didn't notice on the first pass.
my favorite way for find test on unknown code is to brake code around and run all tests: check-all

something like this and "ninja check-ubsan-minimal"

diff --git a/compiler-rt/test/ubsan_minimal/TestCases/alignment-assumption.c b/compiler-rt/test/ubsan_minimal/TestCases/alignment-assumption.c
index 08696c81e573..763261dda1d5 100644
--- a/compiler-rt/test/ubsan_minimal/TestCases/alignment-assumption.c
+++ b/compiler-rt/test/ubsan_minimal/TestCases/alignment-assumption.c
@@ -8,7 +8,7 @@ int main(int argc, char *argv[]) {
 
   void *t = __builtin_assume_aligned(ptr + 1, 0x8000);
   (void)t;
-  // CHECK: ubsan: alignment-assumption
+  // CHECK: ubsan: alignment-assumption at 0x{{[0-9af]}}
   // CHECK-NOT: alignment-assumption
 
   free(ptr);

maybe update all these few test cases in the dir

[vitalybuka]

compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp

97

Can we have "@" -> "at 0x"?

Looks like we mostly use "at" for load/store location
maybe "@" -> "by 0x", then it would be clear that this is the code

[ikudrin]

Thanks for the review!

• An NFC part is committed as https://reviews.llvm.org/rG1959a55591da
• Print leading zeroes,
• "@..." -> "by 0x...",
• Update tests, except recover-dedup-limit.cpp, because that change would only add some noise and not any value to it.

compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp

54

report_this_error() has the attribute, so I decided to follow that here. It seems to correspond to the intention to make the library as small as possible.

97

Changed to "by 0x".

[MaskRay]

compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp

56

s/unsigned int/unsigned/ everywhere

56

You may change the loop to do shift -= 4 first, then the declaration can omit - 4.

[MaskRay]

compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp

96

constexpr unsigned kAddrBuf = ...

static is redundant for const. But when constexpr works, prefer constexpr instead.

compiler-rt/test/ubsan_minimal/TestCases/alignment-assumption.c

11

Note: [[#%x,]] can replace {{[[:xdigit:]]+}} but here we match $ too which requires {{$}}, I think xdigit is fine, too.

[ikudrin]

Thanks!

compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp

 #endif
 
 static const int kMaxCallerPcs = 20;
 static __sanitizer::atomic_uintptr_t caller_pcs[kMaxCallerPcs];
 // Number of elements in caller_pcs. A special value of kMaxCallerPcs + 1 means
 // that "too many errors" has already been reported.
 static __sanitizer::atomic_uint32_t caller_pcs_sz;
 
 __attribute__((noinline)) static bool report_this_error(uintptr_t caller) {
   if (caller == 0)
     return false;
   while (true) {
     unsigned sz = __sanitizer::atomic_load_relaxed(&caller_pcs_sz);
     if (sz > kMaxCallerPcs) return false;  // early exit
     // when sz==kMaxCallerPcs print "too many errors", but only when cmpxchg
     // succeeds in order to not print it multiple times.
     if (sz > 0 && sz < kMaxCallerPcs) {
       uintptr_t p;
       for (unsigned i = 0; i < sz; ++i) {
         p = __sanitizer::atomic_load_relaxed(&caller_pcs[i]);
if (sz == kMaxCallerPcs) {
       message("ubsan: too many errors\n");
       return false;
     }
     __sanitizer::atomic_store_relaxed(&caller_pcs[sz], caller);
     return true;
   }
 }
 
+__attribute__((noinline)) static void decorate_msg(char *buf,
+                                                   uintptr_t caller) {
+  unsigned int shift = sizeof(uintptr_t) * 8 - 4;
+  // print the address by nibbles
+  for (;;) {
+    unsigned int nibble = (caller >> shift) & 0xf;
+    *(buf++) = nibble < 10 ? nibble + '0' : nibble - 10 + 'a';
+    if (!shift)
+      break;
+    shift -= 4;
+  }
+  // finish the message
+  buf[0] = '\n';
+  buf[1] = '\0';
+}
+
 #if defined(__ANDROID__)
 extern "C" __attribute__((weak)) void android_set_abort_message(const char *);
 static void abort_with_message(const char *msg) {
   if (&android_set_abort_message) android_set_abort_message(msg);
   abort();
 }
 #else
 static void abort_with_message(const char *) { abort(); }
void NORETURN CheckFailed(const char *file, int, const char *cond, u64, u64) {
   message(cond);
   abort();
 }
 } // namespace __sanitizer
 #endif
 
 #define INTERFACE extern "C" __attribute__((visibility("default")))
 
-// FIXME: add caller pc to the error message (possibly as "ubsan: error-type
-// @1234ABCD").
+// How many chars we need to reserve to print an address.
+static const unsigned int kAddrBuf = SANITIZER_WORDSIZE / 4;
+#define MSG_TMPL(msg) "ubsan: " msg " by 0x"
+#define MSG_TMPL_END(buf, msg) (buf + sizeof(MSG_TMPL(msg)) - 1)
+// Reserve an additional byte for '\n'.
+#define MSG_BUF_LEN(msg) (sizeof(MSG_TMPL(msg)) + kAddrBuf + 1)
+
 #define HANDLER_RECOVER(name, msg)                               \
   INTERFACE void __ubsan_handle_##name##_minimal() {             \
-    if (!report_this_error(GET_CALLER_PC())) return; \
-    message("ubsan: " msg "\n");                                 \
+    uintptr_t caller = GET_CALLER_PC();                  \
+    if (!report_this_error(caller)) return;                      \
+    char msg_buf[MSG_BUF_LEN(msg)] = MSG_TMPL(msg);              \
+    decorate_msg(MSG_TMPL_END(msg_buf, msg), caller);            \
+    message(msg_buf);                                            \
   }
 
 #define HANDLER_NORECOVER(name, msg)                             \
   INTERFACE void __ubsan_handle_##name##_minimal_abort() {       \
-    message("ubsan: " msg "\n");                                 \
-    abort_with_message("ubsan: " msg);                           \
+    char msg_buf[MSG_BUF_LEN(msg)] = MSG_TMPL(msg);              \
+    decorate_msg(MSG_TMPL_END(msg_buf, msg), GET_CALLER_PC());   \
+    message(msg_buf);                                            \
+    abort_with_message(msg_buf);                                 \
   }
 
 #define HANDLER(name, msg)                                       \
   HANDLER_RECOVER(name, msg)                                     \
   HANDLER_NORECOVER(name, msg)
 
 HANDLER(type_mismatch, "type-mismatch")
 HANDLER(alignment_assumption, "alignment-assumption")

compiler-rt/test/ubsan_minimal/TestCases/alignment-assumption.c

 // RUN: %clang -fsanitize=alignment %s -o %t && %run %t 2>&1 | FileCheck %s --check-prefixes=CHECK
 
 #include <stdlib.h>
 
 int main(int argc, char *argv[]) {
 
   char *ptr = (char *)malloc(2);
 
   void *t = __builtin_assume_aligned(ptr + 1, 0x8000);
   (void)t;
-  // CHECK: ubsan: alignment-assumption
+  // CHECK: ubsan: alignment-assumption by 0x{{[[:xdigit:]]+$}}
   // CHECK-NOT: alignment-assumption
 
   free(ptr);
 
   return 0;
 }

compiler-rt/test/ubsan_minimal/TestCases/implicit-integer-sign-change.c

 // RUN: %clang -fsanitize=implicit-integer-sign-change %s -o %t && %run %t 2>&1 | FileCheck %s --check-prefixes=CHECK
 
 #include <stdint.h>
 
 int main() {
 // CHECK-NOT: implicit-conversion
 
   // Explicitly casting hides it,
   int32_t n0 = (int32_t)(~((uint32_t)0));
 
   // Positive tests.
   int32_t t0 = (~((uint32_t)0));
-// CHECK: implicit-conversion
+// CHECK: implicit-conversion by 0x{{[[:xdigit:]]+$}}
 // CHECK-NOT: implicit-conversion
 
   return 0;
 }

compiler-rt/test/ubsan_minimal/TestCases/implicit-signed-integer-truncation-or-sign-change.c

 // RUN: %clang -fsanitize=implicit-signed-integer-truncation,implicit-integer-sign-change %s -o %t && %run %t 2>&1 | FileCheck %s --check-prefixes=CHECK
 
 #include <stdint.h>
 
 int main() {
 // CHECK-NOT: implicit-conversion
 
   // Explicitly casting hides it,
   int8_t n0 = (int8_t)((uint32_t)-1);
 
   // Positive tests.
   int8_t t0 = (uint32_t)-1;
-// CHECK: implicit-conversion
+// CHECK: implicit-conversion by 0x{{[[:xdigit:]]+$}}
 // CHECK-NOT: implicit-conversion
 
   return 0;
 }

compiler-rt/test/ubsan_minimal/TestCases/implicit-signed-integer-truncation.c

// CHECK-NOT: implicit-conversion
   _Bool b0 = (~((uint32_t)(0)));
   _Bool b1 = 255;
 
   // Explicit and-ing of bits will silence it.
   uint8_t nc0 = ((int32_t)(-1)) & 255;
 
   // Positive tests.
   uint8_t t0 = (int32_t)(-1);
-// CHECK: implicit-conversion
+// CHECK: implicit-conversion by 0x{{[[:xdigit:]]+$}}
 // CHECK-NOT: implicit-conversion
 
   return 0;
 }

compiler-rt/test/ubsan_minimal/TestCases/implicit-unsigned-integer-truncation.c

// CHECK-NOT: implicit-conversion
   _Bool b0 = (~((uint32_t)(0)));
   _Bool b1 = 255;
 
   // Explicit and-ing of bits will silence it.
   uint8_t nc0 = ((~((uint32_t)(0))) & 255);
 
   // Positive tests.
   uint8_t t0 = (~((uint32_t)(0)));
-// CHECK: implicit-conversion
+// CHECK: implicit-conversion by 0x{{[[:xdigit:]]+$}}
 // CHECK-NOT: implicit-conversion
 
   return 0;
 }

compiler-rt/test/ubsan_minimal/TestCases/nullptr-and-nonzero-offset.c

 // RUN: %clang   -fsanitize=pointer-overflow %s -o %t && %run %t 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-C --implicit-check-not="pointer-overflow"
 // RUN: %clangxx -x c++ -fsanitize=pointer-overflow %s -o %t && %run %t 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-CPP --implicit-check-not="pointer-overflow"
 
 #include <stdlib.h>
 
 int main(int argc, char *argv[]) {
   char *base, *result;
 
   base = (char *)0;
   result = base + 0;
-  // CHECK-C: pointer-overflow
+  // CHECK-C: pointer-overflow by 0x{{[[:xdigit:]]+$}}
   // CHECK-CPP-NOT: pointer-overflow
 
   base = (char *)0;
   result = base + 1;
-  // CHECK: pointer-overflow
+  // CHECK: pointer-overflow by 0x{{[[:xdigit:]]+$}}
 
   base = (char *)1;
   result = base - 1;
-  // CHECK: pointer-overflow
+  // CHECK: pointer-overflow by 0x{{[[:xdigit:]]+$}}
 
   return 0;
 }

compiler-rt/test/ubsan_minimal/TestCases/recover-dedup.cpp

 // RUN: %clangxx -w -fsanitize=signed-integer-overflow,nullability-return,returns-nonnull-attribute -fsanitize-recover=all %s -o %t && %run %t 2>&1 | FileCheck %s
 
 #include <stdint.h>
 #include <stdio.h>
 
 int *_Nonnull h() {
-  // CHECK: nullability-return
+  // CHECK: nullability-return by 0x{{[[:xdigit:]]+$}}
   return NULL;
 }
 
 __attribute__((returns_nonnull))
 int *i() {
-  // CHECK: nonnull-return
+  // CHECK: nonnull-return by 0x{{[[:xdigit:]]+$}}
   return NULL;
 }
 
 __attribute__((noinline))
 int f(int x, int y) {
-  // CHECK: mul-overflow
+  // CHECK: mul-overflow by 0x{{[[:xdigit:]]+$}}
   return x * y;
 }
 
 __attribute__((noinline))
 int g(int x, int y) {
-  // CHECK: mul-overflow
+  // CHECK: mul-overflow by 0x{{[[:xdigit:]]+$}}
   return x * (y + 1);
 }
 
 int main() {
   h();
   i();
   int x = 2;
   for (int i = 0; i < 10; ++i)
     x = f(x, x);
   x = 2;
   for (int i = 0; i < 10; ++i)
     x = g(x, x);
   // CHECK-NOT: mul-overflow
 }

compiler-rt/test/ubsan_minimal/TestCases/uadd-overflow.cpp

 // RUN: %clangxx -fsanitize=unsigned-integer-overflow %s -o %t && %run %t 2>&1 | FileCheck %s
 // RUN: %clangxx -fsanitize=unsigned-integer-overflow -fno-sanitize-recover=all %s -o %t && not --crash %run %t 2>&1 | FileCheck %s
 
 #include <stdint.h>
 
 int main() {
   uint32_t k = 0x87654321;
   k += 0xedcba987;
-  // CHECK: add-overflow
+  // CHECK: add-overflow by 0x{{[[:xdigit:]]+$}}
 }