win: Add more memory regions to gathering of PEB

win: Add more memory regions to gathering of PEB

Previously:

0:000> !peb
PEB at 7f374000
    InheritedAddressSpace:    No
    ReadImageFileExecOptions: No
    BeingDebugged:            No
    ImageBaseAddress:         01380000
    Ldr                       77ec8b40
    *** unable to read Ldr table at 77ec8b40
    SubSystemData:     00000000
    ProcessHeap:       00740000
    ProcessParameters: 007414e0
    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.

Now:

0:000> !peb
PEB at 7f494000
    InheritedAddressSpace:    No
    ReadImageFileExecOptions: No
    BeingDebugged:            No
    ImageBaseAddress:         00ef0000
    Ldr                       77ec8b40
    Ldr.Initialized:          Yes
    Ldr.InInitializationOrderModuleList: 01042b68 . 01043c68
    Ldr.InLoadOrderModuleList:           01042c38 . 01043c58
    Ldr.InMemoryOrderModuleList:         01042c40 . 01043c60
            Base TimeStamp                     Module
          ef0000 5609bd17 Sep 28 15:20:07 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
    SubSystemData:     00000000
    ProcessHeap:       01040000
    ProcessParameters: 01041520
    CurrentDirectory:  'd:\src\crashpad\crashpad\'
    WindowTitle:  'out\debug\crashy_program.exe  \\.\pipe\stuff'
    ImageFile:    'd:\src\crashpad\crashpad\out\debug\crashy_program.exe'
    CommandLine:  'out\debug\crashy_program.exe  \\.\pipe\stuff'
    DllPath:      '< Name not readable >'
    Environment:  010405c8
        =D:=d:\src\crashpad\crashpad
        =ExitCode=C0000005
        ALLUSERSPROFILE=C:\ProgramData
        APPDATA=C:\Users\scott\AppData\Roaming
        CommonProgramFiles=C:\Program Files (x86)\Common Files
        CommonProgramFiles(x86)=C:\Program Files (x86)\Common Files
...


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

[Mark Mentovai, 2015-09-25 21:10:17]

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:245:
CreateMemorySnapshotForUNICODE_STRING(process_parameters.DllPath));
The “now” section of the CL description seems to indicate that this isn’t
working. Or was this field just empty in your test?

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:255: kMaxEnvironmentBlockSize));
It might be worth trying to work out the actual size of the environment block so
that we don’t over-report, but you can save that for a TODO if it’s rough to
work out.

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:260: WinVMAddress size) {
If size is 0, it’s probably worth not creating anything. Instead, return
nullptr, and have the caller not add anything to the peb_memory_ vector.

That may mean that it’s easier for this return void and take the vector as an
argument, appending to it if size is nonzero, instead of returning something for
the caller to append (or not).

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win.h (right):

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.h:144: template <class Traits>
Blank line before this.

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.h:147: return CreateMemorySnapshot(us.Buffer,
us.MaximumLength);
Length instead of MaximumLength? We don’t need to capture what’s not currently
valid.

Probably it should be std::min(us.Length + sizeof(us.Buffer[0]),
us.MaximumLength), to ensure that we’re capturing the memory from the trailing
NUL character (if there is one) without overrunning the buffer.

I can’t think of a good reason to inline this in the header, it’s just more of
an opportunity to have to rebuild .cc files that depend on
process_snapshot_win.h when this implementation changes.

[scottmg, 2015-09-25 22:52:25]

Thanks

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:245:
CreateMemorySnapshotForUNICODE_STRING(process_parameters.DllPath));
On 2015/09/25 21:10:16, Mark Mentovai - August is over wrote:
> The “now” section of the CL description seems to indicate that this isn’t
> working. Or was this field just empty in your test?

Yes, it's often null in a live process too. It's the search path that the loader
uses to locate dlls, but if it's not overridden then (I guess) it doesn't get
filled in here.

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:255: kMaxEnvironmentBlockSize));
On 2015/09/25 21:10:16, Mark Mentovai - August is over wrote:
> It might be worth trying to work out the actual size of the environment block
so
> that we don’t over-report, but you can save that for a TODO if it’s rough to
> work out.

