Stackwalker sees invalid stack memory for the thread

This change is addressing a particularly nasty issue where the stackwalker
doesn't see the correct thread stack memory.  Instead, it loads garbage
(from offset 0 of the minidump file - well that's not garbage, but it is
not the stack memory region either) and attempts to walk it.  A typical
symptom of this issue is when you get a single stack frame after
processing - the context frame - for which you don't need stack memory.

This issue is caused by an invalid RVA in the memory descriptor stored
inside the MINIDUMP_THREAD structure for the thread.  Luckily, the
invalid RVA is 0, and the start_of_memory_region appears to be correct,
so this issue can be easily detected and the correct memory region can be
loaded using an RVA specified in the MinidumpMemoryList.

I couldn't find a reasonable description on MSDN regarding
MINIDUMP_MEMORY_DESCRIPTOR.MINIDUMP_LOCATION_DESCRIPTOR having RVA of 0
except maybe for full dumps where the 64-bit version of the structure
(MINIDUMP_MEMORY_DESCRIPTOR64) is used and it has no RVA at all.  It has
a 64-bit DataSize which if interpreted as the 32-bit structure will very
likely result in 0 for the RVA:
  http://msdn.microsoft.com/en-us/library/windows/desktop/ms680384(v=vs.85).aspx


Anyways, the dump that I looked at was not a full dump so 0 for RVA is a
bit puzzling (at least easily detectable):
...
Microsoft (R) Windows Debugger Version 6.2.9200.20512 X86
Copyright (c) Microsoft Corporation. All rights reserved.
...
User Mini Dump File: Only registers, stack and portions of memory are available
...
MINIDUMP_HEADER:
Version         A793 (62F0)
NumberOfStreams 11
Flags           160
                0020 MiniDumpWithUnloadedModules
                0040 MiniDumpWithIndirectlyReferencedMemory
                0100 MiniDumpWithProcessThreadData
Committed: https://code.google.com/p/google-breakpad/source/detail?r=1194

[Ivan Penkov]

.

[Mark Mentovai]

Start with the last comment I raised because it might invalidate the preceding
ones.

https://breakpad.appspot.com/606002/diff/2001/src/google_breakpad/processor/m...
File src/google_breakpad/processor/minidump.h (right):

https://breakpad.appspot.com/606002/diff/2001/src/google_breakpad/processor/m...
src/google_breakpad/processor/minidump.h:334: // containing the thread stack
memory in the MinidumpMemoryList.
This explains the problem but it doesn’t explain how to use this function.
Specifically, what’s the argument and what’s the return value?

Why is it a bool? It might be handier to return the MinidumpMemoryRegion*

https://breakpad.appspot.com/606002/diff/2001/src/google_breakpad/processor/m...
src/google_breakpad/processor/minidump.h:335: virtual bool
UseMemoryFromMinidumpMemoryList(MinidumpMemoryList *memory_list);
This doesn’t need to be virtual, does it?

