This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by MaskRay on Mar 28 2021, 6:26 PM.

Details

Summary

This is the behavior required by the standards.

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

Diff Detail

Unit TestsFailed

Event Timeline

MaskRay requested review of this revision.Mar 28 2021, 6:26 PM
MaskRay created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2021, 6:26 PM
MaskRay added inline comments.Mar 28 2021, 8:16 PM
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 updated this revision to Diff 333760.Mar 28 2021, 9:02 PM

Make tests work on aarch64 & ppc64le.

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

vitalybuka added inline comments.Mar 29 2021, 10:18 AM
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 added inline comments.Mar 29 2021, 10:20 AM
compiler-rt/test/lsan/TestCases/malloc_zero.c
4 ↗(On Diff #333760)

why this one is disabled for i386?

MaskRay updated this revision to Diff 333930.Mar 29 2021, 10:55 AM
MaskRay marked 2 inline comments as done.

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 accepted this revision.Mar 29 2021, 11:07 AM
vitalybuka added inline comments.
compiler-rt/test/lsan/TestCases/malloc_zero.c
19 ↗(On Diff #333930)

this should help

This revision is now accepted and ready to land.Mar 29 2021, 11:07 AM
MaskRay updated this revision to Diff 333949.Mar 29 2021, 11:43 AM

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).

This revision was landed with ongoing or failed builds.Mar 29 2021, 1:35 PM
This revision was automatically updated to reflect the committed changes.

@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?

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?

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

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.