[AddressSanitizer] [Windows] Fix HeapReAlloc and _recalloc bugs in asan_malloc_win.cc

[mcgov]

Summary

HeapReAlloc should allow for 0 sized reallocations without freeing the memory block provided by the user.

_recalloc previously did not zero new memory after reallocation.

[mcgov]

adding tests for correct behaviors

[mcgov]

Ready for review

[vitalybuka]

Let first land NFC reformatting part of this patch?
And rebase this patch on top of the NFC one.

[mcgov]

In D61268#1482723, @vitalybuka wrote:

Let first land NFC reformatting part of this patch?
And rebase this patch on top of the NFC one.

That's a good idea, I'll do that now.

[rnk]

Watch out for compiler-rt/lib/sanitizer_common/scripts/check_lint.sh, sometimes it disagrees with what clang-format does. It runs as part of check-sanitizer on Linux but not Windows. I have run into this accidentally numerous times. If you don't want to worry about that, consider using git-clang-format to avoid reformatting lines not affected by your patch.

Functionality seems good, thanks!

[mcgov]

Update diff to only apply clang-format to new code

[mcgov]

fix call to get_usable_size so that it is not called unless HEAP_ZERO_MEMORY is passed in (the only case when it is needed).

[mcgov]

Ready for review (clang-format only applied to new code)

[vitalybuka]

LGTM

compiler-rt/lib/asan/asan_malloc_win.cc

147

not formated?

[rnk]

compiler-rt/lib/asan/asan_malloc_win.cc

23

I don't think this is needed, you can use u8 * instead which is more common across the rest of the ASan runtime.

147

Make sure this is just 2 space indentation.

[mcgov]

fix BYTE typdef and incorrect format

[rnk]

lgtm, thanks!

[mcgov]

closed with revision 359498

compiler-rt/lib/asan/asan_malloc_win.cc

 #endif
 
 extern "C" {
+
+ALLOCATION_FUNCTION_ATTRIBUTE
+size_t _msize(void *ptr) {
+  GET_CURRENT_PC_BP_SP;
+  (void)sp;
+  return asan_malloc_usable_size(ptr, pc, bp);
+}
+
+ALLOCATION_FUNCTION_ATTRIBUTE
+size_t _msize_base(void *ptr) {
+  return _msize(ptr);
+}
+
 ALLOCATION_FUNCTION_ATTRIBUTE
 void free(void *ptr) {
   GET_STACK_TRACE_FREE;

   const size_t size = n * elem_size;
   if (elem_size != 0 && size / elem_size != n)
     return 0;
-  return realloc(p, size);
+
+  size_t old_size = _msize(p);
+  void *new_alloc = malloc(size);
+  if (new_alloc) {
+    REAL(memcpy)(new_alloc, p, Min(size, old_size));
+    if (old_size < size)
+      REAL(memset)(((u8 *)new_alloc) + old_size, 0, size - old_size);
+    free(p);
+  }
+  return new_alloc;
 }
 
 ALLOCATION_FUNCTION_ATTRIBUTE

   return _recalloc(p, n, elem_size);
 }
 
