[llvm-commits] [PATCH] [ASan] Use dylib interposition to hook memory allocation in the dynamic runtime.

[Alexander Potapenko]

================
Comment at: asan_intercepted_functions.h:232
@@ +231,3 @@
+typedef void malloc_zone_t;
+typedef size_t vm_size_t;
+DECLARE_FUNCTION_AND_WRAPPER(malloc_zone_t *, malloc_create_zone,
----------------
Alexey Samsonov wrote:
> Hm, can you use uptr instead of size_t?
I think it's generally wrong to rewrite the function prototypes using our typenames.
Anyway, Kostya wants this file to be gone soon after my CL, so let us just leave it as is for now.

================
Comment at: asan_malloc_mac.cc:87
@@ +86,3 @@
+    GET_STACK_TRACE_MALLOC;
+    new_name = (char*)asan_malloc(buflen, &stack);
+    if (new_name) {
----------------
Alexey Samsonov wrote:
> Why do you need to allocate/free memory for zone name via ASan allocator (with fetching stack trace for malloc etc.)
> Can you use InternalScopedBuffer instead?
Good catch, thanks! Done.

================
Comment at: asan_malloc_mac.cc:109
@@ +108,3 @@
+  void *res = asan_malloc(size, &stack);
+  ///Report("malloc(%d)=%p\n", size, res);
+  return res;
----------------
Alexey Samsonov wrote:
> Remove this (or hide under verbosity?)
Done

================
Comment at: asan_malloc_mac.cc:149
@@ +148,3 @@
+  if (result) {
+    *memptr = result;
+    return 0;
----------------
Alexey Samsonov wrote:
> Can memptr be zero? Or it's fine to segfault in this case?
Added a check.

================
Comment at: asan_malloc_mac.cc:204
@@ +203,3 @@
+  GET_STACK_TRACE_FREE;
+  // FIXME: need to retire this flag.
+  if (!flags()->mac_ignore_invalid_free) {
----------------
Alexey Samsonov wrote:
> Why? (just curious)
We needed it because some allocations used to be intercepted incorrectly on Mac, and sometimes invalid free were reported because of the zone mismatch. In such cases we wanted to keep going.
Now everything will be allocated from a single zone, thus no mismatches

================
Comment at: lit_tests/heap-overflow.cc:32
@@ -31,5 +31,3 @@
 
-  // CHECK-Darwin: {{    #0 0x.* in .*mz_malloc.*}}
-  // CHECK-Darwin: {{    #1 0x.* in malloc_zone_malloc.*}}
-  // CHECK-Darwin: {{    #2 0x.* in malloc.*}}
-  // CHECK-Darwin: {{    #3 0x.* in _?main .*heap-overflow.cc:21}}
+  // CHECK-Darwin: {{    #0 0x.* in wrap_malloc.*}}
+  // CHECK-Darwin: {{    #1 0x.* in _?main .*heap-overflow.cc:21}}
----------------
Alexey Samsonov wrote:
> _?wrap_malloc here and below?
Seems to work as is, but good idea anyway.

================
Comment at: tests/CMakeLists.txt:84
@@ -83,3 +83,3 @@
                  LINK_FLAGS "-framework Foundation")
-    target_link_libraries(${testname} clang_rt.asan_osx)
+    target_link_libraries(${testname} clang_rt.asan_osx_dynamic)
   elseif (ANDROID)
----------------
Alexey Samsonov wrote:
> Will static runtime work at all after this change? If no, plan to remove rules for building it in /lib/asan/CMakeLists.txt
It won't. There'll be a cleanup CL afterwards.


http://llvm-reviews.chandlerc.com/D216