I don't know of any way other than probing the target process for \0, which is a
bit tedious (and I'm not sure if it's 100% correct either). Added a TODO as
*something* must know how big that block is.

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:260: WinVMAddress size) {
On 2015/09/25 21:10:16, Mark Mentovai - August is over wrote:
> If size is 0, it’s probably worth not creating anything. Instead, return
> nullptr, and have the caller not add anything to the peb_memory_ vector.
> 
> That may mean that it’s easier for this return void and take the vector as an
> argument, appending to it if size is nonzero, instead of returning something
for
> the caller to append (or not).

Done.

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win.h (right):

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.h:144: template <class Traits>
On 2015/09/25 21:10:16, Mark Mentovai - August is over wrote:
> Blank line before this.

Done.

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.h:147: return CreateMemorySnapshot(us.Buffer,
us.MaximumLength);
On 2015/09/25 21:10:17, Mark Mentovai - August is over wrote:
> Length instead of MaximumLength? We don’t need to capture what’s not currently
> valid.
> 
> Probably it should be std::min(us.Length + sizeof(us.Buffer[0]),
> us.MaximumLength), to ensure that we’re capturing the memory from the trailing
> NUL character (if there is one) without overrunning the buffer.
> 
> I can’t think of a good reason to inline this in the header, it’s just more of
> an opportunity to have to rebuild .cc files that depend on
> process_snapshot_win.h when this implementation changes.

Done. (Can't use us.Buffer[0] because it's Traits::Pointer)

[Mark Mentovai, 2015-09-25 23:11:59]

LGTM.

This is great! I don’t know if MiniDumpWriteDump() even collects all of this
stuff for MiniDumpNormal-type dumps. I don’t think it did nine years ago, but
dbghelp has probably improved since then.

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:255: kMaxEnvironmentBlockSize));
scottmg wrote:
> I don't know of any way other than probing the target process for \0, which is
a
> bit tedious (and I'm not sure if it's 100% correct either). Added a TODO as
> *something* must know how big that block is.

Well, if windbg’s able to work out where to stop printing environment variables,
there’s got to be some way to do it. I think you just look for a double-NUL.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682653(v=vs.85).aspx

https://codereview.chromium.org/1360863006/diff/60001/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/60001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:255: const int kMaxEnvironmentBlockSize =
32768;
32,768 bytes or 32,768 characters? All of the references seem to say characters.

https://codereview.chromium.org/1360863006/diff/60001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:278: std::min(static_cast<USHORT>(us.Length
+ sizeof(wchar_t)),
Y’know, on the second thought, the documentation for UNICODE_STRING actually
says that it may not be NUL-terminated at all. So maybe this should just be
us.Length without the std::min or the extra wchar_t.

If you do that, is windbg OK with reading empty strings that don’t have any
memory or NULs to point to?

[scottmg, 2015-09-26 00:07:38]

I added the environment end-probe, which made me realize that I was getting
lucky on the Ldr subpointers (Ldr.InMemoryOrderModuleList, etc.).

windbg gets sad when it can't find that memory and then won't dump the rest of
the PEB. So I'll pick this up later and have it walk those lists too (you could
look at the environment probe now if you like).

There's going to be quite a few regions when walking the Ldr, which might mean
we want to do coalescing at some level, but it might also mean that we really
want to block-capture some larger region rather than capture 95% of it in little
blocks. Will need to investigate what the dumps actually look like once we get
everything working on that front.

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/40001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:255: kMaxEnvironmentBlockSize));
On 2015/09/25 23:11:59, Mark Mentovai - August is over wrote:
> scottmg wrote:
> > I don't know of any way other than probing the target process for \0, which
is
> a
> > bit tedious (and I'm not sure if it's 100% correct either). Added a TODO as
> > *something* must know how big that block is.
> 
> Well, if windbg’s able to work out where to stop printing environment
variables,
> there’s got to be some way to do it. I think you just look for a double-NUL.
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682653(v=vs.85).aspx

Yeah, that was my thinking too. But then I was worried that we might lose some
interesting information if the environment block was the thing that had been
futzed with. But maybe that's useful too, because you'd see what the process
would probably be able to retrieve.

... ok, switched to grabbing 32k here to find the \0\0 and then only storing up
to that point.

