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

util/win/process_info.cc

-// 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.
 // You may obtain a copy of the License at
 //
 //     http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS,
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
 #include "util/win/process_info.h"
 
 #include <winternl.h>
 
 #include "base/logging.h"
 #include "util/numeric/safe_assignment.h"
+#include "util/win/ntstatus_logging.h"
 #include "util/win/process_structs.h"
 
 namespace crashpad {
 
 namespace {
 
 NTSTATUS NtQueryInformationProcess(HANDLE process_handle,
                                    PROCESSINFOCLASS process_information_class,
                                    PVOID process_information,
                                    ULONG process_information_length,
@@ skipping 42 matching lines @@
     PLOG(ERROR) << "ReadProcessMemory UNICODE_STRING";
     return false;
   }
   if (bytes_read != us.Length) {
     LOG(ERROR) << "ReadProcessMemory UNICODE_STRING incorrect size";
     return false;
   }
   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),
                          into,
                          sizeof(T),
                          &bytes_read)) {
     // We don't have a name for the type we're reading, so include the signature
     // to get the type of T.
     PLOG(ERROR) << "ReadProcessMemory " << __FUNCSIG__;
     return false;
   }
   if (bytes_read != sizeof(T)) {
     LOG(ERROR) << "ReadProcessMemory " << __FUNCSIG__ << " incorrect size";
     return false;
   }
   return true;
 }
 
 }  // 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;
   }
 
   // Try to read the process environment block.
   process_types::PEB<Traits> peb;
   if (!ReadStruct(process, peb_address, &peb))
     return false;
 
   process_types::RTL_USER_PROCESS_PARAMETERS<Traits> process_parameters;
   if (!ReadStruct(process, peb.ProcessParameters, &process_parameters))
@@ skipping 83 matching lines @@
       initialized_() {
 }
 
 ProcessInfo::~ProcessInfo() {
 }
 
 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.
     is_64_bit_ = false;
   } else {
     // Otherwise, it's either 32 on 32, or 64 on 64. Use GetSystemInfo() to
     // distinguish between these two cases.
     SYSTEM_INFO system_info;
     GetSystemInfo(&system_info);
     is_64_bit_ =
         system_info.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_AMD64;
   }
 
 #if ARCH_CPU_32_BITS
   if (is_64_bit_) {
     LOG(ERROR) << "Reading x64 process from x86 process not supported";
     return false;
   }
 #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;
-    return false;
-  }
-  if (bytes_returned != sizeof(process_basic_information)) {
-    LOG(ERROR) << "NtQueryInformationProcess incorrect size";
+  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;
   }
 
-  // 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;
 }
 
 bool ProcessInfo::Is64Bit() const {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
   return is_64_bit_;
 }
 
@@ skipping 18 matching lines @@
   return true;
 }
 
 bool ProcessInfo::Modules(std::vector<Module>* modules) const {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
   *modules = modules_;
   return true;
 }
 
 }  // namespace crashpad