Subzero: Improved quality of ASan error messages

runtime/szrt_asan.c

Index: runtime/szrt_asan.c
diff --git a/runtime/szrt_asan.c b/runtime/szrt_asan.c
index b1cf9d1dd524d305a7818d5e64544d0b668ced85..b0d985a414da498b982a974a614e6a7b43bf718a 100644
--- a/runtime/szrt_asan.c
+++ b/runtime/szrt_asan.c
@@ -44,7 +44,12 @@
 #define SHADOW2MEM(p)                                                          \
   ((uintptr_t)((char *)(p)-shadow_offset) << SHADOW_SCALE_LOG2)
 
-#define POISON_VAL (-1)
+#define STACK_POISON_VAL ((char)-1)
+#define HEAP_POISON_VAL ((char)-2)
+#define GLOBAL_POISON_VAL ((char)-3)
+
+#define ACCESS_LOAD (0)
+#define ACCESS_STORE (1)
 
 #if DEBUG
 #define DUMP(args...)                                                          \
@@ -57,8 +62,8 @@
 
 static char *shadow_offset = NULL;
 
-static void __asan_error(char *, int);
-static void __asan_check(char *, int);
+static bool __asan_check(char *, int);
+static void __asan_error(char *, int, int);
 static void __asan_get_redzones(char *, char **, char **);
 
 void __asan_init(int, void **, int *);
@@ -68,27 +73,53 @@ void *__asan_malloc(size_t);
 void *__asan_calloc(size_t, size_t);
 void *__asan_realloc(char *, size_t);
 void __asan_free(char *);
-void __asan_poison(char *, int);
+void __asan_poison(char *, int, char);
 void __asan_unpoison(char *, int);
 