https://codereview.chromium.org/1360863006/diff/60001/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/60001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:255: const int kMaxEnvironmentBlockSize =
32768;
On 2015/09/25 23:11:59, Mark Mentovai - August is over wrote:
> 32,768 bytes or 32,768 characters? All of the references seem to say
characters.

You're right, characters. (That would make more sense as a limit too for
wchar_ts in a short.)

https://codereview.chromium.org/1360863006/diff/60001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:278: std::min(static_cast<USHORT>(us.Length
+ sizeof(wchar_t)),
On 2015/09/25 23:11:59, Mark Mentovai - August is over wrote:
> Y’know, on the second thought, the documentation for UNICODE_STRING actually
> says that it may not be NUL-terminated at all. So maybe this should just be
> us.Length without the std::min or the extra wchar_t.
> 
> If you do that, is windbg OK with reading empty strings that don’t have any
> memory or NULs to point to?

It seems fine.

[Mark Mentovai, 2015-09-26 01:46:49]

We’ll definitely want to figure out coalescing. Something’s going to get upset
if there are overlapping memory ranges in the minidump. The Breakpad processor
starts throwing things out when it encounters that case.

https://codereview.chromium.org/1360863006/diff/80001/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/80001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:286: env_block_size * sizeof(wchar_t),
sizeof(env_block[0])

https://codereview.chromium.org/1360863006/diff/80001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:290: // We could be out of range of the
process so the read might
TODO to revisit this with a more generic “read as much as possible” kind of
thing that knows about the memory map or just tries to read in a loop kind of
like you have here? This may be useful elsewhere (probably operating on bytes
instead of wchar_ts, though).

For now, you can make sure that your read attempts try to get as much as
possible by reducing the trial block size on each iteration so that its end is
aligned with the end of a page.

  end_of_environment_block = start_of_environment_block + env_block_size *
sizeof(env_block[0]);

  // Subtract 1 and truncate down to the nearest page. If it was page-aligned to
  // begin with, the subtraction assures that the new end will be lower than the
  // old end.
  end_of_environment_block = (end_of_environment_block - 1) & ~(page_size - 1);

  // Truncation seems OK if this can’t be divided evenly, because it’ll just
  // throw away an incomplete code unit.
  env_block_size = (end_of_environment_block - start_of_environment_block) /
sizeof(env_block[0]);

You could write this whole thing in a single line, but it’d be harder to follow.

https://codereview.chromium.org/1360863006/diff/80001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:295: std::wstring look_in(&env_block[0],
env_block_size);
Since you’re just copying the whole thing into look_in, probably env_block
should have been the wstring you read into to begin with.

[Mark Mentovai, 2015-09-26 01:47:21]

I don’t mean to figure out coalescing now, just that we’ll need it soon as we
continue down this ExtraMemory() path.

[scottmg, 2015-09-28 22:38:09]

Needs another look after adding the LDR reading.

https://codereview.chromium.org/1360863006/diff/80001/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/80001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:286: env_block_size * sizeof(wchar_t),
On 2015/09/26 01:46:48, Mark Mentovai - August is over wrote:
> sizeof(env_block[0])

Done.

https://codereview.chromium.org/1360863006/diff/80001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:290: // We could be out of range of the
process so the read might
On 2015/09/26 01:46:48, Mark Mentovai - August is over wrote:
> TODO to revisit this with a more generic “read as much as possible” kind of
> thing that knows about the memory map or just tries to read in a loop kind of
> like you have here? This may be useful elsewhere (probably operating on bytes
> instead of wchar_ts, though).
> 
> For now, you can make sure that your read attempts try to get as much as
> possible by reducing the trial block size on each iteration so that its end is
> aligned with the end of a page.
> 
>   end_of_environment_block = start_of_environment_block + env_block_size *
> sizeof(env_block[0]);
> 
>   // Subtract 1 and truncate down to the nearest page. If it was page-aligned
to
>   // begin with, the subtraction assures that the new end will be lower than
the
>   // old end.
>   end_of_environment_block = (end_of_environment_block - 1) & ~(page_size -
1);
> 
>   // Truncation seems OK if this can’t be divided evenly, because it’ll just
>   // throw away an incomplete code unit.
>   env_block_size = (end_of_environment_block - start_of_environment_block) /
> sizeof(env_block[0]);
> 
> You could write this whole thing in a single line, but it’d be harder to
follow.

