[lsan] realloc: don't deallocate if requested size is too large

[MaskRay]

Summary

This is the behavior required by the standards.

And add a test for realloc(p, 0) (implementation-defined, we deallocate p).

[MaskRay]

compiler-rt/test/lsan/TestCases/realloc_oom.c

4

I'll change it to REQUIRES: x86_64-target-arch.

aarch64 and powerpc64le cannot detect basic malloc leaks..

[MaskRay]

Make tests work on aarch64 & ppc64le.

Add malloc_zero.c - since it is unrelated to this patch, I'll push it separately.

[vitalybuka]

compiler-rt/test/lsan/TestCases/realloc_oom.c

20

With -1 it's not clear if we hit (new_size > max_malloc_size) condition or we just can't map requested size.
I reccomend to use value above max_allocation_size_mb

also you can use e.g. LSAN_OPTIONS=max_allocation_size_mb=100 it to limit value to compatible with i386

[vitalybuka]

compiler-rt/test/lsan/TestCases/malloc_zero.c

4 ↗

(On Diff #333760)

why this one is disabled for i386?

[MaskRay]

Improve tests

compiler-rt/test/lsan/TestCases/malloc_zero.c

4 ↗

(On Diff #333760)

The test fails on i386. I think its fprintf implementation leaves a pointer on the stack. Printing a mangled address like p+1000 can fix it but I'll use use_stacks=0.

p = 0; is to make powerpc64le/aarch64 happy.

[vitalybuka]

compiler-rt/test/lsan/TestCases/malloc_zero.c

19 ↗

(On Diff #333930)

this should help

[MaskRay]

Thanks for the for (int i = 0; i < 10; i++) tip.

Stick with use_stacks=0 for now and remove unneeded __attribute__((noinline)).
Pre-commit malloc(0) and realloc(p, 0) tests in 59e422c90bf4796fc73237e838d8954b4e2099b1
because they are not directly related to this patch (but was missing coverage).

[DavidSpickett]

@MaskRay realloc_too_big.c is failing on our AArch32 bot: http://lab.llvm.org:8011/#/builders/59/builds/1596

******************** TEST 'LeakSanitizer-Standalone-armhf :: TestCases/realloc_too_big.c' FAILED ********************
Script:
--
: 'RUN: at line 1';      /home/tcwg-buildslave/worker/clang-cmake-armv7-full/stage2/./bin/clang  -O0  -mcpu=cortex-a15 -mfpu=vfpv3 -marm   -gline-tables-only -fsanitize=leak -I/home/tcwg-buildslave/worker/clang-cmake-armv7-full/llvm/compiler-rt/test/lsan/../ /home/tcwg-buildslave/worker/clang-cmake-armv7-full/llvm/compiler-rt/test/lsan/TestCases/realloc_too_big.c -o /home/tcwg-buildslave/worker/clang-cmake-armv7-full/stage2/projects/compiler-rt/test/lsan/ARMHFLsanConfig/TestCases/Output/realloc_too_big.c.tmp
: 'RUN: at line 2';   env LSAN_OPTIONS=:detect_leaks=1:allocator_may_return_null=1:max_allocation_size_mb=1:use_stacks=0 not  /home/tcwg-buildslave/worker/clang-cmake-armv7-full/stage2/projects/compiler-rt/test/lsan/ARMHFLsanConfig/TestCases/Output/realloc_too_big.c.tmp 2>&1 | FileCheck /home/tcwg-buildslave/worker/clang-cmake-armv7-full/llvm/compiler-rt/test/lsan/TestCases/realloc_too_big.c
--
Exit Code: 1
Command Output (stderr):
--
/home/tcwg-buildslave/worker/clang-cmake-armv7-full/llvm/compiler-rt/test/lsan/TestCases/realloc_too_big.c:10:11: error: CHECK: expected string not found in input
// CHECK: {{Leak|Address}}Sanitizer: detected memory leaks
          ^
<stdin>:2:66: note: scanning from here
==18503==WARNING: LeakSanitizer failed to allocate 0x100001 bytes
                                                                 ^
Input file: <stdin>
Check file: /home/tcwg-buildslave/worker/clang-cmake-armv7-full/llvm/compiler-rt/test/lsan/TestCases/realloc_too_big.c
-dump-input=help explains the following input dump.
Input was:
<<<<<<
          1: nine: 0x408003d0 
          2: ==18503==WARNING: LeakSanitizer failed to allocate 0x100001 bytes 
check:10                                                                      X error: no match found
>>>>>>
--
********************

I'm going to try to reproduce it on our end. I wonder if it could be that the realloc doesn't return NULL for the version of glibc we're using, or it is actually freeing the allocation.

Can you tell me how that alloc size limit is implemented? For instance, if my standard malloc allowed more than that would that be an issue?

[DavidSpickett]

I think the failures are to do with whether we have Address and Leak sanitizer enabled. With both it passes, just leak and it doesn't.

(is leak sanitizer a subset of asan? I'm not too familiar with them)

$ grep -P "(malloc_zero|realloc_too_big)" Downloads/stdio\ \(2\)
PASS: LeakSanitizer-AddressSanitizer-armhf :: TestCases/malloc_zero.c (74005 of 75225)
PASS: LeakSanitizer-AddressSanitizer-armhf :: TestCases/realloc_too_big.c (74008 of 75225)
FAIL: LeakSanitizer-Standalone-armhf :: TestCases/malloc_zero.c (74052 of 75225)
******************** TEST 'LeakSanitizer-Standalone-armhf :: TestCases/malloc_zero.c' FAILED ********************
<...>
FAIL: LeakSanitizer-Standalone-armhf :: TestCases/realloc_too_big.c (74059 of 75225)
******************** TEST 'LeakSanitizer-Standalone-armhf :: TestCases/realloc_too_big.c' FAILED ********************
<...>
  LeakSanitizer-Standalone-armhf :: TestCases/malloc_zero.c
  LeakSanitizer-Standalone-armhf :: TestCases/realloc_too_big.c

I see the same thing on an armv8l machine (32 bit armv8). fsanitize address seems to enable leak and address. Just leak doesn't see the leak.

$ ./bin/clang ../llvm-project/compiler-rt/test/lsan/TestCases/realloc_too_big.c -o /tmp/test.o -fsanitize=address && LSAN_OPTIONS=allocator_may_return_null=1:max_allocation_size_mb=1:use_stacks=0 /tmp/test.o
nine: 0xf55007b0
==409056==WARNING: AddressSanitizer failed to allocate 0x100001 bytes

=================================================================
==409056==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 9 byte(s) in 1 object(s) allocated from:
    #0 0x962f6  (/tmp/test.o+0x962f6)
    #1 0xcb1bc  (/tmp/test.o+0xcb1bc)
    #2 0xf78ee062  (/lib/arm-linux-gnueabihf/libc.so.6+0x17062)

SUMMARY: AddressSanitizer: 9 byte(s) leaked in 1 allocation(s).

$ ./bin/clang ../llvm-project/compiler-rt/test/lsan/TestCases/realloc_too_big.c -o /tmp/test.o -fsanitize=leak && LSAN_OPTIONS=allocator_may_return_null=1:max_allocation_size_mb=1:use
_stacks=0 /tmp/test.o
nine: 0xf6c003d0
==409061==WARNING: LeakSanitizer failed to allocate 0x100001 bytes

(side note: the tests aren't enabled for armv8l but they probably could be, I just bodged it since I had easier access to that sort of machine)

Any ideas?

[DavidSpickett]

Marked the test UNSUPPORTED while we figure out the failure: https://reviews.llvm.org/rG466fab5c9410abb79f1a70c5075147e9a768124e

[MaskRay]

In D99480#2676626, @DavidSpickett wrote:

I think the failures are to do with whether we have Address and Leak sanitizer enabled. With both it passes, just leak and it doesn't.

(is leak sanitizer a subset of asan? I'm not too familiar with them)

$ grep -P "(malloc_zero|realloc_too_big)" Downloads/stdio\ \(2\)
PASS: LeakSanitizer-AddressSanitizer-armhf :: TestCases/malloc_zero.c (74005 of 75225)
PASS: LeakSanitizer-AddressSanitizer-armhf :: TestCases/realloc_too_big.c (74008 of 75225)
FAIL: LeakSanitizer-Standalone-armhf :: TestCases/malloc_zero.c (74052 of 75225)
******************** TEST 'LeakSanitizer-Standalone-armhf :: TestCases/malloc_zero.c' FAILED ********************
<...>
FAIL: LeakSanitizer-Standalone-armhf :: TestCases/realloc_too_big.c (74059 of 75225)
******************** TEST 'LeakSanitizer-Standalone-armhf :: TestCases/realloc_too_big.c' FAILED ********************
<...>
  LeakSanitizer-Standalone-armhf :: TestCases/malloc_zero.c
  LeakSanitizer-Standalone-armhf :: TestCases/realloc_too_big.c

I see the same thing on an armv8l machine (32 bit armv8). fsanitize address seems to enable leak and address. Just leak doesn't see the leak.

$ ./bin/clang ../llvm-project/compiler-rt/test/lsan/TestCases/realloc_too_big.c -o /tmp/test.o -fsanitize=address && LSAN_OPTIONS=allocator_may_return_null=1:max_allocation_size_mb=1:use_stacks=0 /tmp/test.o
nine: 0xf55007b0
==409056==WARNING: AddressSanitizer failed to allocate 0x100001 bytes

=================================================================
==409056==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 9 byte(s) in 1 object(s) allocated from:
    #0 0x962f6  (/tmp/test.o+0x962f6)
    #1 0xcb1bc  (/tmp/test.o+0xcb1bc)
    #2 0xf78ee062  (/lib/arm-linux-gnueabihf/libc.so.6+0x17062)

SUMMARY: AddressSanitizer: 9 byte(s) leaked in 1 allocation(s).

$ ./bin/clang ../llvm-project/compiler-rt/test/lsan/TestCases/realloc_too_big.c -o /tmp/test.o -fsanitize=leak && LSAN_OPTIONS=allocator_may_return_null=1:max_allocation_size_mb=1:use
_stacks=0 /tmp/test.o
nine: 0xf6c003d0
==409061==WARNING: LeakSanitizer failed to allocate 0x100001 bytes

(side note: the tests aren't enabled for armv8l but they probably could be, I just bodged it since I had easier access to that sort of machine)

Any ideas?

On many configurations LeakSanitizer is run as part of -fsanitize=address. LeakSanitizer can run standalone, too.

32-bit architectures generally have larger false negatives because more patterns tend to be recognized as referenced in a 32-bit address space.

The test intentionally tests the implementation-defined behavior (malloc(0)) just to check what happens in this case.

compiler-rt/lib/lsan/lsan_allocator.cpp

void Deallocate(void *p) {
   if (&__sanitizer_free_hook) __sanitizer_free_hook(p);
   RunFreeHooks(p);
   RegisterDeallocation(p);
   allocator.Deallocate(GetAllocatorCache(), p);
 }
 
 void *Reallocate(const StackTrace &stack, void *p, uptr new_size,
                  uptr alignment) {
-  RegisterDeallocation(p);
   if (new_size > max_malloc_size) {
-    allocator.Deallocate(GetAllocatorCache(), p);
-    return ReportAllocationSizeTooBig(new_size, stack);
+    ReportAllocationSizeTooBig(new_size, stack);
+    return nullptr;
   }
-  p = allocator.Reallocate(GetAllocatorCache(), p, new_size, alignment);
+  RegisterDeallocation(p);
+  void *new_p =
+      allocator.Reallocate(GetAllocatorCache(), p, new_size, alignment);
+  if (new_p)
+    RegisterAllocation(stack, new_p, new_size);
+  else if (new_size != 0)
     RegisterAllocation(stack, p, new_size);
-  return p;
+  return new_p;
 }
 
 void GetAllocatorCacheRange(uptr *begin, uptr *end) {
   *begin = (uptr)GetAllocatorCache();
   *end = *begin + sizeof(AllocatorCache);
 }
 
 uptr GetMallocUsableSize(const void *p) {

compiler-rt/test/lsan/TestCases/realloc_oom.c

+// RUN: %clang_lsan %s -o %t
+// RUN: %env_lsan_opts=allocator_may_return_null=1 not %run %t 2>&1 | FileCheck %s
+
+// UNSUPPORTED: i386-linux,arm-linux
+
+#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+// CHECK: {{Leak|Address}}Sanitizer failed to allocate 0xffffffff{{(ffffffff)?}} bytes
+
+// CHECK: {{Leak|Address}}Sanitizer: detected memory leaks
+// CHECK: {{Leak|Address}}Sanitizer: 10 byte(s) leaked in 2 allocation(s).
+
+int main() {
+  // The behavior of malloc(0) is implementation-defined.
+  char *zero = malloc(0);
+  char *nine = malloc(9);
+  fprintf(stderr, "zero: %p\n", zero);
+  fprintf(stderr, "nine: %p\n", nine);
+  assert(realloc(nine, (size_t)-1) == NULL);
+}

compiler-rt/test/lsan/TestCases/realloc_zero.c

+// RUN: %clang_lsan %s -o %t
+// RUN: %run %t
+
+#include <assert.h>
+#include <stdlib.h>
+
+int main() {
+  char *p = malloc(1);
+  // The behavior of realloc(p, 0) is implementation-defined.
+  // We free the allocation.
+  assert(realloc(p, 0) == NULL);
+}