win: support x64 reading x86 (wow64)

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 81a86704c177e7e7d2c64fdf19788bea3baa5818..eeeb2aad2ff728a36eec769e5e05d8fecd759a57 100644
--- a/snapshot/win/process_reader_win.cc
+++ b/snapshot/win/process_reader_win.cc
@@ -116,19 +116,19 @@ HANDLE OpenThread(
 template <class Traits>
 bool FillThreadContextAndSuspendCount(HANDLE thread_handle,
                                       ProcessReaderWin::Thread* thread,
-                                      ProcessSuspensionState suspension_state) {
+                                      ProcessSuspensionState suspension_state,
+                                      bool is_wow64) {
   // 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->id ==
                            reinterpret_cast<process_types::TEB<Traits>*>(
                                NtCurrentTeb())->ClientId.UniqueThread;
 
-  // TODO(scottmg): Handle cross-bitness in this function.
-
   if (is_current_thread) {
     DCHECK(suspension_state == ProcessSuspensionState::kRunning);
     thread->suspend_count = 0;
-    RtlCaptureContext(&thread->context);
+    DCHECK(!is_wow64);

[Mark Mentovai, 2015/09/18 15:44:07]

Why not?

(After reading more of this file) Oh, I see, you force is_wow64 to false for
ARCH_CPU_32_BIT.

That’s kind of confusing…I see what you’re doing, but maybe the variable needs
to be renamed. Someone looking at this function in isolation isn’t going to get
it.

[scottmg, 2015/09/18 19:45:26]

I'm not quite sure what you mean, is that a better name?

+    RtlCaptureContext(&thread->context_native);
   } else {
     DWORD previous_suspend_count = SuspendThread(thread_handle);
     if (previous_suspend_count == -1) {
@@ -141,12 +141,25 @@ bool FillThreadContextAndSuspendCount(HANDLE thread_handle,
         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, &thread->context)) {
-      PLOG(ERROR) << "GetThreadContext failed";
-      return false;
+#if defined(ARCH_CPU_64_BITS)
+    if (is_wow64) {
+      memset(&thread->context_wow64, 0, sizeof(thread->context_wow64));

[Mark Mentovai, 2015/09/18 15:44:07]

Maybe give the union a name so that you can memset the whole thing to 0 outside
of any conditional.

[scottmg, 2015/09/18 19:45:26]

Done.

+      thread->context_wow64.ContextFlags = CONTEXT_ALL;
+      if (!Wow64GetThreadContext(thread_handle, &thread->context_wow64)) {

[Mark Mentovai, 2015/09/18 15:44:06]

Isn’t this Vista and later?

[scottmg, 2015/09/18 19:45:26]

Yes, but it's in ARCH_CPU_64_BITS, and we've never pretended to try to support
WinXP x64.

+        PLOG(ERROR) << "Wow64GetThreadContext failed";
+        return false;
+      }
+    } else {
+#endif  // ARCH_CPU_64_BITS
+      memset(&thread->context_native, 0, sizeof(thread->context_native));
+      thread->context_native.ContextFlags = CONTEXT_ALL;
+      if (!GetThreadContext(thread_handle, &thread->context_native)) {
+        PLOG(ERROR) << "GetThreadContext failed";
+        return false;
+      }
+#if defined(ARCH_CPU_64_BITS)

[Mark Mentovai, 2015/09/18 15:44:06]

Maybe do the const bool thing I suggested before to avoid the preprocessor
funnies and this extra #ifdef just for a }.

[scottmg, 2015/09/18 19:45:26]

Is that what you were thinking?

     }
+#endif  // ARCH_CPU_64_BITS
 
     if (!ResumeThread(thread_handle)) {
       PLOG(ERROR) << "ResumeThread failed";
@@ -160,7 +173,10 @@ bool FillThreadContextAndSuspendCount(HANDLE thread_handle,
 }  // namespace
 
 ProcessReaderWin::Thread::Thread()
-    : context(),
+    : context_native(),
+#if defined(ARCH_CPU_64_BITS)
+      context_wow64(),

[Mark Mentovai, 2015/09/18 15:44:07]

If the union is named, you should just initialize the whole thing once.

Maybe a better structure is

  union {
    CONTEXT native;
#if defined(ARCH_CPU_64_BITS)
    WOW64_CONTEXT wow64;
#endif
  } context;

[scottmg, 2015/09/18 19:45:26]

Done.

+#endif  // ARCH_CPU_64_BITS
       id(0),
       teb(0),
       stack_region_address(0),
@@ -242,10 +258,11 @@ const std::vector<ProcessReaderWin::Thread>& ProcessReaderWin::Threads() {
 
   initialized_threads_ = true;
 
-  if (process_info_.Is64Bit())
-    ReadThreadData<process_types::internal::Traits64>();
-  else
-    ReadThreadData<process_types::internal::Traits32>();
+#if ARCH_CPU_64_BITS

[Mark Mentovai, 2015/09/18 15:44:06]

defined()

[scottmg, 2015/09/18 19:45:26]

Done.

+  ReadThreadData<process_types::internal::Traits64>(process_info_.IsWow64());
+#else
+  ReadThreadData<process_types::internal::Traits32>(false);
+#endif
 
   return threads_;
 }
@@ -261,7 +278,7 @@ const std::vector<ProcessInfo::Module>& ProcessReaderWin::Modules() {
 }
 
 template <class Traits>
-void ProcessReaderWin::ReadThreadData() {
+void ProcessReaderWin::ReadThreadData(bool is_wow64) {
   DCHECK(threads_.empty());
 
   scoped_ptr<uint8_t[]> buffer;
@@ -281,7 +298,7 @@ void ProcessReaderWin::ReadThreadData() {
       continue;
 
     if (!FillThreadContextAndSuspendCount<Traits>(
-            thread_handle.get(), &thread, suspension_state_)) {
+            thread_handle.get(), &thread, suspension_state_, is_wow64)) {
       continue;
     }
 
@@ -309,15 +326,32 @@ void ProcessReaderWin::ReadThreadData() {
     // 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 (ReadMemory(thread_basic_info.TebBaseAddress, sizeof(tib), &tib)) {
+    thread.teb = thread_basic_info.TebBaseAddress;
+    if (ReadMemory(thread.teb, sizeof(tib), &tib)) {
+      WinVMAddress base = 0;
+      WinVMAddress limit = 0;
+      // If we're reading a WOW64 process, then the TIB we just retrieved is the
+      // x64 one. The first word of the x64 TIB points at the x86 TIB. See
+      // https://msdn.microsoft.com/en-us/library/dn424783.aspx
+      if (is_wow64) {
+        process_types::NT_TIB<process_types::internal::Traits32> tib32;
+        thread.teb = tib.Wow64Teb;
+        if (ReadMemory(thread.teb, sizeof(tib32), &tib32)) {
+          base = tib32.StackBase;
+          limit = tib32.StackLimit;
+        }
+      } else {
+        base = tib.StackBase;
+        limit = tib.StackLimit;
+      }
+
       // Note, "backwards" because of direction of stack growth.
-      thread.stack_region_address = tib.StackLimit;
-      if (tib.StackLimit > tib.StackBase) {
-        LOG(ERROR) << "invalid stack range: " << tib.StackBase << " - "
-                   << tib.StackLimit;
+      thread.stack_region_address = limit;
+      if (limit > base) {
+        LOG(ERROR) << "invalid stack range: " << base << " - " << limit;
         thread.stack_region_size = 0;
       } else {
-        thread.stack_region_size = tib.StackBase - tib.StackLimit;
+        thread.stack_region_size = base - limit;
       }
     }
     threads_.push_back(thread);