Uses the memory map-aware read now.

https://codereview.chromium.org/1360863006/diff/80001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:295: std::wstring look_in(&env_block[0],
env_block_size);
On 2015/09/26 01:46:48, Mark Mentovai - August is over wrote:
> Since you’re just copying the whole thing into look_in, probably env_block
> should have been the wstring you read into to begin with.

Done.

[Mark Mentovai, 2015-09-29 21:57:11]

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:240: return;
Don’t bail out here. You still might be able to save the
RTL_USER_PROCESS_PARAMETERS stuff.

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:243: // Walk the LDR structure to retrieve
its pointed-to data. We don't care too
Question: does MiniDumpWriteDump() collect these regions too, or are we reaching
the land of beating MiniDumpWriteDump() at its own game? I don’t recall seeing
this many more regions than just stacks in recent-ish minidumps that it
produced.

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:247: AddMemorySnapshotForLdrLIST_ENTRY(
I’ve got a new concern. A lot of the extra memory that we’re adding now isn’t
critical to the minidump, and a lot of these regions are unchecked in the sense
that we don’t really know that we’ll be able to read them when it comes time to
write the minidump. And I think that the behavior of a failed memory read when
the dump is being written will result in the whole dump being thrown out. It
would be a shame for a corrupt pointer in one of these structures to mess
everything up.

Can we make AddMemorySnapshot() ensure that it’s adding a mapped and readable
range? We can have it reject adds of totally unmapped memory. We can have it add
the mapped subset of a requested range, or we can also just have it reject
attempts to add memory when any portion of it is unmapped.

I think it’s enough to just make sure that things look good in the map, since
the process will be suspended when we do this stuff for real. I don’t think that
we need to worry about someone else calling VirtualFreeEx() while we’re chewing
on a suspended process.

By the way, this problem doesn’t just apply to the new ExtraMemory() support, we
might run into similar trouble with stack memory too. The Mac version is immune
to this problem because of how CalculateStackRegion() works, but we’d need to be
aware of it when we start leveraging ExtraMemory() over there too.

Fixing this may be out of scope for this change, and I think that’s fine. But we
will need to fix this.

[scottmg, 2015-09-30 18:19:41]

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:240: return;
On 2015/09/29 21:57:11, Mark Mentovai - August is over wrote:
> Don’t bail out here. You still might be able to save the
> RTL_USER_PROCESS_PARAMETERS stuff.

Done.

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:243: // Walk the LDR structure to retrieve
its pointed-to data. We don't care too
On 2015/09/29 21:57:10, Mark Mentovai - August is over wrote:
> Question: does MiniDumpWriteDump() collect these regions too, or are we
reaching
> the land of beating MiniDumpWriteDump() at its own game? I don’t recall seeing
> this many more regions than just stacks in recent-ish minidumps that it
> produced.

Doesn't look like MiniDumpWriteDump() grabs the LDR data. I think it could
sometimes be nice to have the module load list here so that it's easily
accessible in windbg as part of !peb.

It shouldn't be a lot of extra memory (< ~100 path strings plus the linked list
structure) but it's not strictly necessary it seems. So I could go either way on
including them.

