Do not rely on malloc context in allocator reports

[GoogleCodeExporter]

Hi,

I've turned off malloc contexts (via ASAN_OPTIONS=malloc_context_size=0) on my 
system because they seem to be rather costly. It turns out that this prevents 
reporting routines (e.g. ReportAllocTypeMismatch) from printing the stack trace 
because they rely on GET_STACK_TRACE_MALLOC for this. Can we change 
libsanitizer to do proper stack unwind (via GET_STACK_TRACE_FATAL) in case 
stack trace obtained from malloc is empty?

I'm using trunk gcc, can retest on Clang if necessary.

-Y

Original issue reported on code.google.com by tetra2...@gmail.com on 1 Nov 2013 at 2:22

[GoogleCodeExporter]

I am not sure I understand your situation, please provide a test and a command 
line. 

Also, did you try ASAN_OPTIONS=fast_unwind_on_malloc=0 ? 

Original comment by konstant...@gmail.com on 1 Nov 2013 at 2:46

[GoogleCodeExporter]

Note that we haven't done merges to gcc since Feb (a fresh merge is in flight 
*now*), 
so please retest with fresh clang or try gcc in a few days. 

Original comment by konstant...@gmail.com on 1 Nov 2013 at 11:39

[GoogleCodeExporter]

> I am not sure I understand your situation, please provide a test and a 
command line.

Sorry, I should probably avoid submitting bugreports on Friday evenings)

Here is a simple program which should trigger alloc-dealloc warning:

 $ cat main.cpp 
 void f(char *p) {
   delete p;
 }

 int main() {
   char *p = new char[20];
   f(p);
   return 0;
 }

I then compile with Sanitizer support:
 $ ~/install/bin/x86_64-unknown-linux-gnu-clang -g -O0 main.cpp -fsanitize=address -o a.out.llvm
(llvm, clang and compiler-rt are up-to-date).

By default it will then report a full stack trace on error:
 $ ./a.out.llvm
 ...
 ==2190==ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new [] vs operator delete) on 0x60300000efe0
    #0 0x46dc69 (/mnt/scratch/ygribov/vdtools/tests/alloc_dealloc_mismatch/a.out.llvm+0x46dc69)
    #1 0x482c1c (/mnt/scratch/ygribov/vdtools/tests/alloc_dealloc_mismatch/a.out.llvm+0x482c1c)
    #2 0x482df4 (/mnt/scratch/ygribov/vdtools/tests/alloc_dealloc_mismatch/a.out.llvm+0x482df4)
    #3 0x7f2d3e7d476c (/lib/x86_64-linux-gnu/libc.so.6+0x2176c)

 0x60300000efe0 is located 0 bytes inside of 20-byte region [0x60300000efe0,0x60300000eff4)
 allocated by thread T0 here:
 ...

But if I disable malloc contexts the stack trace becomes unavailable:
 $ ASAN_OPTIONS=malloc_context_size=0 ./a.out.llvm
 ...
 ==2229==ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new [] vs operator delete) on 0x60300000efe0
    #0 0x46dc69 (/mnt/scratch/ygribov/vdtools/tests/alloc_dealloc_mismatch/a.out.llvm+0x46dc69)

 0x60300000efe0 is located 0 bytes inside of 20-byte region [0x60300000efe0,0x60300000eff4)
 allocated by thread T0 here:
 ...

I tried adding fast_unwind_on_malloc=0 per your advice but this triggered a 
CHECK in GetStackTraceFromId (already reported here btw 
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58718):
 $ ASAN_OPTIONS=malloc_context_size=0 ./a.out.llvm
 ...
 ==2301==AddressSanitizer CHECK failed: /mnt/scratch/ygribov/llvm/projects/compiler-rt/lib/asan/asan_allocator2.cc:237 "((id)) != (0)" (0x0, 0x0)

I worked around this check but libsanitizer still didn't report full stack to 
me:
 ==9050==ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new [] vs operator delete) on 0x60300000efe0

 0x60300000efe0 is located 0 bytes inside of 20-byte region [0x60300000efe0,0x60300000eff4)
 allocated by thread T0 here:
 ...

I think the ultimate reason is that we use GET_STACK_TRACE_FREE to obtain stack 
for all allocator-related errors instead of GET_STACK_TRACE. IMHO there should 
be no need to consult malloc_context_size/fast_unwind_on_malloc when reporting 
a fatal allocation error.

-Y

Original comment by tetra2...@gmail.com on 5 Nov 2013 at 8:37

[GoogleCodeExporter]

>> But if I disable malloc contexts the stack trace becomes unavailable:
I don't understand what you are complaining about. 
Disabling malloc context means you don't collect stack traces on malloc calls
and sure enough "stack trace becomes unavailable"

The check failure with 
ASAN_OPTIONS=malloc_context_size=0:fast_unwind_on_malloc=0
is something we'll need to fix

Original comment by konstant...@gmail.com on 5 Nov 2013 at 5:04

[GoogleCodeExporter]

> The check failure with 
ASAN_OPTIONS=malloc_context_size=0:fast_unwind_on_malloc=0
> is something we'll need to fix

Is it still reproducible? I thought I've fixed this already.  // Can't check 
right now

Original comment by timurrrr@google.com on 5 Nov 2013 at 6:31

[GoogleCodeExporter]

yes

Original comment by konstant...@gmail.com on 5 Nov 2013 at 6:40

[GoogleCodeExporter]

> I don't understand what you are complaining about. 
> Disabling malloc context means you don't collect stack traces on malloc calls
> and sure enough "stack trace becomes unavailable"