-static void __asan_error(char *ptr, int size) {
-  fprintf(stderr, "Illegal access of %d bytes at %p\n", size, ptr);
+static void __asan_error(char *ptr, int size, int access) {
+  char *shadow_addr = MEM2SHADOW(ptr);
+  char shadow_val = *shadow_addr;
+  if (shadow_val > 0)
+    shadow_val = *(shadow_addr + 1);
+  char *accesstype = NULL;
+  switch (access) {

[Karl, 2016/08/04 14:42:24]

Why not use a global array with these values, and just index?

Example:

static char* kAccessNames[] = {
  "load from",
  "store to"
};

and then replace the switch with:

accesstype = kAccessNames[access];

[Jim Stichnoth, 2016/08/04 14:55:09]

That sounds appealing, but note that it will probably need to x-macros magic to
be done properly.

[tlively, 2016/08/04 17:58:22]

Done.

+  case ACCESS_LOAD:
+    accesstype = "load from";
+    break;
+  case ACCESS_STORE:
+    accesstype = "store to";
+    break;
+  }
+  assert(accesstype != NULL);
+  char *memtype = NULL;
+  switch (shadow_val) {

[Karl, 2016/08/04 14:42:24]

Similar here, replace with something like:

memtype = kMemTypeNames[MEMTYPE_INDEX(shadow_val)];

and then up after GLOBAL_POISON_VAL

define:

#define MEMTYPE_INDEX(x) (-1 - (x))

[tlively, 2016/08/04 17:58:22]

Done.

+  case STACK_POISON_VAL:
+    memtype = "stack";
+    break;
+  case HEAP_POISON_VAL:
+    memtype = "heap";
+    break;
+  case GLOBAL_POISON_VAL:
+    memtype = "global";
+    break;
+  }
+  assert(memtype != NULL);
+  fprintf(stderr, "Illegal %d byte %s %s object at %p\n", size, accesstype,
+          memtype, ptr);
   abort();
 }
 
 // check only the first byte of each word unless strict
-static void __asan_check(char *ptr, int size) {
+static bool __asan_check(char *ptr, int size) {
   assert(size == 1 || size == 2 || size == 4 || size == 8);
   char *shadow_addr = (char *)MEM2SHADOW(ptr);
+  char shadow_val = *shadow_addr;
   DUMP("check %d bytes at %p: %p + %d (%d)\n", size, ptr, shadow_addr,
-       (uintptr_t)ptr % SHADOW_SCALE, *shadow_addr);
+       (uintptr_t)ptr % SHADOW_SCALE, shadow_val);
   if (size == SHADOW_SCALE) {
-    if (*shadow_addr != 0)
-      __asan_error(ptr, size);
-    return;
+    return shadow_val == 0;
   }
-  if (*shadow_addr != 0 && (char)SHADOW_OFFSET(ptr) + size > *shadow_addr)
-    __asan_error(ptr, size);
+  return shadow_val == 0 || (char)SHADOW_OFFSET(ptr) + size <= shadow_val;
 }
 
 static void __asan_get_redzones(char *ptr, char **left, char **right) {
@@ -103,15 +134,16 @@ static void __asan_get_redzones(char *ptr, char **left, char **right) {
 void __asan_check_load(char *ptr, int size) {
   // aligned single word accesses may be widened single byte accesses, but for
   // all else use strict check
-  if (size == WORD_SIZE && (uintptr_t)ptr % WORD_SIZE == 0)
-    size = 1;
-  __asan_check(ptr, size);
+  int check_size =
+      (size == WORD_SIZE && (uintptr_t)ptr % WORD_SIZE == 0) ? 1 : size;
+  if (!__asan_check(ptr, check_size))
+    __asan_error(ptr, size, ACCESS_LOAD);
 }
 
 void __asan_check_store(char *ptr, int size) {
   // stores may never be partially out of bounds so use strict check
-  bool strict = true;
-  __asan_check(ptr, size);
+  if (!__asan_check(ptr, size))
+    __asan_error(ptr, size, ACCESS_STORE);
 }
 
 void __asan_init(int n_rzs, void **rzs, int *rz_sizes) {
@@ -138,7 +170,7 @@ void __asan_init(int n_rzs, void **rzs, int *rz_sizes) {
   DUMP("poisioning %d global redzones\n", n_rzs);
   for (int i = 0; i < n_rzs; i++) {
     DUMP("(%d) poisoning redzone of size %d at %p\n", i, rz_sizes[i], rzs[i]);
-    __asan_poison(rzs[i], rz_sizes[i]);
+    __asan_poison(rzs[i], rz_sizes[i], GLOBAL_POISON_VAL);
   }
 }
 
@@ -157,8 +189,8 @@ void *__asan_malloc(size_t size) {
   }
   void *ret = rz_left + rz_left_size;
   void *rz_right = ret + size;
-  __asan_poison(rz_left, rz_left_size);
-  __asan_poison(rz_right, rz_right_size);
+  __asan_poison(rz_left, rz_left_size, HEAP_POISON_VAL);
+  __asan_poison(rz_right, rz_right_size, HEAP_POISON_VAL);
   // record size and location data so we can find it again
   *(void **)rz_left = rz_right;
   *(size_t *)rz_right = rz_right_size;
@@ -204,7 +236,7 @@ void __asan_free(char *ptr) {
   free(rz_left);
 }
 
-void __asan_poison(char *ptr, int size) {
+void __asan_poison(char *ptr, int size, char poison_val) {
   char *end = ptr + size;
   assert(IS_SHADOW_ALIGNED(end));
   // redzones should be no greater than RZ_SIZE + RZ_SIZE-1 for alignment
@@ -212,12 +244,11 @@ void __asan_poison(char *ptr, int size) {
   DUMP("poison %d bytes at %p: %p - %p\n", size, ptr, MEM2SHADOW(ptr),
        MEM2SHADOW(end));
   size_t offset = SHADOW_OFFSET(ptr);
-  *(char *)MEM2SHADOW(ptr) = (offset == 0) ? POISON_VAL : offset;
+  *(char *)MEM2SHADOW(ptr) = (offset == 0) ? poison_val : offset;
   ptr += SHADOW_OFFSET(size);
   assert(IS_SHADOW_ALIGNED(ptr));
-  for (; ptr != end; ptr += SHADOW_SCALE) {
-    *(char *)MEM2SHADOW(ptr) = POISON_VAL;
-  }
+  int len = (end - ptr) >> SHADOW_SCALE_LOG2;
+  memset(MEM2SHADOW(ptr), poison_val, len);
 }
 
 void __asan_unpoison(char *ptr, int size) {
@@ -229,7 +260,5 @@ void __asan_unpoison(char *ptr, int size) {
   *(char *)MEM2SHADOW(ptr) = 0;
   ptr += SHADOW_OFFSET(size);
   assert(IS_SHADOW_ALIGNED(ptr));
-  for (; ptr != end; ptr += SHADOW_SCALE) {
-    *(char *)MEM2SHADOW(ptr) = 0;
-  }
+  memset(MEM2SHADOW(ptr), 0, (end - ptr) >> SHADOW_SCALE_LOG2);
 }