Fix stack collection with size limit

Fix stack collection with size limit

src/client/linux/minidump_writer/minidump_writer.cc:273 obtains the
stack info by calling GetStackInfo(). That method will return the
stack base address, aligned to the bottom of the memory page that
'stack_pointer' is in. After that it will cap the size of the memory
area to be copied into the minidump to 'max_stack_len', starting from
the base address, if the caller requested so. This will be the case
when collecting reduced stacks, as introduced by this change:
https://breakpad.appspot.com/487002/

In such cases the caller will request 2048 bytes of memory. However
GetStackInfo() will have aligned the base address to the page
boundary, by default 4096 bytes. If the stack, which grows towards the
base address from the top ends before the 2048 bytes of the first
block, then we will not collect any useful part of the stack.

As a fix we skip chunks of 'max_stack_len' bytes starting from
the base address until the stack_pointer is actually contained in the
chunk, which we will add to the minidump file.

BUG=https://bugs.chromium.org/p/google-breakpad/issues/detail?id=695
R=ivanpe@chromium.org

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/f25a4112004efca7065068e87155757ef821d1e9

[lv, 2016-05-06 19:59:20]

I don't know how to pick reviewers. Could someone please have a look? Thanks,
Lars

[lv, 2016-05-06 22:55:49]

lv@cloudera.com changed reviewers:
+ mark@chromium.org, mmandlis@chromium.org, vadimsh@chromium.org

[Mark Mentovai, 2016-05-16 17:30:32]

mark@chromium.org changed reviewers:
+ ivanpe@chromium.org

[Mark Mentovai, 2016-05-16 17:30:33]

Ivan gave the LG on the original change that this updates, so +him.

[ivanpe, 2016-05-17 01:03:10]

https://codereview.chromium.org/1959643004/diff/1/src/client/linux/minidump_w...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://codereview.chromium.org/1959643004/diff/1/src/client/linux/minidump_w...
src/client/linux/minidump_writer/minidump_writer.cc:280: while (int_stack + 
max_stack_len < stack_pointer) {
Please, remove the extra blank after '+'

https://codereview.chromium.org/1959643004/diff/1/src/client/linux/minidump_w...
src/client/linux/minidump_writer/minidump_writer.cc:281: int_stack +=
max_stack_len;
This will be an infinite loop when max_stack_len = 0.

[lv, 2016-05-23 15:54:57]

Thanks for the review, please see the new patch.

https://codereview.chromium.org/1959643004/diff/1/src/client/linux/minidump_w...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://codereview.chromium.org/1959643004/diff/1/src/client/linux/minidump_w...
src/client/linux/minidump_writer/minidump_writer.cc:280: while (int_stack + 
max_stack_len < stack_pointer) {
On 2016/05/17 01:03:10, ivanpe wrote:
> Please, remove the extra blank after '+'

Done.

https://codereview.chromium.org/1959643004/diff/1/src/client/linux/minidump_w...
src/client/linux/minidump_writer/minidump_writer.cc:281: int_stack +=
max_stack_len;
On 2016/05/17 01:03:10, ivanpe wrote:
> This will be an infinite loop when max_stack_len = 0.

I added an additional check. I don't think anyone uses a value of 0 here, so I
could also re-purpose 0 instead of -1 to indicate no cap, which would simplify
the code a bit. Let me know which way you prefer.

[ivanpe, 2016-05-23 17:37:01]

On 2016/05/23 15:54:57, lv wrote:
> Thanks for the review, please see the new patch.
> 
>
https://codereview.chromium.org/1959643004/diff/1/src/client/linux/minidump_w...
> File src/client/linux/minidump_writer/minidump_writer.cc (right):
> 
>
https://codereview.chromium.org/1959643004/diff/1/src/client/linux/minidump_w...
> src/client/linux/minidump_writer/minidump_writer.cc:280: while (int_stack + 
> max_stack_len < stack_pointer) {
> On 2016/05/17 01:03:10, ivanpe wrote:
> > Please, remove the extra blank after '+'
> 
> Done.
> 
>
https://codereview.chromium.org/1959643004/diff/1/src/client/linux/minidump_w...
> src/client/linux/minidump_writer/minidump_writer.cc:281: int_stack +=
> max_stack_len;
> On 2016/05/17 01:03:10, ivanpe wrote:
> > This will be an infinite loop when max_stack_len = 0.
> 
> I added an additional check. I don't think anyone uses a value of 0 here, so I
> could also re-purpose 0 instead of -1 to indicate no cap, which would simplify
> the code a bit. Let me know which way you prefer.

OK.  This looks safer to me.  LGTM.

[ivanpe, 2016-05-24 18:49:38]

Description was changed from

==========
Fix stack collection with size limit

src/client/linux/minidump_writer/minidump_writer.cc:273 obtains the
stack info by calling GetStackInfo(). That method will return the
stack base address, aligned to the bottom of the memory page that
'stack_pointer' is in. After that it will cap the size of the memory
area to be copied into the minidump to 'max_stack_len', starting from
the base address, if the caller requested so. This will be the case
when collecting reduced stacks, as introduced by this change:
https://breakpad.appspot.com/487002/

In such cases the caller will request 2048 bytes of memory. However
GetStackInfo() will have aligned the base address to the page
boundary, by default 4096 bytes. If the stack, which grows towards the
base address from the top ends before the 2048 bytes of the first
block, then we will not collect any useful part of the stack.

As a fix we skip chunks of 'max_stack_len' bytes starting from
the base address until the stack_pointer is actually contained in the
chunk, which we will add to the minidump file.

BUG=https://bugs.chromium.org/p/google-breakpad/issues/detail?id=695
R=
==========

to

==========
Fix stack collection with size limit

src/client/linux/minidump_writer/minidump_writer.cc:273 obtains the
stack info by calling GetStackInfo(). That method will return the
stack base address, aligned to the bottom of the memory page that
'stack_pointer' is in. After that it will cap the size of the memory
area to be copied into the minidump to 'max_stack_len', starting from
the base address, if the caller requested so. This will be the case
when collecting reduced stacks, as introduced by this change:
https://breakpad.appspot.com/487002/

In such cases the caller will request 2048 bytes of memory. However
GetStackInfo() will have aligned the base address to the page
boundary, by default 4096 bytes. If the stack, which grows towards the
base address from the top ends before the 2048 bytes of the first
block, then we will not collect any useful part of the stack.

As a fix we skip chunks of 'max_stack_len' bytes starting from
the base address until the stack_pointer is actually contained in the
chunk, which we will add to the minidump file.

BUG=https://bugs.chromium.org/p/google-breakpad/issues/detail?id=695
R=ivanpe@chromium.org

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/f25a4112004efca7065068e...
==========

[ivanpe, 2016-05-24 18:49:39]

Committed patchset #2 (id:20001) manually as
f25a4112004efca7065068e87155757ef821d1e9 (presubmit successful).