Implement ModuleSnapshotWin::UUID

Implement ModuleSnapshotWin::UUID

Reads CodeView PDB GUID from Debug Directory of PE header.

R=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/3ef04d14f2fd75355d3e5b04d40fe8654a892936

[Mark Mentovai, 2015-08-31 21:48:21]

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/module_sn...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:154:
pe_image_reader_->DebugDirectoryInformation(uuid, &age, &pdbname);
If this fails, reinitialize uuid to the default UUID?

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.cc (right):

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:36: struct CodeviewInfoPDB70 {
Isn’t this MinidumpModuleCodeViewRecordPDB70 from
minidump/minidump_extensions.h? Maybe you should use that struct. If necessary,
we can move it out of minidump to resolve the conceptual dependency problem.

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:135: IMAGE_DEBUG_DIRECTORY debug_directory;
Declare this before the test above so that you can use the preferred
sizeof(variable) instead of sizeof(Type).

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:136: if (!CheckedReadMemory(Address() +
data_directory.VirtualAddress,
This allowed data_directory.Size to be larger than sizeof(data_directory), but
then does a ReadMemory of the potentially-larger size into a memory region
that’s always the smaller size. If data_directory.Size can really be bigger, it
implies that we need to handle the case of multiple IMAGE_DEBUG_DIRECTORY
structures somehow. (Loop through them and find the one with the right type?)

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:141: }
Validate the Type (and slash or version fields) to make sure that it’s really
CodeView. (If you loop over multiple structs, this may be what a loop needs to
look for.)

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:145: data.get())) {
Don’t do this if debug_directory.SizeOfData was 0.

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:150: CodeviewInfoPDB70* codeview =
How are you sure it’s 7.0?

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:151:
reinterpret_cast<CodeviewInfoPDB70*>(data.get());
Speaking of that…

You don’t need to worry about non-7.0 in this change, but we quite likely should
support putting non-7.0 CV links (and even IMAGE_DEBUG_MISC and perhaps even the
non-debug-link-but-debug-in-module stuff) into the minidump file. At the very
least, TODO it or stick a comment somewhere saying that we don’t currently
support it.

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.h (right):

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:97: //! \brief Obtains the module's debug
directory, if any.
information from the module’s debug directory

[Mark Mentovai, 2015-08-31 21:49:22]

I see this under patch set 6:

Depends on Patchset:

Issue 1314093002 Patch 80001
Dependent Patchsets:

Issue 1317023003 Patch 40001
Issue 1317023003 Patch 60001

Cool! How did you do that? Local git branch relationships?

[scottmg, 2015-08-31 23:05:02]

Thanks. Moved the structure out of minidump to util. I couldn't get it to find
the right base for the diff unfortunately, but I didn't change the structures at
all.

I also added a basic test for PEImageReader. I can't check the UUID without
rewriting the implementation in the test, but it at least confirms that we read
a record and got a sane pdb path, which probably means the record got extracted
correctly.

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/module_sn...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:154:
pe_image_reader_->DebugDirectoryInformation(uuid, &age, &pdbname);
On 2015/08/31 21:48:20, Mark Mentovai - August is over wrote:
> If this fails, reinitialize uuid to the default UUID?

Done.

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.cc (right):

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:36: struct CodeviewInfoPDB70 {
On 2015/08/31 21:48:20, Mark Mentovai - August is over wrote:
> Isn’t this MinidumpModuleCodeViewRecordPDB70 from
> minidump/minidump_extensions.h? Maybe you should use that struct. If
necessary,
> we can move it out of minidump to resolve the conceptual dependency problem.

Done.

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:135: IMAGE_DEBUG_DIRECTORY debug_directory;
On 2015/08/31 21:48:20, Mark Mentovai - August is over wrote:
> Declare this before the test above so that you can use the preferred
> sizeof(variable) instead of sizeof(Type).

Done.

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:136: if (!CheckedReadMemory(Address() +
data_directory.VirtualAddress,
On 2015/08/31 21:48:20, Mark Mentovai - August is over wrote:
> This allowed data_directory.Size to be larger than sizeof(data_directory), but
> then does a ReadMemory of the potentially-larger size into a memory region
> that’s always the smaller size. If data_directory.Size can really be bigger,
it
> implies that we need to handle the case of multiple IMAGE_DEBUG_DIRECTORY
> structures somehow. (Loop through them and find the one with the right type?)

Done. The correct type to read is IMAGE_DEBUG_TYPE_CODEVIEW. In practice,
typical binaries seem to have two entries, the first is of type
IMAGE_DEBUG_TYPE_CODEVIEW, and the second of .Type == 0xc which is undocumented,
but appears to be a comment field.

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:141: }
On 2015/08/31 21:48:21, Mark Mentovai - August is over wrote:
> Validate the Type (and slash or version fields) to make sure that it’s really
> CodeView. (If you loop over multiple structs, this may be what a loop needs to
> look for.)

Done.

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:145: data.get())) {
On 2015/08/31 21:48:20, Mark Mentovai - August is over wrote:
> Don’t do this if debug_directory.SizeOfData was 0.

Done.

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:150: CodeviewInfoPDB70* codeview =
On 2015/08/31 21:48:21, Mark Mentovai - August is over wrote:
> How are you sure it’s 7.0?

Done.

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:151:
reinterpret_cast<CodeviewInfoPDB70*>(data.get());
On 2015/08/31 21:48:20, Mark Mentovai - August is over wrote:
> Speaking of that…
> 
> You don’t need to worry about non-7.0 in this change, but we quite likely
should
> support putting non-7.0 CV links (and even IMAGE_DEBUG_MISC and perhaps even
the
> non-debug-link-but-debug-in-module stuff) into the minidump file. At the very
> least, TODO it or stick a comment somewhere saying that we don’t currently
> support it.

Done.

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.h (right):

https://codereview.chromium.org/1311003003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:97: //! \brief Obtains the module's debug
directory, if any.
On 2015/08/31 21:48:21, Mark Mentovai - August is over wrote:
> information from the module’s debug directory

Done.

[scottmg, 2015-08-31 23:10:10]

On 2015/08/31 21:49:22, Mark Mentovai - August is over wrote:
> I see this under patch set 6:
> 
> Depends on Patchset:
> 
> Issue 1314093002 Patch 80001
> Dependent Patchsets:
> 
> Issue 1317023003 Patch 40001
> Issue 1317023003 Patch 60001
> 
> Cool! How did you do that? Local git branch relationships?

Yeah, `git cl upload` does that now based on the local git tracking branch.

I use (the badly named script) "git-newx" to branch off of the current branch:

===================
#!/bin/bash
if [ $# -ne 1 ]
then
  echo "Usage: git newx branch-name"
  exit 65
fi
git checkout -b $1 -t `git symbolic-ref --short -q HEAD`
===================

In this case, I was using the child relationship just to have those changes
active so I could test end-to-end uploading, but it doesn't actually depend on
the patches listed in this case. (Though the parent does depend on
grandparent... so I guess it's sometimes useful information.)

[Mark Mentovai, 2015-09-01 01:20:31]

LGTM otherwise. Nice job shifting all of the Doxygen stuff as needed, too.

Separately, I think that pe_image_reader should guard against reading beyond the
end of the image.

https://codereview.chromium.org/1311003003/diff/140001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.h (right):

https://codereview.chromium.org/1311003003/diff/140001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:108: //! \brief Reads the `NT_HEADERS` from the
beginning of the image.
IMAGE_NT_HEADERS?

https://codereview.chromium.org/1311003003/diff/140001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader_test.cc (right):

https://codereview.chromium.org/1311003003/diff/140001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader_test.cc:36: EXPECT_EQ(module_info.lpBaseOfDll,
self);
For the EXPECT_/ASSERT_ family, the arguments are ordered (expected, actual).
That happens later in this file too.

https://codereview.chromium.org/1311003003/diff/140001/util/misc/shared_pdb_s...
File util/misc/shared_pdb_structures.h (right):

https://codereview.chromium.org/1311003003/diff/140001/util/misc/shared_pdb_s...
util/misc/shared_pdb_structures.h:15: #ifndef
CRASHPAD_UTIL_MISC_SHARED_PDB_STRUCTURES_H_
Probably just “pdb_structures.h”. Sharing’s just a trait of everyone being able
to depend on util.

[scottmg, 2015-09-01 16:29:53]

Thanks. I'll send a follow up for range checking in pe_image_reader.

https://codereview.chromium.org/1311003003/diff/140001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.h (right):

https://codereview.chromium.org/1311003003/diff/140001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:108: //! \brief Reads the `NT_HEADERS` from the
beginning of the image.
On 2015/09/01 01:20:31, Mark Mentovai - August is over wrote:
> IMAGE_NT_HEADERS?

Done.

https://codereview.chromium.org/1311003003/diff/140001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader_test.cc (right):

https://codereview.chromium.org/1311003003/diff/140001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader_test.cc:36: EXPECT_EQ(module_info.lpBaseOfDll,
self);
On 2015/09/01 01:20:31, Mark Mentovai - August is over wrote:
> For the EXPECT_/ASSERT_ family, the arguments are ordered (expected, actual).
> That happens later in this file too.

Done.

https://codereview.chromium.org/1311003003/diff/140001/util/misc/shared_pdb_s...
File util/misc/shared_pdb_structures.h (right):

https://codereview.chromium.org/1311003003/diff/140001/util/misc/shared_pdb_s...
util/misc/shared_pdb_structures.h:15: #ifndef
CRASHPAD_UTIL_MISC_SHARED_PDB_STRUCTURES_H_
On 2015/09/01 01:20:31, Mark Mentovai - August is over wrote:
> Probably just “pdb_structures.h”. Sharing’s just a trait of everyone being
able
> to depend on util.

Done.

[scottmg, 2015-09-01 16:31:30]

Committed patchset #9 (id:160001) manually as
3ef04d14f2fd75355d3e5b04d40fe8654a892936 (presubmit successful).

[scottmg, 2015-09-01 16:47:33]

On 2015/09/01 01:20:31, Mark Mentovai - August is over wrote:
> LGTM otherwise. Nice job shifting all of the Doxygen stuff as needed, too.
> 
> Separately, I think that pe_image_reader should guard against reading beyond
the
> end of the image.

Looking again, I think the reads are validated using a CheckedWinAddressRange
with the module base/size that's passed in in Initialize(). Did you notice some
that look wrong?

> 
>
https://codereview.chromium.org/1311003003/diff/140001/snapshot/win/pe_image_...
> File snapshot/win/pe_image_reader.h (right):
> 
>
https://codereview.chromium.org/1311003003/diff/140001/snapshot/win/pe_image_...
> snapshot/win/pe_image_reader.h:108: //! \brief Reads the `NT_HEADERS` from the
> beginning of the image.
> IMAGE_NT_HEADERS?
> 
>
https://codereview.chromium.org/1311003003/diff/140001/snapshot/win/pe_image_...
> File snapshot/win/pe_image_reader_test.cc (right):
> 
>
https://codereview.chromium.org/1311003003/diff/140001/snapshot/win/pe_image_...
> snapshot/win/pe_image_reader_test.cc:36: EXPECT_EQ(module_info.lpBaseOfDll,
> self);
> For the EXPECT_/ASSERT_ family, the arguments are ordered (expected, actual).
> That happens later in this file too.
> 
>
https://codereview.chromium.org/1311003003/diff/140001/util/misc/shared_pdb_s...
> File util/misc/shared_pdb_structures.h (right):
> 
>
https://codereview.chromium.org/1311003003/diff/140001/util/misc/shared_pdb_s...
> util/misc/shared_pdb_structures.h:15: #ifndef
> CRASHPAD_UTIL_MISC_SHARED_PDB_STRUCTURES_H_
> Probably just “pdb_structures.h”. Sharing’s just a trait of everyone being
able
> to depend on util.

[Mark Mentovai, 2015-09-01 18:52:20]

Oh, duh, we’ve already been through this before. Yes, this looks really good
with the in-range checks.