win: Save contents of PEB to minidump

win: Save contents of PEB to minidump to start making !peb work

This makes the basics of !peb work in windbg, however, pointed-to things
are not yet retrieved. For full functionality, a variety of pointers in
the PEB also needs to be walked and captured.

e.g.

Previously:

0:000> .ecxr
eax=00000007 ebx=7e383000 ecx=c3f9a943 edx=00000000 esi=006d62d0 edi=003c9280
eip=00384828 esp=005bf634 ebp=005bf638 iopl=0         nv up ei pl zr na pe nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010246
crashy_program!crashpad::`anonymous namespace'::SomeCrashyFunction+0x28:
00384828 c7002a000000    mov     dword ptr [eax],2Ah  ds:002b:00000007=????????
0:000> !peb
PEB at 7e383000
error 1 InitTypeRead( nt!_PEB at 7e383000)...


Now:

0:000> .ecxr
eax=00000007 ebx=7f958000 ecx=02102f4d edx=00000000 esi=00e162d0 edi=01389280
eip=01344828 esp=00c2fb64 ebp=00c2fb68 iopl=0         nv up ei pl zr na pe nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010246
crashy_program!crashpad::`anonymous namespace'::SomeCrashyFunction+0x28:
01344828 c7002a000000    mov     dword ptr [eax],2Ah  ds:002b:00000007=????????
0:000> !peb
PEB at 7f958000
    InheritedAddressSpace:    No
    ReadImageFileExecOptions: No
    BeingDebugged:            No
    ImageBaseAddress:         01340000
    Ldr                       77ec8b40
    *** unable to read Ldr table at 77ec8b40
    SubSystemData:     00000000
    ProcessHeap:       00e10000
    ProcessParameters: 00e114e0
    CurrentDirectory:  '< Name not readable >'
    WindowTitle:  '< Name not readable >'
    ImageFile:    '< Name not readable >'
    CommandLine:  '< Name not readable >'
    DllPath:      '< Name not readable >'
    Environment:  00000000
       Unable to read Environment string.


R=mark@chromium.org
BUG=crashpad:46
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/0758dbde9a39fcc965d18f1791082daa6add40aa

[Mark Mentovai, 2015-09-23 21:40:50]

I’m curious about these:

    CurrentDirectory:  '< Name not readable >'

Are the addresses for those things just beyond the PEB itself, or can they be
anywhere? If they’re just beyond the PEB, we may want to fudge the PEB size to
include them.

On the other hand, if they can be anywhere and we want to capture them, we may
want to pursue a more generic “extra memory regions to capture in a minidump”
sort of thing, and we might want the PEB to be one such region rather than
having it be its own first-class thing on ProcessSnapshot that just happens to
be a memory region.

https://codereview.chromium.org/1364053002/diff/20001/minidump/minidump_file_...
File minidump/minidump_file_writer.cc (right):

https://codereview.chromium.org/1364053002/diff/20001/minidump/minidump_file_...
minidump/minidump_file_writer.cc:84: std::vector<const MemorySnapshot*>
peb_vector;
Use the (1, peb) constructor and skip the push_back.

https://codereview.chromium.org/1364053002/diff/20001/minidump/minidump_file_...
File minidump/minidump_file_writer_test.cc (right):

https://codereview.chromium.org/1364053002/diff/20001/minidump/minidump_file_...
minidump/minidump_file_writer_test.cc:260:
process_snapshot.SetPeb(peb_snapshot.Pass());
You never test that this had any effect on the output. Do you want to examine
the memory stream to make sure that it’s got this?

https://codereview.chromium.org/1364053002/diff/20001/snapshot/process_snapsh...
File snapshot/process_snapshot.h (right):

https://codereview.chromium.org/1364053002/diff/20001/snapshot/process_snapsh...
snapshot/process_snapshot.h:141: //!
Say that this is only a sensible thing on Windows.