https://breakpad.appspot.com/606002/diff/2001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://breakpad.appspot.com/606002/diff/2001/src/processor/minidump.cc#newco...
src/processor/minidump.cc:1544: if (region != NULL) {
This file rarely uses the |!= NULL| style, it would normally just use |if
(region) {|. In fact, you used that style in this function already, on lines
1541 and 1532.

https://breakpad.appspot.com/606002/diff/2001/src/processor/minidump.cc#newco...
src/processor/minidump.cc:1546: descriptor_from_memory_list_ =
*(region->descriptor_);
The *ing and &ing here is a little circuitous. Why do you even need a member
variable?

https://breakpad.appspot.com/606002/diff/2001/src/processor/minidump_processo...
File src/processor/minidump_processor.cc (right):

https://breakpad.appspot.com/606002/diff/2001/src/processor/minidump_processo...
src/processor/minidump_processor.cc:222:
thread->UseMemoryFromMinidumpMemoryList(memory_list)) {
I think that all of your logic actually belongs here and not in minidump.cc.
(Well, you can keep the check for RVA 0 in minidump.cc.)

minidump.cc and the Minidump* classes are the reading interface to minidump
files. The minidump file does contain a bogus memory region for the thread, and
the interface can detect that and make it appear as though it doesn’t contain a
memory region at all. That’s fine. But why would you need to involve minidump.cc
in finding a good memory region by using the memory list? You don’t need the
Minidump* interface for that, you can handle it entirely right here in the
minidump processor.

[Ivan Penkov]

Great comments!
Done.

https://breakpad.appspot.com/606002/diff/2001/src/google_breakpad/processor/m...
File src/google_breakpad/processor/minidump.h (right):

https://breakpad.appspot.com/606002/diff/2001/src/google_breakpad/processor/m...
src/google_breakpad/processor/minidump.h:334: // containing the thread stack
memory in the MinidumpMemoryList.
On 2013/06/26 22:38:38, Mark Mentovai wrote:
> This explains the problem but it doesn’t explain how to use this function.
> Specifically, what’s the argument and what’s the return value?
> 
> Why is it a bool? It might be handier to return the MinidumpMemoryRegion*

This method is now gone.

https://breakpad.appspot.com/606002/diff/2001/src/google_breakpad/processor/m...
src/google_breakpad/processor/minidump.h:335: virtual bool
UseMemoryFromMinidumpMemoryList(MinidumpMemoryList *memory_list);
On 2013/06/26 22:38:38, Mark Mentovai wrote:
> This doesn’t need to be virtual, does it?

The only reason for the virtual was testing (gmock).

https://breakpad.appspot.com/606002/diff/2001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://breakpad.appspot.com/606002/diff/2001/src/processor/minidump.cc#newco...
src/processor/minidump.cc:1544: if (region != NULL) {
On 2013/06/26 22:38:38, Mark Mentovai wrote:
> This file rarely uses the |!= NULL| style, it would normally just use |if
> (region) {|. In fact, you used that style in this function already, on lines
> 1541 and 1532.

Good point.  This code was moved to minidump.cc and the styling issue was fixed.

https://breakpad.appspot.com/606002/diff/2001/src/processor/minidump.cc#newco...
src/processor/minidump.cc:1546: descriptor_from_memory_list_ =
*(region->descriptor_);
On 2013/06/26 22:38:38, Mark Mentovai wrote:
> The *ing and &ing here is a little circuitous. Why do you even need a member
> variable?

This code is now gone, but the reason was to make a copy of  the descriptor that
comes with the memory region from the MinidumpMemoryList so that it doesn't
create unnecessary lifetime dependency.

https://breakpad.appspot.com/606002/diff/2001/src/processor/minidump_processo...
File src/processor/minidump_processor.cc (right):

https://breakpad.appspot.com/606002/diff/2001/src/processor/minidump_processo...
src/processor/minidump_processor.cc:222:
thread->UseMemoryFromMinidumpMemoryList(memory_list)) {
On 2013/06/26 22:38:38, Mark Mentovai wrote:
> I think that all of your logic actually belongs here and not in minidump.cc.
> (Well, you can keep the check for RVA 0 in minidump.cc.)
> 
> minidump.cc and the Minidump* classes are the reading interface to minidump
> files. The minidump file does contain a bogus memory region for the thread,
and
> the interface can detect that and make it appear as though it doesn’t contain
a
> memory region at all. That’s fine. But why would you need to involve
minidump.cc
> in finding a good memory region by using the memory list? You don’t need the
> Minidump* interface for that, you can handle it entirely right here in the
> minidump processor.

Great comment.  I'm not sure why I was trying to fix up the memory descriptor
inside MinidumpThread.  In perspective, this seems obvious.  Thank you.

[Mark Mentovai]

That’s much better! LGTM.

https://breakpad.appspot.com/606002/diff/5003/src/google_breakpad/processor/m...
File src/google_breakpad/processor/minidump.h (right):

https://breakpad.appspot.com/606002/diff/5003/src/google_breakpad/processor/m...
src/google_breakpad/processor/minidump.h:331: // the thread memory cannot be
read  and GetMemory returns NULL.
There’s an extra space in there.

https://breakpad.appspot.com/606002/diff/5003/src/processor/minidump_processo...
File src/processor/minidump_processor.cc (right):

https://breakpad.appspot.com/606002/diff/5003/src/processor/minidump_processo...
src/processor/minidump_processor.cc:221: if (!thread_memory) {
Move the check for memory_list up here, since there’s no point in doing any of
this if there is no memory_list.

[Ivan Penkov]

Done

https://breakpad.appspot.com/606002/diff/5003/src/google_breakpad/processor/m...
File src/google_breakpad/processor/minidump.h (right):

https://breakpad.appspot.com/606002/diff/5003/src/google_breakpad/processor/m...
src/google_breakpad/processor/minidump.h:331: // the thread memory cannot be
read  and GetMemory returns NULL.
On 2013/06/27 13:25:31, Mark Mentovai wrote:
> There’s an extra space in there.

Done.

https://breakpad.appspot.com/606002/diff/5003/src/processor/minidump_processo...
File src/processor/minidump_processor.cc (right):

https://breakpad.appspot.com/606002/diff/5003/src/processor/minidump_processo...
src/processor/minidump_processor.cc:221: if (!thread_memory) {
On 2013/06/27 13:25:31, Mark Mentovai wrote:
> Move the check for memory_list up here, since there’s no point in doing any of
> this if there is no memory_list.

Good point.  Done.

[Mark Mentovai]

LGTM