[sanitizer] Handle malloc_destroy_zone() on Darwin

[kubamracek]

Summary

We currently have a interceptor for malloc_create_zone, which returns a new zone that redirects all the zone requests to our sanitizer zone. However, calling malloc_destroy_zone on that zone will cause libmalloc to print out some warning messages, because the zone is not registered in the list of zones. This patch handles this and adds a testcase for that.

Secondly, in certain OS versions, it was possible that libmalloc replaced the sanitizer zone from being the default zone (i.e. being in malloc_zones[0]). This patch also introduces a failsafe that makes sure we always stay the default zone. No testcase for this, because this doesn't reproduce under normal circumstances.

[filcab]

Please split this in two commits: The malloc_zone_destroy part and the bugfix for malloc_zones[0]. They seem orthogonal.

I'd really like a test for the malloc_zones[0] problem, though. Is it not possible at all? Does it happen only on certain versions (as in: maybe it was a bug somewhere else)?
Otherwise LGTM, but you might want to check if anyone that's using ASan on the mac on more complex systems (Google Chrome?) have something to say.

lib/sanitizer_common/sanitizer_malloc_mac.inc

78

Why not this (seems more explicit to me. Not a big deal, though)?

malloc_zones[i] = malloc_zones[0];
malloc_zones[0] = &sanitizer_zone;

[kubamracek]

Updating patch.

Please split this in two commits: The malloc_zone_destroy part and the bugfix for malloc_zones[0]. They seem orthogonal.

Ok, I'll commit this separately.

I'd really like a test for the malloc_zones[0] problem, though. Is it not possible at all? Does it happen only on certain versions (as in: maybe it was a bug somewhere else)?
you might want to check if anyone that's using ASan on the mac on more complex systems (Google Chrome?) have something to say.

I'm afraid I can't come up with a test for this. This was a "bug" (or rather an incompatible change) in some versions of libmalloc on macOS, where in certain cases this mprotect() interceptor was needed; but I believe the problem was fixed before ending in any public release. We have been using this patch internally for a long time now, so it should be safe.

[zaks.anna]

lib/sanitizer_common/sanitizer_malloc_mac.inc

68

I'd add a comment that explains the intent here (why are we doing this), ex:
// Ensure that the sanitizer_zone is registered as malloc_zones[0].

74

You could strengthen the condition above to malloc_num_zones > 1 && and start iteration from '1'. If there is a single zone:

• if it is sanitizer_zone, we are good to go.
• if it is not a sanitizer_zone, there is nothing to swap it with.

[kubamracek]

Updating patch, addressing review comments.

[zaks.anna]

lib/sanitizer_common/sanitizer_malloc_mac.inc

75

Iteration should start with 1 since the condition will always be false for i==0. The current version is correct, but was a bit more complicated to reason about since the intention is never to malloc_zones[0] with malloc_zones[0].

[zaks.anna]

LGTM!

[kubamracek]

Landed in r289375 and r289376.

lib/sanitizer_common/sanitizer_malloc_mac.inc

INTERCEPTOR(malloc_zone_t *, malloc_create_zone,
   internal_memcpy(new_zone, &sanitizer_zone, sizeof(sanitizer_zone));
   new_zone->zone_name = NULL;  // The name will be changed anyway.
   if (GetMacosVersion() >= MACOS_VERSION_LION) {
     // Prevent the client app from overwriting the zone contents.
     // Library functions that need to modify the zone will set PROT_WRITE on it.
     // This matches the behavior of malloc_create_zone() on OSX 10.7 and higher.
     mprotect(new_zone, allocated_size, PROT_READ);
   }
+  // We're explicitly *NOT* registering the zone.
   return new_zone;
 }
 
+INTERCEPTOR(void, malloc_destroy_zone, malloc_zone_t *zone) {
+  COMMON_MALLOC_ENTER();
+  // We don't need to do anything here.  We're not registering new zones, so we
+  // don't to unregister.  Just un-mprotect and free() the zone.
+  if (GetMacosVersion() >= MACOS_VERSION_LION) {
+    uptr page_size = GetPageSizeCached();
+    uptr allocated_size = RoundUpTo(sizeof(sanitizer_zone), page_size);
+    mprotect(zone, allocated_size, PROT_READ | PROT_WRITE);
+  }
+  COMMON_MALLOC_FREE(zone);
+}
+
+extern unsigned malloc_num_zones;
+extern malloc_zone_t **malloc_zones;
+
+// If libmalloc tries to set up a different zone as malloc_zones[0], it will
+// call mprotect(malloc_zones, ..., PROT_READ).  This interceptor will catch
+// that and make sure we are still the first (default) zone.
+INTERCEPTOR(int, mprotect, void *addr, size_t len, int prot) {
+  if (addr == malloc_zones && prot == PROT_READ) {
+    if (malloc_num_zones > 0 && malloc_zones[0] != &sanitizer_zone) {
+      for (unsigned i = 0; i < malloc_num_zones; i++) {
+        if (malloc_zones[i] == &sanitizer_zone) {
+          malloc_zone_t *tmp = malloc_zones[0];
+          malloc_zones[0] = malloc_zones[i];
+          malloc_zones[i] = tmp;
+          break;
+        }
+      }
+    }
+  }
+  return REAL(mprotect)(addr, len, prot);
+}
+
 INTERCEPTOR(malloc_zone_t *, malloc_default_zone, void) {
   COMMON_MALLOC_ENTER();
   return &sanitizer_zone;
 }
 
 INTERCEPTOR(malloc_zone_t *, malloc_default_purgeable_zone, void) {
   // FIXME: ASan should support purgeable allocations.
   // https://github.com/google/sanitizers/issues/139

test/asan/TestCases/Darwin/malloc_destroy_zone.cc

+// RUN: %clangxx_asan %s -o %t && %run %t 2>&1 | FileCheck %s
+
+#include <malloc/malloc.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+int main() {
+  fprintf(stderr, "start\n");
+  malloc_zone_t *zone = malloc_create_zone(0, 0);
+  fprintf(stderr, "zone = %p\n", zone);
+  malloc_set_zone_name(zone, "myzone");
+  fprintf(stderr, "name changed\n");
+  malloc_destroy_zone(zone);
+  fprintf(stderr, "done\n");
+  return 0;
+}
+
+// CHECK: start
+// CHECK-NEXT: zone = 0x{{.*}}
+// CHECK-NEXT: name changed
+// CHECK-NEXT: done