https://codereview.chromium.org/1364053002/diff/20001/snapshot/win/process_re...
File snapshot/win/process_reader_win.h (right):

https://codereview.chromium.org/1364053002/diff/20001/snapshot/win/process_re...
snapshot/win/process_reader_win.h:116: const ProcessInfo& process_info() const {
Move this out-of-line (and rename ProcessInfo()) since it’s becoming less simple
now.

https://codereview.chromium.org/1364053002/diff/20001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/1364053002/diff/20001/util/win/process_info.c...
util/win/process_info.cc:266: peb_address_(),
_(0), next line too.

https://codereview.chromium.org/1364053002/diff/20001/util/win/process_info.h
File util/win/process_info.h (right):

https://codereview.chromium.org/1364053002/diff/20001/util/win/process_info.h...
util/win/process_info.h:83: 
Missing //!

[Mark Mentovai, 2015-09-23 21:41:51]

This tells me that at least some of this stuff is pretty discontiguous:

PEB at 7f958000
[…]
    Ldr                       77ec8b40
    *** unable to read Ldr table at 77ec8b40

[scottmg, 2015-09-23 22:15:08]

On 2015/09/23 21:40:50, Mark Mentovai - August is over wrote:
> I’m curious about these:
> 
>     CurrentDirectory:  '< Name not readable >'
> 
> Are the addresses for those things just beyond the PEB itself, or can they be
> anywhere? If they’re just beyond the PEB, we may want to fudge the PEB size to
> include them.
> 
> On the other hand, if they can be anywhere and we want to capture them, we may
> want to pursue a more generic “extra memory regions to capture in a minidump”
> sort of thing, and we might want the PEB to be one such region rather than
> having it be its own first-class thing on ProcessSnapshot that just happens to
> be a memory region.

It looks like it points all over the place, though much of it seems to be at the
beginning of the main process heap. Here's a full one from a running process:

0:000> !peb
PEB at 7f7ce000
    InheritedAddressSpace:    No
    ReadImageFileExecOptions: No
    BeingDebugged:            Yes
    ImageBaseAddress:         00c90000
    Ldr                       77ec8b40
    Ldr.Initialized:          Yes
    Ldr.InInitializationOrderModuleList: 01473660 . 01474928
    Ldr.InLoadOrderModuleList:           01473750 . 01474918
    Ldr.InMemoryOrderModuleList:         01473758 . 01474920
            Base TimeStamp                     Module
          c90000 56031848 Sep 23 14:23:20 2015
D:\src\crashpad\crashpad\out\Debug\crashy_program.exe
        77dc0000 55c599e1 Aug 07 22:55:45 2015 C:\Windows\SYSTEM32\ntdll.dll
        758e0000 559f3b21 Jul 09 20:25:21 2015 C:\Windows\SYSTEM32\KERNEL32.DLL
        76850000 559f3b2a Jul 09 20:25:30 2015
C:\Windows\SYSTEM32\KERNELBASE.dll
        6f2b0000 559f3b08 Jul 09 20:24:56 2015 C:\Windows\system32\apphelp.dll
    SubSystemData:     00000000
    ProcessHeap:       01470000
    ProcessParameters: 01471a80
    CurrentDirectory:  'd:\src\crashpad\crashpad\'
    WindowTitle:  'D:\src\crashpad\crashpad\out\Debug\crashy_program.exe'
    ImageFile:    'D:\src\crashpad\crashpad\out\Debug\crashy_program.exe'
    CommandLine:  'D:\src\crashpad\crashpad\out\Debug\crashy_program.exe'
    DllPath:      '< Name not readable >'
    Environment:  014705e8
        =D:=d:\src\crashpad\crashpad
        =ExitCode=00000000
        ALLUSERSPROFILE=C:\ProgramData
...


It does seem like we'll want to capture a variety of miscellaneous regions for
bug #46. I started on the TEB(s) too
(https://codereview.chromium.org/1364803004/) and it got a bit cumbersome. We'll
also want to capture near EIP/RIP, maybe the first page  of each module, some
some memory around registers that look like they contain pointer values, and so
on.

Did you have any design in mind for doing that? I'm not sure of a good place for
it to live.



> 
>
https://codereview.chromium.org/1364053002/diff/20001/minidump/minidump_file_...
> File minidump/minidump_file_writer.cc (right):
> 
>
https://codereview.chromium.org/1364053002/diff/20001/minidump/minidump_file_...
> minidump/minidump_file_writer.cc:84: std::vector<const MemorySnapshot*>
> peb_vector;
> Use the (1, peb) constructor and skip the push_back.
> 
>
https://codereview.chromium.org/1364053002/diff/20001/minidump/minidump_file_...
> File minidump/minidump_file_writer_test.cc (right):
> 
>
https://codereview.chromium.org/1364053002/diff/20001/minidump/minidump_file_...
> minidump/minidump_file_writer_test.cc:260:
> process_snapshot.SetPeb(peb_snapshot.Pass());
> You never test that this had any effect on the output. Do you want to examine
> the memory stream to make sure that it’s got this?
> 
>
https://codereview.chromium.org/1364053002/diff/20001/snapshot/process_snapsh...
> File snapshot/process_snapshot.h (right):
> 
>
https://codereview.chromium.org/1364053002/diff/20001/snapshot/process_snapsh...
> snapshot/process_snapshot.h:141: //!
> Say that this is only a sensible thing on Windows.
> 
>
https://codereview.chromium.org/1364053002/diff/20001/snapshot/win/process_re...
> File snapshot/win/process_reader_win.h (right):
> 
>
https://codereview.chromium.org/1364053002/diff/20001/snapshot/win/process_re...
> snapshot/win/process_reader_win.h:116: const ProcessInfo& process_info() const
{
> Move this out-of-line (and rename ProcessInfo()) since it’s becoming less
simple
> now.
> 
> https://codereview.chromium.org/1364053002/diff/20001/util/win/process_info.cc
> File util/win/process_info.cc (right):
> 
>
https://codereview.chromium.org/1364053002/diff/20001/util/win/process_info.c...
> util/win/process_info.cc:266: peb_address_(),
> _(0), next line too.
> 
> https://codereview.chromium.org/1364053002/diff/20001/util/win/process_info.h
> File util/win/process_info.h (right):
> 
>
https://codereview.chromium.org/1364053002/diff/20001/util/win/process_info.h...
> util/win/process_info.h:83: 
> Missing //!

[Mark Mentovai, 2015-09-24 01:57:02]

Perhaps both ProcessSnapshot and ThreadSnapshot (and ModuleSnapshot, if you want
the first few pages of each module, or its headers, or something) should expose
a vector<MemorySnapshot*> ExtraMemory() documented as being things that are
important for a “mini” dump, short of carrying the process’ entire memory image.

We may need to be careful at some point to avoid having overlapping memory
regions, but that may be something better dealt with in the minidump writer?
Then again, if there were overlapping regions, we’d probably want to coalesce
them, which would be best dealt with in the snapshot.

I hadn’t anticipated needing this for TEB because I assumed that the TEB would
be just above the stack base in memory, and we could just capture it along with
the stack. That’s where TEB-equivalent stuff like TLS slots live on Mac. But
maybe that’s not the case on Windows.

For that matter, some of the PEB-equivalent stuff like the command line and
environment live just above the main thread’s stack bottom on Mac, and we pick
them up along with the main thread’s stack.

And I’m a little shocked that the system trusts user space with
CurrentDirectory.

[scottmg, 2015-09-24 20:00:10]

Thanks, ExtraMemory() seems like a good idea. I still only included the root of
the PEB for now, and it's only on ProcessSnapshot for now.

https://codereview.chromium.org/1364053002/diff/20001/minidump/minidump_file_...
File minidump/minidump_file_writer.cc (right):

https://codereview.chromium.org/1364053002/diff/20001/minidump/minidump_file_...
minidump/minidump_file_writer.cc:84: std::vector<const MemorySnapshot*>
peb_vector;
On 2015/09/23 21:40:50, Mark Mentovai - August is over wrote:
> Use the (1, peb) constructor and skip the push_back.

Gone now that ExtraMemory() directly returns a vector.

https://codereview.chromium.org/1364053002/diff/20001/minidump/minidump_file_...
File minidump/minidump_file_writer_test.cc (right):

https://codereview.chromium.org/1364053002/diff/20001/minidump/minidump_file_...
minidump/minidump_file_writer_test.cc:260:
process_snapshot.SetPeb(peb_snapshot.Pass());
On 2015/09/23 21:40:50, Mark Mentovai - August is over wrote:
> You never test that this had any effect on the output. Do you want to examine
> the memory stream to make sure that it’s got this?

Oops, done.

https://codereview.chromium.org/1364053002/diff/20001/snapshot/process_snapsh...
File snapshot/process_snapshot.h (right):

https://codereview.chromium.org/1364053002/diff/20001/snapshot/process_snapsh...
snapshot/process_snapshot.h:141: //!
On 2015/09/23 21:40:50, Mark Mentovai - August is over wrote:
> Say that this is only a sensible thing on Windows.

Gone.

https://codereview.chromium.org/1364053002/diff/20001/snapshot/win/process_re...
File snapshot/win/process_reader_win.h (right):

https://codereview.chromium.org/1364053002/diff/20001/snapshot/win/process_re...
snapshot/win/process_reader_win.h:116: const ProcessInfo& process_info() const {
On 2015/09/23 21:40:50, Mark Mentovai - August is over wrote:
> Move this out-of-line (and rename ProcessInfo()) since it’s becoming less
simple
> now.

Done, unfortunately can't be called ProcessInfo() because that's the type name.

https://codereview.chromium.org/1364053002/diff/20001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/1364053002/diff/20001/util/win/process_info.c...
util/win/process_info.cc:266: peb_address_(),
On 2015/09/23 21:40:50, Mark Mentovai - August is over wrote:
> _(0), next line too.

Done.

https://codereview.chromium.org/1364053002/diff/20001/util/win/process_info.h
File util/win/process_info.h (right):

https://codereview.chromium.org/1364053002/diff/20001/util/win/process_info.h...
util/win/process_info.h:83: 
On 2015/09/23 21:40:50, Mark Mentovai - August is over wrote:
> Missing //!

Done.

[Mark Mentovai, 2015-09-25 17:05:35]

LGTM

https://codereview.chromium.org/1364053002/diff/100001/minidump/minidump_file...
File minidump/minidump_file_writer.cc (right):

https://codereview.chromium.org/1364053002/diff/100001/minidump/minidump_file...
minidump/minidump_file_writer.cc:82:
memory_list->AddFromSnapshot(process_snapshot->ExtraMemory());
Was there a significance to this position? Just to get it right after all of the
thread stacks? No code change or code comment necessary, just curious.

https://codereview.chromium.org/1364053002/diff/100001/snapshot/minidump/proc...
File snapshot/minidump/process_snapshot_minidump.h (right):

https://codereview.chromium.org/1364053002/diff/100001/snapshot/minidump/proc...
snapshot/minidump/process_snapshot_minidump.h:41: class MemorySnapshot;
The other things like ExceptionSnapshot that are needed for pointers in this
file are #included instead of forward-declared.

https://codereview.chromium.org/1364053002/diff/100001/snapshot/process_snaps...
File snapshot/process_snapshot.h (right):

https://codereview.chromium.org/1364053002/diff/100001/snapshot/process_snaps...
snapshot/process_snapshot.h:168: //! \return An std::vector<const
MemorySnapshot*> that will be included in the
A, not an. Or for the comment, English is OK, so “A vector of MemorySnapshot
objects that will…”

https://codereview.chromium.org/1364053002/diff/100001/snapshot/process_snaps...
snapshot/process_snapshot.h:169: //!     crash dump. Ownership of the  pointers
in the vector are not
Two spaces between the and pointers.

https://codereview.chromium.org/1364053002/diff/100001/snapshot/process_snaps...
snapshot/process_snapshot.h:169: //!     crash dump. Ownership of the  pointers
in the vector are not
is not transferred, there’s only one ownership. Or borrow from the comments for
other similar things like Threads() and Modules().

https://codereview.chromium.org/1364053002/diff/100001/snapshot/test/test_pro...
File snapshot/test/test_process_snapshot.h (right):

https://codereview.chromium.org/1364053002/diff/100001/snapshot/test/test_pro...
snapshot/test/test_process_snapshot.h:79: void SetPeb(scoped_ptr<MemorySnapshot>
peb) { peb_ = peb.Pass(); }
AddExtraMemory() instead, now. Like AddThread() and AddModule().

https://codereview.chromium.org/1364053002/diff/100001/snapshot/test/test_pro...
snapshot/test/test_process_snapshot.h:79: void SetPeb(scoped_ptr<MemorySnapshot>
peb) { peb_ = peb.Pass(); }
Since the getters are declared in the order (thread, module, exception,
extra-memory), the setters should follow.

https://codereview.chromium.org/1364053002/diff/100001/snapshot/test/test_pro...
snapshot/test/test_process_snapshot.h:133: scoped_ptr<MemorySnapshot> peb_;
Likewise.

[scottmg, 2015-09-25 17:26:29]

Patchset #7 (id:120001) has been deleted

[scottmg, 2015-09-25 17:29:31]

https://codereview.chromium.org/1364053002/diff/100001/minidump/minidump_file...
File minidump/minidump_file_writer.cc (right):

https://codereview.chromium.org/1364053002/diff/100001/minidump/minidump_file...
minidump/minidump_file_writer.cc:82:
memory_list->AddFromSnapshot(process_snapshot->ExtraMemory());
On 2015/09/25 17:05:34, Mark Mentovai - August is over wrote:
> Was there a significance to this position? Just to get it right after all of
the
> thread stacks? No code change or code comment necessary, just curious.

Not really, just the first place after memory_list was created that looked
reasonable. It'd be more sensible right after line 96 though.

https://codereview.chromium.org/1364053002/diff/100001/snapshot/minidump/proc...
File snapshot/minidump/process_snapshot_minidump.h (right):

https://codereview.chromium.org/1364053002/diff/100001/snapshot/minidump/proc...
snapshot/minidump/process_snapshot_minidump.h:41: class MemorySnapshot;
On 2015/09/25 17:05:34, Mark Mentovai - August is over wrote:
> The other things like ExceptionSnapshot that are needed for pointers in this
> file are #included instead of forward-declared.

Done.

https://codereview.chromium.org/1364053002/diff/100001/snapshot/process_snaps...
File snapshot/process_snapshot.h (right):

https://codereview.chromium.org/1364053002/diff/100001/snapshot/process_snaps...
snapshot/process_snapshot.h:168: //! \return An std::vector<const
MemorySnapshot*> that will be included in the
On 2015/09/25 17:05:34, Mark Mentovai - August is over wrote:
> A, not an. Or for the comment, English is OK, so “A vector of MemorySnapshot
> objects that will…”

Really? :) I say "An ess tee dee vector..." when I'm reading that, so "A" sounds
awkward. I'll judo that with just "A vector..." though.

https://codereview.chromium.org/1364053002/diff/100001/snapshot/process_snaps...
snapshot/process_snapshot.h:169: //!     crash dump. Ownership of the  pointers
in the vector are not
On 2015/09/25 17:05:34, Mark Mentovai - August is over wrote:
> is not transferred, there’s only one ownership. Or borrow from the comments
for
> other similar things like Threads() and Modules().

Oops, done.

https://codereview.chromium.org/1364053002/diff/100001/snapshot/process_snaps...
snapshot/process_snapshot.h:169: //!     crash dump. Ownership of the  pointers
in the vector are not
On 2015/09/25 17:05:35, Mark Mentovai - August is over wrote:
> Two spaces between the and pointers.

Done.

https://codereview.chromium.org/1364053002/diff/100001/snapshot/test/test_pro...
File snapshot/test/test_process_snapshot.h (right):

https://codereview.chromium.org/1364053002/diff/100001/snapshot/test/test_pro...
snapshot/test/test_process_snapshot.h:79: void SetPeb(scoped_ptr<MemorySnapshot>
peb) { peb_ = peb.Pass(); }
On 2015/09/25 17:05:35, Mark Mentovai - August is over wrote:
> AddExtraMemory() instead, now. Like AddThread() and AddModule().

Done.

https://codereview.chromium.org/1364053002/diff/100001/snapshot/test/test_pro...
snapshot/test/test_process_snapshot.h:79: void SetPeb(scoped_ptr<MemorySnapshot>
peb) { peb_ = peb.Pass(); }
On 2015/09/25 17:05:35, Mark Mentovai - August is over wrote:
> Since the getters are declared in the order (thread, module, exception,
> extra-memory), the setters should follow.

Done.

https://codereview.chromium.org/1364053002/diff/100001/snapshot/test/test_pro...
snapshot/test/test_process_snapshot.h:133: scoped_ptr<MemorySnapshot> peb_;
On 2015/09/25 17:05:35, Mark Mentovai - August is over wrote:
> Likewise.

Done.

[scottmg, 2015-09-25 17:31:09]

Committed patchset #7 (id:140001) manually as
0758dbde9a39fcc965d18f1791082daa6add40aa (presubmit successful).

[Mark Mentovai, 2015-09-25 17:32:32]

https://codereview.chromium.org/1364053002/diff/100001/snapshot/process_snaps...
File snapshot/process_snapshot.h (right):

https://codereview.chromium.org/1364053002/diff/100001/snapshot/process_snaps...
snapshot/process_snapshot.h:168: //! \return An std::vector<const
MemorySnapshot*> that will be included in the
scottmg wrote:
> Really? :) I say "An ess tee dee vector..." when I'm reading that, so "A"
sounds
> awkward. I'll judo that with just "A vector..." though.

Heh. “ess tee dee” has a different meaning to me when you spell it out like
that. I say “standard.”

https://codereview.chromium.org/1364053002/diff/140001/minidump/minidump_file...
File minidump/minidump_file_writer.cc (right):

https://codereview.chromium.org/1364053002/diff/140001/minidump/minidump_file...
minidump/minidump_file_writer.cc:96:
memory_list->AddFromSnapshot(process_snapshot->ExtraMemory());
It’s OK to do this after the crashpad_info stuff, but right here, it’s in the
middle of the crashpad_info.

[scottmg, 2015-09-25 19:20:34]

https://codereview.chromium.org/1364053002/diff/140001/minidump/minidump_file...
File minidump/minidump_file_writer.cc (right):

https://codereview.chromium.org/1364053002/diff/140001/minidump/minidump_file...
minidump/minidump_file_writer.cc:96:
memory_list->AddFromSnapshot(process_snapshot->ExtraMemory());
On 2015/09/25 17:32:32, Mark Mentovai - August is over wrote:
> It’s OK to do this after the crashpad_info stuff, but right here, it’s in the
> middle of the crashpad_info.

Hmm, you're right. OK, at the end it is:
https://codereview.chromium.org/1369823002.