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

snapshot/win/process_reader_win.cc

 // Copyright 2015 The Crashpad Authors. All rights reserved.
 //
 // 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 "snapshot/win/process_reader_win.h"
 
 #include <winternl.h>
 
 #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"
 
 namespace crashpad {
 
 namespace {
 
 // Gets a pointer to the process information structure after a given one, or
 // null when iteration is complete, assuming they've been retrieved in a block
@@ skipping 18 matching lines @@
 template <class Traits>
 process_types::SYSTEM_PROCESS_INFORMATION<Traits>* GetProcessInformation(
     HANDLE process_handle,
     scoped_ptr<uint8_t[]>* buffer) {
   ULONG buffer_size = 16384;
   buffer->reset(new uint8_t[buffer_size]);
   NTSTATUS status;
   // 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);
     if (status == STATUS_BUFFER_TOO_SMALL ||
         status == STATUS_INFO_LENGTH_MISMATCH) {
       // Add a little extra to try to avoid an additional loop iteration. We're
       // racing with system-wide process creation between here and the next call
       // to NtQuerySystemInformation().
       buffer_size += 4096;
       buffer->reset(new uint8_t[buffer_size]);
     } else {
       break;
     }
   }
 
   if (!NT_SUCCESS(status)) {
-    LOG(ERROR) << "NtQuerySystemInformation failed: " << std::hex << status;
+    NTSTATUS_LOG(ERROR, status) << "NtQuerySystemInformation";
     return nullptr;
   }
 
   process_types::SYSTEM_PROCESS_INFORMATION<Traits>* process =
       reinterpret_cast<process_types::SYSTEM_PROCESS_INFORMATION<Traits>*>(
           buffer->get());
   DWORD process_id = GetProcessId(process_handle);
   do {
     if (process->UniqueProcessId == process_id)
       return process;
   } while (process = NextProcess(process));
 
   LOG(ERROR) << "process " << process_id << " not found";
   return nullptr;
 }
 
 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;
 }
 
 // It's necessary to suspend the thread to grab CONTEXT. SuspendThread has a
 // 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) {
     DCHECK(suspension_state == ProcessSuspensionState::kRunning);
     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;
     }
     DCHECK(previous_suspend_count > 0 ||
            suspension_state == ProcessSuspensionState::kRunning);
     thread->suspend_count =
         previous_suspend_count -
         (suspension_state == ProcessSuspensionState::kSuspended ? 1 : 0);
 
     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";
     }
   }
 }
 
 }  // 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),
       teb(0),
       stack_region_address(0),
       stack_region_size(0),
       suspend_count(0),
       priority_class(0),
       priority(0) {
 }
@@ skipping 63 matching lines @@
 }
 
 const std::vector<ProcessReaderWin::Thread>& ProcessReaderWin::Threads() {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
   if (initialized_threads_)
     return 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_;
 }
 
 const std::vector<ProcessInfo::Module>& ProcessReaderWin::Modules() {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
   if (!process_info_.Modules(&modules_)) {
     LOG(ERROR) << "couldn't retrieve modules";
   }
 
   return modules_;
 }
 
 }  // namespace crashpad