I thought it was necessary to read these for windbg, but it appears it's not
required, as current crashes don't have the LDR and the rest of the PEB dumps
fine. I'm not sure what was going wrong in the dumps before when it appeared to
be aborting, but I guess I was mistaken. (Updated the comment here.)

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:247: AddMemorySnapshotForLdrLIST_ENTRY(
On 2015/09/29 21:57:11, Mark Mentovai - August is over wrote:
> I’ve got a new concern. A lot of the extra memory that we’re adding now isn’t
> critical to the minidump, and a lot of these regions are unchecked in the
sense
> that we don’t really know that we’ll be able to read them when it comes time
to
> write the minidump. And I think that the behavior of a failed memory read when
> the dump is being written will result in the whole dump being thrown out. It
> would be a shame for a corrupt pointer in one of these structures to mess
> everything up.
> 
> Can we make AddMemorySnapshot() ensure that it’s adding a mapped and readable
> range? We can have it reject adds of totally unmapped memory. We can have it
add
> the mapped subset of a requested range, or we can also just have it reject
> attempts to add memory when any portion of it is unmapped.
> 
> I think it’s enough to just make sure that things look good in the map, since
> the process will be suspended when we do this stuff for real. I don’t think
that
> we need to worry about someone else calling VirtualFreeEx() while we’re
chewing
> on a suspended process.
> 
> By the way, this problem doesn’t just apply to the new ExtraMemory() support,
we
> might run into similar trouble with stack memory too. The Mac version is
immune
> to this problem because of how CalculateStackRegion() works, but we’d need to
be
> aware of it when we start leveraging ExtraMemory() over there too.
> 
> Fixing this may be out of scope for this change, and I think that’s fine. But
we
> will need to fix this.

Yeah, all good points.

In general, I don't like how I need to do some things twice here. That is, a
read here to walk pointers and then throw that away, only to have the minidump
code read it again. It's not likely to be inconsistent because we're suspended,
but still seems like it could be better.

For now, I made AddMemorySnapshot() verify that the requested range is fully
readable, and filed https://code.google.com/p/crashpad/issues/detail?id=59 to
address it more generally.

[Mark Mentovai, 2015-10-01 19:05:47]

The possible duplicate Ldr string data is the only truly concerning thing here.
Otherwise, this is ready to go.

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:243: // Walk the LDR structure to retrieve
its pointed-to data. We don't care too
On 2015/09/30 18:19:40, scottmg wrote:
> On 2015/09/29 21:57:10, Mark Mentovai - August is over wrote:
> > Question: does MiniDumpWriteDump() collect these regions too, or are we
> reaching
> > the land of beating MiniDumpWriteDump() at its own game? I don’t recall
seeing
> > this many more regions than just stacks in recent-ish minidumps that it
> > produced.
> 
> Doesn't look like MiniDumpWriteDump() grabs the LDR data. I think it could
> sometimes be nice to have the module load list here so that it's easily
> accessible in windbg as part of !peb.
> 
> It shouldn't be a lot of extra memory (< ~100 path strings plus the linked
list
> structure) but it's not strictly necessary it seems. So I could go either way
on
> including them.
> 
> I thought it was necessary to read these for windbg, but it appears it's not
> required, as current crashes don't have the LDR and the rest of the PEB dumps
> fine. I'm not sure what was going wrong in the dumps before when it appeared
to
> be aborting, but I guess I was mistaken. (Updated the comment here.)

