win x86: Grab bag of restructuring to get tests working on x86-on-x86

util/win/process_info.cc

Index: util/win/process_info.cc
diff --git a/util/win/process_info.cc b/util/win/process_info.cc
index ca8277ed0308736ad257a3b5c9ef46deee356cb7..afba0cae16071420d0eec6c128340e6f2ce21dca 100644
--- a/util/win/process_info.cc
+++ b/util/win/process_info.cc
@@ -1,4 +1,3 @@
-// Copyright 2015 The Crashpad Authors. All rights reserved.

[Mark Mentovai, 2015/09/16 02:57:20]

Pretty sure we still own this file.

[scottmg, 2015/09/16 18:05:07]

Done.

 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -18,6 +17,7 @@
 
 #include "base/logging.h"
 #include "util/numeric/safe_assignment.h"
+#include "util/win/ntstatus_logging.h"
 #include "util/win/process_structs.h"
 
 namespace crashpad {
@@ -80,7 +80,8 @@ bool ReadUnicodeString(HANDLE process,
   return true;
 }
 
-template <class T> bool ReadStruct(HANDLE process, WinVMAddress at, T* into) {
+template <class T>
+bool ReadStruct(HANDLE process, WinVMAddress at, T* into) {
   SIZE_T bytes_read;
   if (!ReadProcessMemory(process,
                          reinterpret_cast<const void*>(at),
@@ -102,13 +103,70 @@ template <class T> bool ReadStruct(HANDLE process, WinVMAddress at, T* into) {
 }  // namespace
 
 template <class Traits>
+bool GetProcessBasicInformation(HANDLE process,
+                                bool is_wow64,
+                                ProcessInfo* process_info,
+                                WinVMAddress* peb_address) {
+  ULONG bytes_returned;
+  process_types::PROCESS_BASIC_INFORMATION<Traits> process_basic_information;
+  NTSTATUS status =
+      crashpad::NtQueryInformationProcess(process,
+                                          ProcessBasicInformation,
+                                          &process_basic_information,
+                                          sizeof(process_basic_information),
+                                          &bytes_returned);
+  if (!NT_SUCCESS(status)) {
+    NTSTATUS_LOG(ERROR, status) << "NtQueryInformationProcess";
+    return false;
+  }
+  if (bytes_returned != sizeof(process_basic_information)) {
+    LOG(ERROR) << "NtQueryInformationProcess incorrect size";
+    return false;
+  }
+
+  // See https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203 on
+  // 32 bit being the correct size for HANDLEs for proceses, even on Windows
+  // x64. API functions (e.g. OpenProcess) take only a DWORD, so there's no
+  // sense in maintaining the top bits.
+  process_info->process_id_ =
+      static_cast<DWORD>(process_basic_information.UniqueProcessId);
+  process_info->inherited_from_process_id_ = static_cast<DWORD>(
+      process_basic_information.InheritedFromUniqueProcessId);
+
+  // We now want to read the PEB to gather the rest of our information. The
+  // PebBaseAddress as returned above is what we want for 64-on-64 and 32-on-32,
+  // but for Wow64, we want to read the 32 bit PEB (a Wow64 process has both).
+  // The address of this is found by a second call to NtQueryInformationProcess.
+  *peb_address = process_basic_information.PebBaseAddress;

[Mark Mentovai, 2015/09/16 02:57:20]

if (!is_wow64) {
  // this assignment
} else {
  // call NtQueryInformationProcess() again
}

[scottmg, 2015/09/16 18:05:07]

Done.

+  if (is_wow64) {
+    ULONG_PTR wow64_peb_address;
+    status = crashpad::NtQueryInformationProcess(process,
+                                                 ProcessWow64Information,

[Mark Mentovai, 2015/09/16 02:57:20]

It’s a little weird that the thing named this way and documented as returning
nonzero for a WoW64 process actually gives you back the 32-bit PEB here. But
this exploration into WoW64 internals has taught me that the whole system is a
little weird.

[scottmg, 2015/09/16 18:05:07]

Yeah, the limited bit that MSDN documented is confusing. We could probably find
another way to get the WOW64 PEB, but this seems like the closest to
almost-documented.

+                                                 &wow64_peb_address,
+                                                 sizeof(wow64_peb_address),
+                                                 &bytes_returned);
+    if (!NT_SUCCESS(status)) {
+      NTSTATUS_LOG(ERROR, status), "NtQueryInformationProcess";
+      return false;
+    }
+    if (bytes_returned != sizeof(wow64_peb_address)) {
+      LOG(ERROR) << "NtQueryInformationProcess incorrect size";
+      return false;
+    }
+    *peb_address = wow64_peb_address;
+  }
+
+  return true;
+}
+
+template <class Traits>
 bool ReadProcessData(HANDLE process,
                      WinVMAddress peb_address_vmaddr,
                      ProcessInfo* process_info) {
+

[Mark Mentovai, 2015/09/16 02:57:20]

Unnecessary new blank line.

[scottmg, 2015/09/16 18:05:06]

Done.

   Traits::Pointer peb_address;
   if (!AssignIfInRange(&peb_address, peb_address_vmaddr)) {
-    LOG(ERROR) << "peb_address_vmaddr " << peb_address_vmaddr
-               << " out of range";
+    LOG(ERROR) << "peb address" << peb_address_vmaddr << " out of range";

[Mark Mentovai, 2015/09/16 02:57:20]

Need a space after the word “address”.

peb_address_vmaddr will probably be formatted as a decimal number, you may want
to use base::StringPrintf() here.

[scottmg, 2015/09/16 18:05:06]

Done.

     return false;
   }
 
@@ -212,6 +270,7 @@ bool ProcessInfo::Initialize(HANDLE process) {
   INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
 
   is_wow64_ = IsProcessWow64(process);
+  bool self_is_wow64 = IsProcessWow64(GetCurrentProcess());

[Mark Mentovai, 2015/09/16 02:57:20]

Don’t declare it ’til you need it. Since you only use it once, you can just
write it inline in the compound expression below, to avoid having to make these
calls unless they’re required.

[scottmg, 2015/09/16 18:05:07]

Gone.

 
   if (is_wow64_) {
     // If it's WoW64, then it's 32-on-64.
@@ -232,73 +291,26 @@ bool ProcessInfo::Initialize(HANDLE process) {
   }
 #endif
 
-  ULONG bytes_returned;
-  // We assume this process is not running on Wow64. The
-  // PROCESS_BASIC_INFORMATION uses the OS size (that is, even Wow64 has a 64
-  // bit one.)
-  // TODO(scottmg): Either support running as Wow64, or check/resolve this at a
-  // higher level.
-#if ARCH_CPU_32_BITS
-  process_types::PROCESS_BASIC_INFORMATION<process_types::internal::Traits32>
-      process_basic_information;
-#else
-  process_types::PROCESS_BASIC_INFORMATION<process_types::internal::Traits64>
-      process_basic_information;
-#endif
-  NTSTATUS status =
-      crashpad::NtQueryInformationProcess(process,
-                                          ProcessBasicInformation,
-                                          &process_basic_information,
-                                          sizeof(process_basic_information),
-                                          &bytes_returned);
-  if (status < 0) {
-    LOG(ERROR) << "NtQueryInformationProcess: status=" << status;
+  WinVMAddress peb_address;
+  bool result =
+      (is_64_bit_ || (is_wow64_ && !self_is_wow64))

[Mark Mentovai, 2015/09/16 02:57:20]

This condition makes my head hurt. It seems like it should be simpler.

[scottmg, 2015/09/16 18:05:07]

Me too! I think what it really needed to be was based on the host arch.

+          ? GetProcessBasicInformation<process_types::internal::Traits64>(
+                process, is_wow64_, this, &peb_address)
+          : GetProcessBasicInformation<process_types::internal::Traits32>(
+                process, is_wow64_, this, &peb_address);
+  if (!result) {
+    LOG(ERROR) << "GetProcessBasicInformation failed";
     return false;
   }
-  if (bytes_returned != sizeof(process_basic_information)) {
-    LOG(ERROR) << "NtQueryInformationProcess incorrect size";
-    return false;
-  }
-
-  // See https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203 on
-  // 32 bit being the correct size for HANDLEs for proceses, even on Windows
-  // x64. API functions (e.g. OpenProcess) take only a DWORD, so there's no
-  // sense in maintaining the top bits.
-  process_id_ = static_cast<DWORD>(process_basic_information.UniqueProcessId);
-  inherited_from_process_id_ = static_cast<DWORD>(
-      process_basic_information.InheritedFromUniqueProcessId);
-
-  // We now want to read the PEB to gather the rest of our information. The
-  // PebBaseAddress as returned above is what we want for 64-on-64 and 32-on-32,
-  // but for Wow64, we want to read the 32 bit PEB (a Wow64 process has both).
-  // The address of this is found by a second call to NtQueryInformationProcess.
-  WinVMAddress peb_address = process_basic_information.PebBaseAddress;
-  if (is_wow64_) {
-    ULONG_PTR wow64_peb_address;
-    status =
-        crashpad::NtQueryInformationProcess(process,
-                                            ProcessWow64Information,
-                                            &wow64_peb_address,
-                                            sizeof(wow64_peb_address),
-                                            &bytes_returned);
-    if (status < 0) {
-      LOG(ERROR) << "NtQueryInformationProcess: status=" << status;
-      return false;
-    }
-    if (bytes_returned != sizeof(wow64_peb_address)) {
-      LOG(ERROR) << "NtQueryInformationProcess incorrect size";
-      return false;
-    }
-    peb_address = wow64_peb_address;
-  }
 
-  // Read the PEB data using the correct word size.
-  bool result = is_64_bit_ ? ReadProcessData<process_types::internal::Traits64>(
+  result = is_64_bit_ ? ReadProcessData<process_types::internal::Traits64>(
                                  process, peb_address, this)
                            : ReadProcessData<process_types::internal::Traits32>(
                                  process, peb_address, this);
-  if (!result)
+  if (!result) {
+    LOG(ERROR) << "ReadProcessData failed";
     return false;
+  }
 
   INITIALIZATION_STATE_SET_VALID(initialized_);
   return true;