Implement ModuleSnapshotWin::UUID

snapshot/win/pe_image_reader.cc

Index: snapshot/win/pe_image_reader.cc
diff --git a/snapshot/win/pe_image_reader.cc b/snapshot/win/pe_image_reader.cc
index 01755612a409a4dfa10cddcf70a0f09255fbf7c7..e9becbfc450c94d834e425c146a555356865fba3 100644
--- a/snapshot/win/pe_image_reader.cc
+++ b/snapshot/win/pe_image_reader.cc
@@ -17,6 +17,7 @@
 #include <string.h>
 
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/strings/stringprintf.h"
 #include "client/crashpad_info.h"
 #include "snapshot/win/process_reader_win.h"
@@ -32,6 +33,13 @@ std::string RangeToString(const CheckedWinAddressRange& range) {
                             range.Is64Bit() ? "64" : "32");
 }
 
+struct CodeviewInfoPDB70 {

[Mark Mentovai, 2015/08/31 21:48:20]

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.

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

Done.

+  DWORD codeview_signature;
+  GUID guid;
+  DWORD age;
+  char pdb_filename[1];
+};
+
 }  // namespace
 
 PEImageReader::PEImageReader()
@@ -110,13 +118,46 @@ bool PEImageReader::GetCrashpadInfo(
   return true;
 }
 
-bool PEImageReader::GetSectionByName(const std::string& name,
-                                     IMAGE_SECTION_HEADER* section) const {
-  if (name.size() > sizeof(section->Name)) {
-    LOG(WARNING) << "supplied section name too long " << name;
+bool PEImageReader::DebugDirectoryInformation(UUID* uuid,
+                                              DWORD* age,
+                                              std::string* pdbname) {
+  WinVMAddress nt_headers_address;
+  IMAGE_NT_HEADERS nt_headers;
+  if (!ReadNtHeaders(&nt_headers_address, &nt_headers))
+    return false;
+
+  const IMAGE_DATA_DIRECTORY& data_directory =
+      nt_headers.OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_DEBUG];
+  if (data_directory.VirtualAddress == 0 || data_directory.Size == 0)
+    return false;
+  if (data_directory.Size % sizeof(IMAGE_DEBUG_DIRECTORY) != 0)
+    return false;
+  IMAGE_DEBUG_DIRECTORY debug_directory;

[Mark Mentovai, 2015/08/31 21:48:20]

Declare this before the test above so that you can use the preferred
sizeof(variable) instead of sizeof(Type).

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

Done.

+  if (!CheckedReadMemory(Address() + data_directory.VirtualAddress,

[Mark Mentovai, 2015/08/31 21:48:20]

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?)

[scottmg, 2015/08/31 23:05:01]

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.

+                         data_directory.Size,
+                         &debug_directory)) {
+    LOG(WARNING) << "could not read data directory";
+    return false;
+  }

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

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.)

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

Done.

+  scoped_ptr<char []> data(new char[debug_directory.SizeOfData]);
+  if (!CheckedReadMemory(Address() + debug_directory.AddressOfRawData,
+                         debug_directory.SizeOfData,
+                         data.get())) {

[Mark Mentovai, 2015/08/31 21:48:20]

Don’t do this if debug_directory.SizeOfData was 0.

[scottmg, 2015/08/31 23:05:01]

Done.

+    LOG(WARNING) << "could not read debug directory";
     return false;
   }
 
+  CodeviewInfoPDB70* codeview =

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

How are you sure it’s 7.0?

[scottmg, 2015/08/31 23:05:01]

Done.

+      reinterpret_cast<CodeviewInfoPDB70*>(data.get());

[Mark Mentovai, 2015/08/31 21:48:20]

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.

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

Done.

+  uuid->InitializeFromSystemUUID(&codeview->guid);
+  *age = codeview->age;
+  *pdbname = std::string(codeview->pdb_filename);
+  return true;
+}
+
+// TODO(scottmg): This needs to be made cross-bitness supporting.
+bool PEImageReader::ReadNtHeaders(WinVMAddress* nt_headers_address,
+                                  IMAGE_NT_HEADERS* nt_headers) const {
   IMAGE_DOS_HEADER dos_header;
   if (!CheckedReadMemory(Address(), sizeof(IMAGE_DOS_HEADER), &dos_header)) {
     LOG(WARNING) << "could not read dos header of " << module_name_;
@@ -128,20 +169,33 @@ bool PEImageReader::GetSectionByName(const std::string& name,
     return false;
   }
 
-  // TODO(scottmg): This is reading a same-bitness sized structure.
-  IMAGE_NT_HEADERS nt_headers;
-  WinVMAddress nt_headers_address = Address() + dos_header.e_lfanew;
+  *nt_headers_address = Address() + dos_header.e_lfanew;
   if (!CheckedReadMemory(
-          nt_headers_address, sizeof(IMAGE_NT_HEADERS), &nt_headers)) {
+          *nt_headers_address, sizeof(IMAGE_NT_HEADERS), nt_headers)) {
     LOG(WARNING) << "could not read nt headers of " << module_name_;
     return false;
   }
 
-  if (nt_headers.Signature != IMAGE_NT_SIGNATURE) {
+  if (nt_headers->Signature != IMAGE_NT_SIGNATURE) {
     LOG(WARNING) << "invalid signature in nt headers of " << module_name_;
     return false;
   }
 
+  return true;
+}
+
+bool PEImageReader::GetSectionByName(const std::string& name,
+                                     IMAGE_SECTION_HEADER* section) const {
+  if (name.size() > sizeof(section->Name)) {
+    LOG(WARNING) << "supplied section name too long " << name;
+    return false;
+  }
+
+  WinVMAddress nt_headers_address;
+  IMAGE_NT_HEADERS nt_headers;
+  if (!ReadNtHeaders(&nt_headers_address, &nt_headers))
+    return false;
+
   WinVMAddress first_section_address =
       nt_headers_address + offsetof(IMAGE_NT_HEADERS, OptionalHeader) +
       nt_headers.FileHeader.SizeOfOptionalHeader;