Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(124)

Issue 478002: Don't bail if a thread's stack pointer is invalid (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 11 months ago by Michael Krebs
Modified:
5 years, 11 months ago
CC:
google-breakpad-dev_googlegroups.com
Base URL:
http://google-breakpad.googlecode.com/svn/trunk/
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : #

Total comments: 4

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M src/client/linux/minidump_writer/linux_core_dumper.cc View 1 chunk +2 lines, -3 lines 0 comments Download
M src/client/linux/minidump_writer/linux_core_dumper_unittest.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M src/client/linux/minidump_writer/linux_dumper.h View 1 chunk +1 line, -4 lines 0 comments Download
M src/client/linux/minidump_writer/linux_ptrace_dumper.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M src/client/linux/minidump_writer/minidump_writer.cc View 1 2 3 5 chunks +45 lines, -27 lines 0 comments Download
M src/client/linux/minidump_writer/minidump_writer_unittest.cc View 1 2 3 4 1 chunk +80 lines, -0 lines 0 comments Download

Messages

Total messages: 5
Michael Krebs
BTW, I should be a committer now, so this requires approval only.
5 years, 11 months ago #1
Michael Krebs
I'm hoping to get this change in soon (to help track down crosbug.com/34880). I'll add ...
5 years, 11 months ago #2
Ted Mielczarek
This mostly looks good, I just have a couple of comments. Also, it'd be nice ...
5 years, 11 months ago #3
Michael Krebs
PTAL. (Tried this Friday but Rietveld wasn't working.) I added a unittest for making sure ...
5 years, 11 months ago #4
Ted Mielczarek
5 years, 11 months ago #5
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1004:630ec63f810e-tainted