Augment SyzyASAN crash reports with stack traces on alloc/free.

Augment SyzyASAN crash reports with stack traces on alloc/free.


BUG=

Committed: https://code.google.com/p/sawbuck/source/detail?r=1211

[Sébastien Marchand, 2012-10-30 19:59:50]

PTAL.

[Roger McFarlane, 2012-10-30 20:08:32]

Nice!

One nit, one suggestion.

https://codereview.appspot.com/6817056/diff/3004/syzygy/agent/asan/asan_heap.cc
File syzygy/agent/asan/asan_heap.cc (right):

https://codereview.appspot.com/6817056/diff/3004/syzygy/agent/asan/asan_heap....
syzygy/agent/asan/asan_heap.cc:57: void CaptureStackTrace(void** stack_trace,
size_t* trace_size) {
maybe populate a vector of const void*?

https://codereview.appspot.com/6817056/diff/3004/syzygy/agent/asan/asan_heap....
syzygy/agent/asan/asan_heap.cc:58: DCHECK(*stack_trace == NULL);
DCHECK(stack_trace != NULL);  // dcheck this before deref

[sebmarchand - Google Account, 2012-10-30 22:53:26]

Some comments on Roger's comments.

https://codereview.appspot.com/6817056/diff/3004/syzygy/agent/asan/asan_heap.cc
File syzygy/agent/asan/asan_heap.cc (right):

https://codereview.appspot.com/6817056/diff/3004/syzygy/agent/asan/asan_heap....
syzygy/agent/asan/asan_heap.cc:57: void CaptureStackTrace(void** stack_trace,
size_t* trace_size) {
On 2012/10/30 20:08:33, Roger McFarlane wrote:
> maybe populate a vector of const void*?

Hum, after some check I've realized that this will have some impacts on the
actual implementation:
- We have to increase the size of the red zone from 32 to 64 bytes.
- In the alloc function we don't construct a BlockHeader, we just cast the
address of the redzone to a BlockHeader*, this require BlockHeader to be a POD
struct.

So replacing the dynamic array by a vector have an impact on speed
(initialization of the struct) and memory (32 to 64 bytes). I'm not sure it's
the best thing to do, but I might be wrong :)

[Roger McFarlane, 2012-10-31 14:16:24]

https://codereview.appspot.com/6817056/diff/3004/syzygy/agent/asan/asan_heap.cc
File syzygy/agent/asan/asan_heap.cc (right):

https://codereview.appspot.com/6817056/diff/3004/syzygy/agent/asan/asan_heap....
syzygy/agent/asan/asan_heap.cc:57: void CaptureStackTrace(void** stack_trace,
size_t* trace_size) {
On 2012/10/30 22:53:26, sebmarchand wrote:
> On 2012/10/30 20:08:33, Roger McFarlane wrote:
> > maybe populate a vector of const void*?
> 
> Hum, after some check I've realized that this will have some impacts on the
> actual implementation:
> - We have to increase the size of the red zone from 32 to 64 bytes.
> - In the alloc function we don't construct a BlockHeader, we just cast the
> address of the redzone to a BlockHeader*, this require BlockHeader to be a POD
> struct.
> 
> So replacing the dynamic array by a vector have an impact on speed
> (initialization of the struct) and memory (32 to 64 bytes). I'm not sure it's
> the best thing to do, but I might be wrong :)


As we discussed, leave this as a pointer and a size. IIRC, the stored size (as a
number of stack frames) can fit in a single byte as I think the limit is 62.

[Sébastien Marchand, 2012-10-31 14:33:42]

Thanks, PTAnL.

https://codereview.appspot.com/6817056/diff/3004/syzygy/agent/asan/asan_heap.cc
File syzygy/agent/asan/asan_heap.cc (right):

https://codereview.appspot.com/6817056/diff/3004/syzygy/agent/asan/asan_heap....
syzygy/agent/asan/asan_heap.cc:57: void CaptureStackTrace(void** stack_trace,
size_t* trace_size) {
On 2012/10/31 14:16:24, Roger McFarlane wrote:
> On 2012/10/30 22:53:26, sebmarchand wrote:
> > On 2012/10/30 20:08:33, Roger McFarlane wrote:
> > > maybe populate a vector of const void*?
> > 
> > Hum, after some check I've realized that this will have some impacts on the
> > actual implementation:
> > - We have to increase the size of the red zone from 32 to 64 bytes.
> > - In the alloc function we don't construct a BlockHeader, we just cast the
> > address of the redzone to a BlockHeader*, this require BlockHeader to be a
POD
> > struct.
> > 
> > So replacing the dynamic array by a vector have an impact on speed
> > (initialization of the struct) and memory (32 to 64 bytes). I'm not sure
it's
> > the best thing to do, but I might be wrong :)
> 
> 
> As we discussed, leave this as a pointer and a size. IIRC, the stored size (as
a
> number of stack frames) can fit in a single byte as I think the limit is 62.

Oh, I thought it was just a suggestion in the case the vector version was using
too much memory. Done.

https://codereview.appspot.com/6817056/diff/3004/syzygy/agent/asan/asan_heap....
syzygy/agent/asan/asan_heap.cc:58: DCHECK(*stack_trace == NULL);
On 2012/10/30 20:08:33, Roger McFarlane wrote:
> DCHECK(stack_trace != NULL);  // dcheck this before deref

Done.

[Roger McFarlane, 2012-10-31 15:08:46]

lgtm with nits

https://codereview.appspot.com/6817056/diff/7001/syzygy/agent/asan/asan_heap.cc
File syzygy/agent/asan/asan_heap.cc (right):

https://codereview.appspot.com/6817056/diff/7001/syzygy/agent/asan/asan_heap....
syzygy/agent/asan/asan_heap.cc:286: free_block->alloc_stack_trace = NULL;
reset the stack trace sizes here and below

https://codereview.appspot.com/6817056/diff/7001/syzygy/agent/asan/asan_heap.h
File syzygy/agent/asan/asan_heap.h (right):

https://codereview.appspot.com/6817056/diff/7001/syzygy/agent/asan/asan_heap....
syzygy/agent/asan/asan_heap.h:107: uint8 free_stack_trace_size;
change the order of these members, put the byte sized members at the end
(otherwise the compiler will pad).

Also, somewhere in the C++ file, COMPILE_ASSERT that the size is as you expect.

[Sébastien Marchand, 2012-10-31 15:16:44]

Thanks, committing.

https://codereview.appspot.com/6817056/diff/7001/syzygy/agent/asan/asan_heap.cc
File syzygy/agent/asan/asan_heap.cc (right):

https://codereview.appspot.com/6817056/diff/7001/syzygy/agent/asan/asan_heap....
syzygy/agent/asan/asan_heap.cc:286: free_block->alloc_stack_trace = NULL;
On 2012/10/31 15:08:46, Roger McFarlane wrote:
> reset the stack trace sizes here and below

Done.

https://codereview.appspot.com/6817056/diff/7001/syzygy/agent/asan/asan_heap.h
File syzygy/agent/asan/asan_heap.h (right):

https://codereview.appspot.com/6817056/diff/7001/syzygy/agent/asan/asan_heap....
syzygy/agent/asan/asan_heap.h:107: uint8 free_stack_trace_size;
On 2012/10/31 15:08:46, Roger McFarlane wrote:
> change the order of these members, put the byte sized members at the end
> (otherwise the compiler will pad).
> 
> Also, somewhere in the C++ file, COMPILE_ASSERT that the size is as you
expect.

Done. There's already a COMPILE_ASSERT for the size in HeapProxy::Create.