No real objection to including this, I’m just trying to maintain a mental list
of things we’re doing better than MiniDumpWriteDump(). In the future, if we find
we’re generating bigger minidumps than we want, we might want to make some of
these things optional. Maybe the list should be written down instead of in our
heads.

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:247: AddMemorySnapshotForLdrLIST_ENTRY(
scottmg wrote:
> In general, I don't like how I need to do some things twice here. That is, a
> read here to walk pointers and then throw that away, only to have the minidump
> code read it again. It's not likely to be inconsistent because we're
suspended,
> but still seems like it could be better.

Nobody says that every memory snapshot you hand out has to be the generic
MemorySnapshotWin. You can read the environment block once into an object that
implements MemorySnapshot, and have its Read() just pass the saved data straight
through to the delegate without ever touching the process.

(If you like this, let’s save it for a follow-up.)

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_r...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:216: void* into) const {
Possibly this and ReadAvailableMemory() should both return early without failing
if num_bytes is 0. Mac’s TaskMemory::ReadMapped() is precedent for this.

ReadProcessMemory() might do the early return thing on its own, so it might be a
no-op and thus unnecessary for ReadMemory(). But for ReadAvailableMemory(), if
you add the logging for the “it wasn’t mapped” cases, that would also log for
the num_bytes = 0 case, since the new logic we agreed on is for a 0-sized range
to not overlap any other, and we’d hit ranges.empty().

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:236: // We only read up until the first
unavailable block, so we only read from the
Put some blank lines before these comments that apply to the things that follow
them but not the things that precede them.

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:239: return 0;
LOG something on the return-0s that don’t pass through ReadMemory()?

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:245: if (!ReadMemory(ranges.front().base(),
ranges.front().size(), into))
It’s not strictly necessary because the tests should cover GetReadableRanges’
behavior well enough that this shouldn’t ever happen, but consider:

  DCHECK_LE(ranges.front.size(), num_bytes);

before calling ReadMemory().

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:241: AddMemorySnapshotForLdrLIST_ENTRY(
Do the elements in the three lists point to the same string data? If so, we’re
going to wind up adding duplicate regions for their UNICODE_STRINGs. As
discussed, I don’t think we should do that.

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:297: //
https://code.google.com/p/crashpad/issues/detail?id=59.
I read this bug and liked the terminology you used, “auxiliary memory”. I’m
kinda keen on renaming ExtraMemory() in the Snapshot family to that. There’s
some naming confusion in the minidump family, where MinidumpMemoryListWriter has
its own thing called “extra memory”. (For a follow-up, if at all.)

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:301: return;
Maybe these early returns should log something. When we get the log messages
encountered during snapshotting hooked up to be carried in the minidump, it
might be nice to see “oh, we’re missing something we tried to collect.”

[scottmg, 2015-10-01 20:17:01]

Thanks!

(Requires regions fix in https://codereview.chromium.org/1370063005/)

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:243: // Walk the LDR structure to retrieve
its pointed-to data. We don't care too
On 2015/10/01 19:05:47, Mark Mentovai wrote:
> On 2015/09/30 18:19:40, scottmg wrote:
> > On 2015/09/29 21:57:10, Mark Mentovai - August is over wrote:
> > > Question: does MiniDumpWriteDump() collect these regions too, or are we
> > reaching
> > > the land of beating MiniDumpWriteDump() at its own game? I don’t recall
> seeing
> > > this many more regions than just stacks in recent-ish minidumps that it
> > > produced.
> > 
> > Doesn't look like MiniDumpWriteDump() grabs the LDR data. I think it could
> > sometimes be nice to have the module load list here so that it's easily
> > accessible in windbg as part of !peb.
> > 
> > It shouldn't be a lot of extra memory (< ~100 path strings plus the linked
> list
> > structure) but it's not strictly necessary it seems. So I could go either
way
> on
> > including them.
> > 
> > I thought it was necessary to read these for windbg, but it appears it's not
> > required, as current crashes don't have the LDR and the rest of the PEB
dumps
> > fine. I'm not sure what was going wrong in the dumps before when it appeared
> to
> > be aborting, but I guess I was mistaken. (Updated the comment here.)
> 
> No real objection to including this, I’m just trying to maintain a mental list
> of things we’re doing better than MiniDumpWriteDump(). In the future, if we
find
> we’re generating bigger minidumps than we want, we might want to make some of
> these things optional. Maybe the list should be written down instead of in our
> heads.

I'll include it for now then as it's nice to have in windbg. Filed
https://code.google.com/p/crashpad/issues/detail?id=60 for the general case.
Might make sense to make some of this configurable.

https://codereview.chromium.org/1360863006/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:247: AddMemorySnapshotForLdrLIST_ENTRY(
On 2015/10/01 19:05:47, Mark Mentovai wrote:
> scottmg wrote:
> > In general, I don't like how I need to do some things twice here. That is, a
> > read here to walk pointers and then throw that away, only to have the
minidump
> > code read it again. It's not likely to be inconsistent because we're
> suspended,
> > but still seems like it could be better.
> 
> Nobody says that every memory snapshot you hand out has to be the generic
> MemorySnapshotWin. You can read the environment block once into an object that
> implements MemorySnapshot, and have its Read() just pass the saved data
straight
> through to the delegate without ever touching the process.
> 
> (If you like this, let’s save it for a follow-up.)

Good point. It's pretty localized at the moment, so I'll just keep it in mind
for future similar cases to see if it comes up again. For "one deep" structures,
the current setup works well.

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_r...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:216: void* into) const {
On 2015/10/01 19:05:47, Mark Mentovai wrote:
> Possibly this and ReadAvailableMemory() should both return early without
failing
> if num_bytes is 0. Mac’s TaskMemory::ReadMapped() is precedent for this.
> 
> ReadProcessMemory() might do the early return thing on its own, so it might be
a
> no-op and thus unnecessary for ReadMemory(). But for ReadAvailableMemory(), if
> you add the logging for the “it wasn’t mapped” cases, that would also log for
> the num_bytes = 0 case, since the new logic we agreed on is for a 0-sized
range
> to not overlap any other, and we’d hit ranges.empty().

Done.

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:236: // We only read up until the first
unavailable block, so we only read from the
On 2015/10/01 19:05:47, Mark Mentovai wrote:
> Put some blank lines before these comments that apply to the things that
follow
> them but not the things that precede them.

Done.

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:239: return 0;
On 2015/10/01 19:05:47, Mark Mentovai wrote:
> LOG something on the return-0s that don’t pass through ReadMemory()?

I was thinking since we're using this in a trial-and-error way for probing the
environment block, it would be better if it weren't noisy. But I guess it is
useful to know that not even the first byte was accessible because that probably
means the parent structure pointing here is broken.

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:245: if (!ReadMemory(ranges.front().base(),
ranges.front().size(), into))
On 2015/10/01 19:05:47, Mark Mentovai wrote:
> It’s not strictly necessary because the tests should cover GetReadableRanges’
> behavior well enough that this shouldn’t ever happen, but consider:
> 
>   DCHECK_LE(ranges.front.size(), num_bytes);
> 
> before calling ReadMemory().

Done.

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:241: AddMemorySnapshotForLdrLIST_ENTRY(
On 2015/10/01 19:05:47, Mark Mentovai wrote:
> Do the elements in the three lists point to the same string data? If so, we’re
> going to wind up adding duplicate regions for their UNICODE_STRINGs. As
> discussed, I don’t think we should do that.

It seems I'm writing more and more of the coalescing in here... but anyway, did
a simple search for identical pre-existing ranges in AddMemorySnapshot().

On the simple crashy_program, this reduced the minidump size from 28,848 bytes
to 27,056 bytes, which is a decent amount considering it only has 4 modules.

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:297: //
https://code.google.com/p/crashpad/issues/detail?id=59.
On 2015/10/01 19:05:47, Mark Mentovai wrote:
> I read this bug and liked the terminology you used, “auxiliary memory”. I’m
> kinda keen on renaming ExtraMemory() in the Snapshot family to that. There’s
> some naming confusion in the minidump family, where MinidumpMemoryListWriter
has
> its own thing called “extra memory”. (For a follow-up, if at all.)

Yeah, I confused myself for a bit with ExtraMemory(). I'll rename these to
AuxiliaryMemory() in a follow-up then.

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:301: return;
On 2015/10/01 19:05:47, Mark Mentovai wrote:
> Maybe these early returns should log something. When we get the log messages
> encountered during snapshotting hooked up to be carried in the minidump, it
> might be nice to see “oh, we’re missing something we tried to collect.”

Done.

[Mark Mentovai, 2015-10-01 22:00:52]

LGTM

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_r...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1360863006/diff/250001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:239: return 0;
scottmg wrote:
> On 2015/10/01 19:05:47, Mark Mentovai wrote:
> > LOG something on the return-0s that don’t pass through ReadMemory()?
> 
> I was thinking since we're using this in a trial-and-error way for probing the
> environment block, it would be better if it weren't noisy. But I guess it is
> useful to know that not even the first byte was accessible because that
probably
> means the parent structure pointing here is broken.

It’s probably reasonable to expect there to be something present where we’re
told the environment block is, and to flag it as an oddity if there isn’t
anything there. It’s good now in the latest patch set.

https://codereview.chromium.org/1360863006/diff/280001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/280001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:317: for (const auto& ms : *into) {
ms is a little bit too abbreviated for an “auto” that doesn’t say the actual
type name. Can we go with memory_snapshot even if it means that it’ll be +1
line?

[scottmg, 2015-10-01 22:05:57]

Thanks.

https://codereview.chromium.org/1360863006/diff/280001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1360863006/diff/280001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:317: for (const auto& ms : *into) {
On 2015/10/01 22:00:52, Mark Mentovai wrote:
> ms is a little bit too abbreviated for an “auto” that doesn’t say the actual
> type name. Can we go with memory_snapshot even if it means that it’ll be +1
> line?

Done.

[Mark Mentovai, 2015-10-01 22:23:28]

Cool. You receive the coveted “unqualified LGTM” now!

[scottmg, 2015-10-01 22:24:18]

Committed patchset #17 (id:300001) manually as
23ab86bc19d0b86d70ccb2a3051090177b90c89c (presubmit successful).