Limit the number of frames we try to walk to prevent runaway processors.

Limit the number of frames we try to walk to prevent runaway processors.

Committed: http://code.google.com/p/google-breakpad/source/detail?r=672

[Lei Zhang (chromium)]

I had a bad .sym file that caused minidump_stackwalk to go into an infinite
loop, chewing up several GBs of memory because I stopped it.

Should we add this as a sanity check? Or perhaps just print an error and break
out of the loop?

[Mark Mentovai]

I had (mis-)remembered this code as having a better check that ensured that each
successive stack frame kept “moving” in the right direction.

Lei, what did your runaway stack look like? Were the stack pointers the same for
each successive frame?

[Ted Mielczarek]

On 2010/08/26 14:27:19, Mark Mentovai wrote:
> I had (mis-)remembered this code as having a better check that ensured that
each
> successive stack frame kept “moving” in the right direction.

The x86, amd64 and arm stackwalkers do:
http://code.google.com/p/google-breakpad/source/browse/trunk/src/processor/st...
http://code.google.com/p/google-breakpad/source/browse/trunk/src/processor/st...
http://code.google.com/p/google-breakpad/source/browse/trunk/src/processor/st...

Was this on another cpu arch or did you have something weirder going on?

[jimb]

[Sorry for the double-send; wanted to get this in the issue.]

Interesting that the test is '<=' in the x86 code, but '<' in the ARM
code; if the SP never changed, the ARM code could go on forever.

On the x86, calls always at least push a return address, but on the
ARM one can make 'frameless' calls to leaf functions: the call
instruction saves the PC in the link register (LR), the callee runs
entirely in registers, and then returns by jumping to LR. In this
case, the SP of the callee and caller would legitimately be equal.

However, frameless functions cannot call other functions: they would
need to save their own value of LR before they make the call. They
can't simply put it in a callee-saves register, since they must
preserve their own caller's values for those registers. So if one
calls other functions, one needs to have one's own stack frame.

So if it's the ARM code that's getting into this infinite loop, then
we could fix it by requiring the caller's SP to be strictly > the
callee's SP *unless* the callee is the youngest frame, in which case
we require >=.

(This would need to be thoroughly commented, and have tests written for it.)

All that aside, I think limiting the stack to 1024 frames is probably
a reasonable sanity check.

[Lei Zhang (chromium)]

On 2010/08/26 14:27:19, Mark Mentovai wrote:
> I had (mis-)remembered this code as having a better check that ensured that
each
> successive stack frame kept “moving” in the right direction.
> 
> Lei, what did your runaway stack look like? Were the stack pointers the same
for
> each successive frame?

The stack pointer was moving in the right direction. This was on amd64.

I changed the patch to break out of the loop instead.

Thread 34
 0  libpthread-2.11.1.so + 0xe464
    rbx = 0x04b65180   r12 = 0x0042d718   r13 = 0x032b9280   r14 = 0x0511d2d0
    r15 = 0x052e5900   rip = 0x57353464   rsp = 0x428c7208   rbp = 0x032b9280
 1  libpthread-2.11.1.so + 0x95d8
    rbx = 0x04b65180   r12 = 0x0042d718   r13 = 0x032b9280   r14 = 0x0511d2d0
    r15 = 0x052e5900   rip = 0x5734e5d9   rsp = 0x428c7220   rbp = 0x032b9280
 2  libpthread-2.11.1.so + 0x95d8
    rbx = 0x04b65180   r12 = 0x0042d718   r13 = 0x032b9280   r14 = 0x0511d2d0
    r15 = 0x052e5900   rip = 0x5734e5d9   rsp = 0x428c72a0   rbp = 0x032b9280
 3  libpthread-2.11.1.so + 0x95d8
    rbx = 0x04b65180   r12 = 0x0042d718   r13 = 0x032b9280   r14 = 0x0511d2d0
    r15 = 0x052e5900   rip = 0x5734e5d9   rsp = 0x428c7320   rbp = 0x032b9280
 4  libpthread-2.11.1.so + 0x95d8
    rbx = 0x04b65180   r12 = 0x0042d718   r13 = 0x032b9280   r14 = 0x0511d2d0
    r15 = 0x052e5900   rip = 0x5734e5d9   rsp = 0x428c73a0   rbp = 0x032b9280
 5  libpthread-2.11.1.so + 0x95d8
    rbx = 0x04b65180   r12 = 0x0042d718   r13 = 0x032b9280   r14 = 0x0511d2d0
    r15 = 0x052e5900   rip = 0x5734e5d9   rsp = 0x428c7420   rbp = 0x032b9280
...

[Mark Mentovai]

LGTM

http://breakpad.appspot.com/175001/diff/9001/10001
File src/google_breakpad/processor/stackwalker.h (right):

http://breakpad.appspot.com/175001/diff/9001/10001#newcode153
Line 153: // This default to 1024 to prevent infinite loops.
default -> defaults