Don't bail if a thread's stack pointer is invalid

Don't bail if a thread's stack pointer is invalid

Currently, if a thread's stack pointer is not within a valid memory page,
the minidump writing will fail with an error.  This change allows an invalid
stack pointer by simply setting the memory size to zero in the minidump.
The processing code already checks for the size being zero, although it
currently just gives an error (see https://breakpad.appspot.com/413002/).

BUG=google-breakpad:499, chromium-os:34880
TEST=make check, manually ran minidump-2-core and core2md
Committed: https://code.google.com/p/google-breakpad/source/detail?r=1065

[Michael Krebs]

BTW, I should be a committer now, so this requires approval only.

[Michael Krebs]

I'm hoping to get this change in soon (to help track down crosbug.com/34880).

I'll add mark@ as a reviewer as well, since he's on
https://breakpad.appspot.com/413002/.

[Ted Mielczarek]

This mostly looks good, I just have a couple of comments.

Also, it'd be nice to be able to unit-test this, but that might be hard. Maybe
you can write a test where a forked child uses inline assembly to intentionally
set its stack pointer to invalid memory before crashing?

https://breakpad.appspot.com/478002/diff/8/src/client/linux/minidump_writer/m...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/478002/diff/8/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:673: thread.stack.memory.rva
= minidump_writer_.position();
This seems a little weird, since the data is of size zero. Does setting the rva
to zero break things on the minidump reading side? That would make it much
clearer that there's no valid data here.

https://breakpad.appspot.com/478002/diff/8/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:752: }
Feels like you should pull this repeated block out into a separate function.

[Michael Krebs]

PTAL.  (Tried this Friday but Rietveld wasn't working.)

I added a unittest for making sure an invalid stack pointer still generates a
minidump.  There's a part of the test that's disabled until
https://breakpad.appspot.com/413002/ is committed.

https://breakpad.appspot.com/478002/diff/8/src/client/linux/minidump_writer/m...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/478002/diff/8/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:673: thread.stack.memory.rva
= minidump_writer_.position();
On 2012/10/05 12:45:46, Ted Mielczarek wrote:
> This seems a little weird, since the data is of size zero. Does setting the
rva
> to zero break things on the minidump reading side? That would make it much
> clearer that there's no valid data here.

I did it that way in order to keep the output as "valid" as possible. 
Technically, a reader could seek to this position and read zero bytes, and
chances are that would still work as it is now.  But if I set it to zero, then a
reader has to know that they should ignore zero-sized data, and if they don't
they might erroneously seek to the arbitrary offset of zero.  Granted, that
likely is not a big deal, but it's a little less "correct" in my opinion.

But I haven't dealt with the reading side of minidumps much at all, so I'm fine
changing this if you think that is a more stable approach.

https://breakpad.appspot.com/478002/diff/8/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:752: }
On 2012/10/05 12:45:46, Ted Mielczarek wrote:
> Feels like you should pull this repeated block out into a separate function.

Done.

[Ted Mielczarek]

On 2012/10/08 19:22:48, Michael Krebs wrote:
> PTAL.  (Tried this Friday but Rietveld wasn't working.)
> 
> I added a unittest for making sure an invalid stack pointer still generates a
> minidump.  There's a part of the test that's disabled until
> https://breakpad.appspot.com/413002/ is committed.
> 
>
https://breakpad.appspot.com/478002/diff/8/src/client/linux/minidump_writer/m...
> File src/client/linux/minidump_writer/minidump_writer.cc (right):
> 
>
https://breakpad.appspot.com/478002/diff/8/src/client/linux/minidump_writer/m...
> src/client/linux/minidump_writer/minidump_writer.cc:673:
thread.stack.memory.rva
> = minidump_writer_.position();
> On 2012/10/05 12:45:46, Ted Mielczarek wrote:
> I did it that way in order to keep the output as "valid" as possible. 
> Technically, a reader could seek to this position and read zero bytes, and
> chances are that would still work as it is now.  But if I set it to zero, then
a
> reader has to know that they should ignore zero-sized data, and if they don't
> they might erroneously seek to the arbitrary offset of zero.  Granted, that
> likely is not a big deal, but it's a little less "correct" in my opinion.
> 
> But I haven't dealt with the reading side of minidumps much at all, so I'm
fine
> changing this if you think that is a more stable approach.

It's not a big deal either way, it'd only make reading minidump_dump output
marginally easier, but length=0 is still there.

LGTM, thanks for the test.