Here is my understanding of this: even though malloc context is disabled, we 
can detect this and do an ordinary stack unwind when critical error is detected 
in allocator (like alloc-dealloc mismatch or double free).

-Y

Original comment by tetra2...@gmail.com on 5 Nov 2013 at 6:55

[GoogleCodeExporter]

I think this makes sense.
Currently we have a distinction between fast_unwind_on_(fatal|malloc), but what 
we really want is to make a distinction between fast_unwind_on(fatal|nonfatal).

The problem is we are unwinding stack on the mismatching free/delete before we 
know that it is mismatching.

Original comment by euge...@google.com on 5 Nov 2013 at 7:02

[GoogleCodeExporter]

ah, got it! 
>> The problem is we are unwinding stack on the mismatching free/delete before 
we know that it is mismatching.

Yes, correct. 

Original comment by konstant...@gmail.com on 5 Nov 2013 at 7:13

[GoogleCodeExporter]

Thanks, Eugene!

> Currently we have a distinction between fast_unwind_on_(fatal|malloc),
> but what we really want is to make a distinction
> between fast_unwind_on(fatal|nonfatal).

Frankly speaking I have an ARM target so none of these really helps...

> The problem is we are unwinding stack on the mismatching free/delete
> before we know that it is mismatching.

Right, but could we re-unwind it once we know about mismatch?

-Y

Original comment by tetra2...@gmail.com on 5 Nov 2013 at 7:15

[GoogleCodeExporter]

>> Right, but could we re-unwind it once we know about mismatch?
I think yes. 

Original comment by konstant...@gmail.com on 5 Nov 2013 at 7:17

[GoogleCodeExporter]

>>> Right, but could we re-unwind it once we know about mismatch?
> I think yes. 

Cool! Here is the list of handlers that may need this:
* ReportDoubleFree
* ReportFreeNotMalloced
* ReportInvalidFree
* ReportAllocTypeMismatch
* ReportMallocUsableSizeNotOwned
* ReportAsanGetAllocatedSizeNotOwned

-Y

Original comment by tetra2...@gmail.com on 5 Nov 2013 at 7:23

[GoogleCodeExporter]

I won't get to this for a while (traveling, etc).
You may want to send a patch to llvm yourself to get this faster :) 
(use http://llvm.org/docs/Phabricator.html)

Original comment by konstant...@gmail.com on 5 Nov 2013 at 7:25

[GoogleCodeExporter]

I agree that we should use fatal unwinder when we report allocator problems.

I've fixed ASan crash with 
ASAN_OPTIONS=malloc_context_size=0:fast_unwind_on_malloc=0 in LLVM r194107. 
Sorry about that (too many unwinders...)

Original comment by samso...@google.com on 5 Nov 2013 at 11:37

• Changed state: Accepted

[GoogleCodeExporter]

> You may want to send a patch to llvm yourself to get this faster :) 

Will do once I'm done with the usual corporate madness...

Original comment by tetra2...@gmail.com on 6 Nov 2013 at 9:48

[GoogleCodeExporter]

Waiting to get account on Phabricator. Meanwhile, here is a draft patch so that 
you guys could check whether I'm going in right direction.

Two questions:
1) I'll have hard times checking this on Mac (noone here has it). How should I 
proceed?
2) Should I instrument Mac-specific report functions (ReportMac* from 
asan_report.cc) as well?

-Y

Original comment by tetra2...@gmail.com on 7 Nov 2013 at 12:16

Attachments:

• alloc_dealloc_patch_1.diff

[GoogleCodeExporter]

few comments: 
  - The code has pairs of GET_STACK_TRACE_MALLOC/GET_CURRENT_FRAME_INFO
that's bad since we are effectively extracting GET_CURRENT_PC_BP_SP twice, 
right? 
I don't want to rely on the compiler to optimize this. 
  - We'll need lit-style tests (asan/lit_tests/...) to verify the output. 
  - FrameInfo doesn't belong to asan, we may want to have in in sanitizer_common (if we want it at all)

Alexey, please coordinate with Yuri so that you don't duplicate each other's 
work. 

Original comment by konstant...@gmail.com on 7 Nov 2013 at 3:54

[GoogleCodeExporter]

I both support the direction of this change, and agree with Kostya's comments. 
Note that if we only need pc+bp pair to re-unwind stack trace if necessary, we 
may save bp (of the top frame) inside StackTrace structure when we do unwinding 
and avoiding introducing FrameInfo struct.

Yuri, do you plan to continue working on this?

Original comment by samso...@google.com on 11 Nov 2013 at 11:29

[GoogleCodeExporter]

> Yuri, do you plan to continue working on this?

Yes, I hope to send out the updated patch today or tomorrow.

> Note that if we only need pc+bp pair to re-unwind stack trace if necessary,
> we may save bp (of the top frame) inside StackTrace structure
> when we do unwinding and avoiding introducing FrameInfo struct.

If you are ok with nuking sp I can redo it the way you suggest.

Original comment by tetra2...@gmail.com on 11 Nov 2013 at 11:47

[GoogleCodeExporter]

Looks like we don't need sp to re-unwind, so I'm ok with it.

Original comment by samso...@google.com on 11 Nov 2013 at 11:54

[GoogleCodeExporter]

Should be fixed by http://llvm.org/viewvc/llvm-project?rev=194579&view=rev

Yuri, thanks for working on this!

Original comment by samso...@google.com on 13 Nov 2013 at 2:54

• Changed state: Fixed

[GoogleCodeExporter]

Np, was a pleasure!

Original comment by tetra2...@gmail.com on 13 Nov 2013 at 4:54