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

snapshot/win/process_reader_win.cc

Index: snapshot/win/process_reader_win.cc
diff --git a/snapshot/win/process_reader_win.cc b/snapshot/win/process_reader_win.cc
index bad81cd7f98311135395f61ddf0c14780b14027e..fb57ce243ae55b9187d43e0d09b63f6a7095d0cf 100644
--- a/snapshot/win/process_reader_win.cc
+++ b/snapshot/win/process_reader_win.cc
@@ -19,6 +19,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/numerics/safe_conversions.h"
 #include "util/win/nt_internals.h"
+#include "util/win/ntstatus_logging.h"
 #include "util/win/process_structs.h"
 #include "util/win/scoped_handle.h"
 #include "util/win/time.h"
@@ -57,10 +58,9 @@ process_types::SYSTEM_PROCESS_INFORMATION<Traits>* GetProcessInformation(
   // This must be in retry loop, as we're racing with process creation on the
   // system to find a buffer large enough to hold all process information.
   for (int tries = 0; tries < 20; ++tries) {
-    const int kSystemExtendedProcessInformation = 57;
+    const int kSystemProcessInformation = 5;

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

Before this, kSystemExtendedProcessInformation had documentary value because it
wasn’t present in any system header. Now, it seems that you can use
SystemProcessInformation from <winternl.h>.

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

Aha, good catch, done.

     status = crashpad::NtQuerySystemInformation(
-        static_cast<SYSTEM_INFORMATION_CLASS>(
-            kSystemExtendedProcessInformation),
+        static_cast<SYSTEM_INFORMATION_CLASS>(kSystemProcessInformation),

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

And then you won’t need a cast anymore!

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

Done.

         reinterpret_cast<void*>(buffer->get()),
         buffer_size,
         &buffer_size);
@@ -77,7 +77,7 @@ process_types::SYSTEM_PROCESS_INFORMATION<Traits>* GetProcessInformation(
   }
 
   if (!NT_SUCCESS(status)) {
-    LOG(ERROR) << "NtQuerySystemInformation failed: " << std::hex << status;
+    NTSTATUS_LOG(ERROR, status) << "NtQuerySystemInformation";
     return nullptr;
   }
 
@@ -95,16 +95,17 @@ process_types::SYSTEM_PROCESS_INFORMATION<Traits>* GetProcessInformation(
 }
 
 template <class Traits>
-HANDLE OpenThread(const process_types::SYSTEM_EXTENDED_THREAD_INFORMATION<
-    Traits>& thread_info) {
+HANDLE OpenThread(
+    const process_types::SYSTEM_THREAD_INFORMATION<Traits>& thread_info) {
   HANDLE handle;
-  ACCESS_MASK query_access = THREAD_GET_CONTEXT | THREAD_SUSPEND_RESUME;
+  ACCESS_MASK query_access =
+      THREAD_GET_CONTEXT | THREAD_SUSPEND_RESUME | THREAD_QUERY_INFORMATION;
   OBJECT_ATTRIBUTES object_attributes;
   InitializeObjectAttributes(&object_attributes, nullptr, 0, nullptr, nullptr);
   NTSTATUS status = crashpad::NtOpenThread(
       &handle, query_access, &object_attributes, &thread_info.ClientId);
   if (!NT_SUCCESS(status)) {
-    LOG(ERROR) << "NtOpenThread failed";
+    NTSTATUS_LOG(ERROR, status) << "NtOpenThread";
     return nullptr;
   }
   return handle;
@@ -114,19 +115,15 @@ HANDLE OpenThread(const process_types::SYSTEM_EXTENDED_THREAD_INFORMATION<
 // side-effect of returning the SuspendCount of the thread on success, so we
 // fill out these two pieces of semi-unrelated data in the same function.
 template <class Traits>
-void FillThreadContextAndSuspendCount(
-    const process_types::SYSTEM_EXTENDED_THREAD_INFORMATION<Traits>&
-        thread_info,
-    ProcessReaderWin::Thread* thread,
-    ProcessSuspensionState suspension_state) {
+void FillThreadContextAndSuspendCount(HANDLE thread_handle,
+                                      ProcessReaderWin::Thread* thread,
+                                      ProcessSuspensionState suspension_state) {
   // Don't suspend the thread if it's this thread. This is really only for test
   // binaries, as we won't be walking ourselves, in general.
-  bool is_current_thread = thread_info.ClientId.UniqueThread ==
+  bool is_current_thread = thread->id ==
                            reinterpret_cast<process_types::TEB<Traits>*>(
                                NtCurrentTeb())->ClientId.UniqueThread;
 
-  ScopedKernelHANDLE thread_handle(OpenThread(thread_info));
-
   // TODO(scottmg): Handle cross-bitness in this function.
 
   if (is_current_thread) {
@@ -134,7 +131,7 @@ void FillThreadContextAndSuspendCount(
     thread->suspend_count = 0;
     RtlCaptureContext(&thread->context);
   } else {
-    DWORD previous_suspend_count = SuspendThread(thread_handle.get());
+    DWORD previous_suspend_count = SuspendThread(thread_handle);
     if (previous_suspend_count == -1) {
       PLOG(ERROR) << "SuspendThread failed";
       return;
@@ -147,12 +144,12 @@ void FillThreadContextAndSuspendCount(
 
     memset(&thread->context, 0, sizeof(thread->context));
     thread->context.ContextFlags = CONTEXT_ALL;
-    if (!GetThreadContext(thread_handle.get(), &thread->context)) {
+    if (!GetThreadContext(thread_handle, &thread->context)) {
       PLOG(ERROR) << "GetThreadContext failed";
       return;
     }
 
-    if (!ResumeThread(thread_handle.get())) {
+    if (!ResumeThread(thread_handle)) {
       PLOG(ERROR) << "ResumeThread failed";
     }
   }
@@ -160,6 +157,63 @@ void FillThreadContextAndSuspendCount(
 
 }  // namespace
 
+template <class Traits>

[scottmg, 2015/09/15 21:13:34]

On XP, the "extended" version of this structure doesn't get a filled out
TebBase. So, switch to a more "normal" approach of just getting the thread
handle from the process enumeration, then using NtQueryInformationThread to get
the stack range.

+void ReadThreadData(ProcessReaderWin* process_reader) {

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

This belongs the unnamed namespace.

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

It wasn't because it needed a friend declaration in ProcessReaderWin, but it
should have just been a member function anyway, so changed to that.

+  DCHECK(process_reader->threads_.empty());
+
+  scoped_ptr<uint8_t[]> buffer;
+  process_types::SYSTEM_PROCESS_INFORMATION<Traits>* process_information =
+      GetProcessInformation<Traits>(process_reader->process_, &buffer);
+  if (!process_information)
+    return;
+
+  for (unsigned long i = 0; i < process_information->NumberOfThreads; ++i) {
+    const process_types::SYSTEM_THREAD_INFORMATION<Traits>& thread_info =
+        process_information->Threads[i];
+    ProcessReaderWin::Thread thread;
+    thread.id = thread_info.ClientId.UniqueThread;
+
+    ScopedKernelHANDLE thread_handle(OpenThread(thread_info));
+    if (!thread_handle.is_valid())
+      continue;
+
+    FillThreadContextAndSuspendCount<Traits>(

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

(This and the next comment describe things found in the original code too.)

Maybe this should be bool-returning so that you can continue if it fails, like
with other failures in this loop.

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

Done.

+        thread_handle.get(), &thread, process_reader->suspension_state_);
+
+    // TODO(scottmg): I believe we could reverse engineer the PriorityClass from
+    // the Priority, BasePriority, and
+    // https://msdn.microsoft.com/en-us/library/windows/desktop/ms685100 .
+    // MinidumpThreadWriter doesn't handle it yet in any case, so investigate
+    // both of those at the same time if it's useful.
+    thread.priority_class = NORMAL_PRIORITY_CLASS;
+
+    thread.priority = thread_info.Priority;
+
+    process_types::THREAD_BASIC_INFORMATION<Traits> thread_basic_info;
+    NTSTATUS status = crashpad::NtQueryInformationThread(
+        thread_handle.get(),
+        static_cast<THREADINFOCLASS>(ThreadBasicInformation),
+        &thread_basic_info,
+        sizeof(thread_basic_info),
+        nullptr);
+    if (!NT_SUCCESS(status)) {
+      NTSTATUS_LOG(ERROR, status) << "NtQueryInformationThread";
+      continue;
+    }
+
+    // Read the TIB (Thread Information Block) which is the first element of the
+    // TEB, for its stack fields.
+    process_types::NT_TIB<Traits> tib;
+    if (process_reader->ReadMemory(
+            thread_basic_info.TebBaseAddress, sizeof(tib), &tib)) {
+      // Note, "backwards" because of direction of stack growth.

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

Might be good to actually check that limit <= base then, since you’re sticking
the result of subtraction into an unsigned variable.

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

Done.

+      thread.stack_region_address = tib.StackLimit;
+      thread.stack_region_size = tib.StackBase - tib.StackLimit;
+    }
+    process_reader->threads_.push_back(thread);
+  }
+}
+
 ProcessReaderWin::Thread::Thread()
     : context(),
       id(0),
@@ -243,50 +297,10 @@ const std::vector<ProcessReaderWin::Thread>& ProcessReaderWin::Threads() {
 
   initialized_threads_ = true;
 
-  DCHECK(threads_.empty());
-
-#if ARCH_CPU_32_BITS
-  using SizeTraits = process_types::internal::Traits32;
-#else
-  using SizeTraits = process_types::internal::Traits64;
-#endif
-  scoped_ptr<uint8_t[]> buffer;
-  process_types::SYSTEM_PROCESS_INFORMATION<SizeTraits>* process_information =
-      GetProcessInformation<SizeTraits>(process_, &buffer);
-  if (!process_information)
-    return threads_;
-
-  for (unsigned long i = 0; i < process_information->NumberOfThreads; ++i) {
-    const process_types::SYSTEM_EXTENDED_THREAD_INFORMATION<SizeTraits>&
-        thread_info = process_information->Threads[i];
-    Thread thread;
-    thread.id = thread_info.ClientId.UniqueThread;
-
-    FillThreadContextAndSuspendCount(thread_info, &thread, suspension_state_);
-
-    // TODO(scottmg): I believe we could reverse engineer the PriorityClass from
-    // the Priority, BasePriority, and
-    // https://msdn.microsoft.com/en-us/library/windows/desktop/ms685100 .
-    // MinidumpThreadWriter doesn't handle it yet in any case, so investigate
-    // both of those at the same time if it's useful.
-    thread.priority_class = NORMAL_PRIORITY_CLASS;
-
-    thread.priority = thread_info.Priority;
-    thread.teb = thread_info.TebBase;
-
-    // While there are semi-documented fields in the thread structure called
-    // StackBase and StackLimit, they don't appear to be correct in practice (or
-    // at least, I don't know how to interpret them). Instead, read the TIB
-    // (Thread Information Block) which is the first element of the TEB, and use
-    // its stack fields.
-    process_types::NT_TIB<SizeTraits> tib;
-    if (ReadMemory(thread_info.TebBase, sizeof(tib), &tib)) {
-      // Note, "backwards" because of direction of stack growth.
-      thread.stack_region_address = tib.StackLimit;
-      thread.stack_region_size = tib.StackBase - tib.StackLimit;
-    }
-    threads_.push_back(thread);
-  }
+  if (process_info_.Is64Bit())
+    ReadThreadData<process_types::internal::Traits64>(this);
+  else
+    ReadThreadData<process_types::internal::Traits32>(this);
 
   return threads_;
 }