-ALLOCATION_FUNCTION_ATTRIBUTE
-size_t _msize(void *ptr) {
-  GET_CURRENT_PC_BP_SP;
-  (void)sp;
-  return asan_malloc_usable_size(ptr, pc, bp);
-}
-
-ALLOCATION_FUNCTION_ATTRIBUTE
-size_t _msize_base(void *ptr) {
-  return _msize(ptr);
-}
-
 ALLOCATION_FUNCTION_ATTRIBUTE
 void *_expand(void *memblock, size_t size) {
   // _expand is used in realloc-like functions to resize the buffer if possible.

 INTERCEPTOR_WINAPI(LPVOID, HeapReAlloc, HANDLE hHeap, DWORD dwFlags,
                    LPVOID lpMem, SIZE_T dwBytes) {
   GET_STACK_TRACE_MALLOC;
+  GET_CURRENT_PC_BP_SP;
   // Realloc should never reallocate in place.
   if (dwFlags & HEAP_REALLOC_IN_PLACE_ONLY)
     return nullptr;
   CHECK(dwFlags == 0 && "unsupported heap flags");
-  return asan_realloc(lpMem, dwBytes, &stack);
+  // HeapReAlloc and HeapAlloc both happily accept 0 sized allocations.
+  // passing a 0 size into asan_realloc will free the allocation.
+  // To avoid this and keep behavior consistent, fudge the size if 0.
+  // (asan_malloc already does this)
+  if (dwBytes == 0)
+    dwBytes = 1;
+  size_t old_size;
+  if (dwFlags & HEAP_ZERO_MEMORY)
+    old_size = asan_malloc_usable_size(lpMem, pc, bp);
+  void *ptr = asan_realloc(lpMem, dwBytes, &stack);
+  if (ptr == nullptr)
+    return nullptr;
+
+  if (dwFlags & HEAP_ZERO_MEMORY) {
+    size_t new_size = asan_malloc_usable_size(ptr, pc, bp);
+    if (old_size < new_size)
+      REAL(memset)(((u8 *)ptr) + old_size, 0, new_size - old_size);
+  }
+  return ptr;
 }
 
 INTERCEPTOR_WINAPI(SIZE_T, HeapSize, HANDLE hHeap, DWORD dwFlags,

compiler-rt/test/asan/TestCases/Windows/heaprealloc_zero_size.cc

+// RUN: %clang_cl_asan /Od -o %t %s
+// RUN: %run %t 2>&1 | FileCheck %s
+// RUN: %clang_cl /Od -o %t %s
+// RUN: %run %t 2>&1 | FileCheck %s
+#include <cassert>
+#include <stdio.h>
+#include<windows.h>
+
+int main() {
+  HANDLE heap = HeapCreate(0, 0, 0);
+  void *ptr = HeapAlloc(heap, 0, 4);
+  assert(ptr);
+  void *ptr2 = HeapReAlloc(heap, 0, ptr, 0);
+  assert(ptr2);
+  HeapFree(heap, 0, ptr2);
+  fprintf(stderr, "passed!\n");
+}
+
+// CHECK-NOT: double-free
+// CHECK-NOT: AddressSanitizer
+// CHECK: passed!
 No newline at end of file

compiler-rt/test/asan/TestCases/Windows/recalloc_sanity.cc

+// RUN: %clang_cl_asan %s -o %t.exe
+// RUN: %run %t.exe 2>&1 | FileCheck %s
+// RUN: %clang_cl %s -o %t.exe
+// RUN: %run %t.exe 2>&1 | FileCheck %s
+
+#include <cassert>
+#include <stdio.h>
+#include <windows.h>
+
+int main() {
+  void *p = calloc(1, 100);
+  assert(p);
+  void *np = _recalloc(p, 2, 100);
+  assert(np);
+  for (int i = 0; i < 2 * 100; i++) {
+    assert(((BYTE *)np)[i] == 0);
+  }
+  void *nnp = _recalloc(np, 1, 100);
+  assert(nnp);
+  for (int i = 0; i < 100; i++) {
+    assert(((BYTE *)nnp)[i] == 0);
+    ((BYTE *)nnp)[i] = 0x0d;
+  }
+  void *nnnp = _recalloc(nnp, 2, 100);
+  assert(nnnp);
+  for (int i = 0; i < 100; i++) {
+    assert(((BYTE *)nnnp)[i] == 0x0d);
+  }
+  for (int i = 100; i < 200; i++) {
+    assert(((BYTE *)nnnp)[i] == 0);
+  }
+  fprintf(stderr, "passed\n");
+  return 0;
+}
+
+// CHECK-NOT: Assertion
+// CHECK: passed
 No newline at end of file