win: Add Handles() to ProcessInfo

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 fa413cc3c8c51ecd04b2f6e5b96dfbc037defad7..beb830fffc6297d1c9a2b1bbb36d3f2185ffc767 100644
--- a/util/win/process_info.cc
+++ b/util/win/process_info.cc
@@ -20,12 +20,15 @@
 #include <limits>
 
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/strings/stringprintf.h"
 #include "base/template_util.h"
 #include "build/build_config.h"
 #include "util/numeric/safe_assignment.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"
 
 namespace crashpad {
 
@@ -127,6 +130,30 @@ MEMORY_BASIC_INFORMATION64 MemoryBasicInformationToMemoryBasicInformation64(
   return mbi64;
 }
 
+// NtQueryObject with a retry for size mismatch and a hint for initial size.
+scoped_ptr<uint8_t[]> QueryObject(
+    HANDLE handle,
+    OBJECT_INFORMATION_CLASS object_information_class,
+    size_t size) {
+  ULONG return_length;
+  scoped_ptr<uint8_t[]> buffer(new uint8_t[size]);
+  NTSTATUS status = crashpad::NtQueryObject(
+      handle, object_information_class, buffer.get(), size, &return_length);
+  if (status == STATUS_INFO_LENGTH_MISMATCH) {
+    DCHECK_GE(return_length, size);
+    size = return_length;
+    buffer.reset(new uint8_t[size]);
+    status = crashpad::NtQueryObject(
+        handle, object_information_class, buffer.get(), size, &return_length);
+    if (!NT_SUCCESS(status)) {
+      NTSTATUS_LOG(ERROR, status) << "NtQueryObject";
+      return scoped_ptr<uint8_t[]>();
+    }
+  }

[Mark Mentovai, 2015/10/14 22:49:55]

  } else if (!NT_SUCCESS(status)) {
    NTSTATUS_LOG(ERROR, status) << "NtQueryObject";

[scottmg, 2015/10/15 00:17:41]

Done.

+
+  return buffer.Pass();
+}
+
 }  // namespace
 
 template <class Traits>
@@ -314,12 +341,127 @@ bool ReadMemoryInfo(HANDLE process, bool is_64_bit, ProcessInfo* process_info) {
   return true;
 }
 
+template <class Traits>
+std::vector<ProcessInfo::Handle> ProcessInfo::BuildHandleVector(
+    HANDLE process) const {
+  size_t buffer_size = 2 * 1024 * 1024;
+  scoped_ptr<uint8_t[]> buffer(new uint8_t[buffer_size]);
+
+  // Typically if the buffer were too small, STATUS_INFO_LENGTH_MISMATCH would
+  // return the correct size in the final argument, but it does not in for this
+  // SYSTEM_INFORMATION_CLASS, so we loop and attempt larger sizes.
+  NTSTATUS status;
+  for (int tries = 0; tries < 5; ++tries) {
+    status = crashpad::NtQuerySystemInformation(

[Mark Mentovai, 2015/10/14 22:49:55]

Do you want a QuerySystemInformation() wrapper for this to handle the resizing,
kind of like QueryObject()?

[scottmg, 2015/10/15 00:17:41]

It's only done once at the moment, so I think it's OK to keep it inline. I could
be convinced either way though.

+        static_cast<SYSTEM_INFORMATION_CLASS>(SystemExtendedHandleInformation),
+        buffer.get(),
+        buffer_size,
+        nullptr);
+    if (NT_SUCCESS(status)) {
+      break;
+    } else if (status == STATUS_INFO_LENGTH_MISMATCH) {
+      buffer_size *= 2;

[Mark Mentovai, 2015/10/14 22:49:55]

Why don’t you use the ReturnLength argument to tell you what to resize to, as in
your QueryObject() wrapper?

[scottmg, 2015/10/15 00:17:41]

That's the comment above the loop -- it doesn't properly return the expected
length on some OSs unfortunately, see e.g.
http://forum.sysinternals.com/topic18892.html at the end of Step 1.

+      buffer.reset(new uint8_t[buffer_size]);
+    }
+  }
+
+  if (!NT_SUCCESS(status)) {
+    NTSTATUS_LOG(ERROR, status)
+        << "NtQuerySystemInformation SystemExtendedHandleInformation";
+    return std::vector<Handle>();
+  }
+
+  process_types::SYSTEM_HANDLE_INFORMATION_EX<
+      Traits>* system_handle_information_ex =

[Mark Mentovai, 2015/10/14 22:49:55]

Weird formatting (did clang-format do this?!)

[scottmg, 2015/10/15 00:17:41]

Yup. A little better with an auto*?

+      reinterpret_cast<process_types::SYSTEM_HANDLE_INFORMATION_EX<Traits>*>(
+          buffer.get());
+
+  std::vector<Handle> handles;
+
+  for (size_t i = 0; i < system_handle_information_ex->NumberOfHandles; ++i) {
+    const auto& handle = system_handle_information_ex->Handles[i];
+    if (handle.UniqueProcessId != process_id_)
+      continue;
+
+    // TODO(scottmg): Could special case for self.
+    HANDLE dup_handle;
+    if (!DuplicateHandle(process,

[Mark Mentovai, 2015/10/14 22:49:55]

In https://code.google.com/p/crashpad/issues/detail?id=21#c2, you claimed that
DuplicateHandle() to dupe handles into our process was scary. It certainly
registers on my heebie-jeebie-meter too, but I’m not sure if that’s me being
overly paranoid or if there’s an actual risk. But maybe it’s fine to just use
with NtQueryObject() as you’re doing here. Can you clarify what you thought was
scary?

You also said that you thought you’d start with option 1, no type and no name,
but here you’re grabbing the type. Did something change in the analysis?

[scottmg, 2015/10/15 00:17:41]

DuplicateHandle()ing into ourselves scared me at first, but after talking to
Justin I was less nervous about that. For "Level #3" I'm definitely a little
more nervous about the NtQueryObject calls with obscure/undocumented values as
people have clearly noted hangs on some OSs. But I (and more importantly Justin)
think it's OK to only do the duplicate and the limited information query.

+                         reinterpret_cast<HANDLE>(handle.HandleValue),
+                         GetCurrentProcess(),
+                         &dup_handle,
+                         0,
+                         false,
+                         DUPLICATE_SAME_ACCESS)) {
+      // Some handles cannot be duplicated, for example handles of type
+      // EtwRegistration, so we simply drop these. (We also do not strictly know

[Mark Mentovai, 2015/10/14 22:49:55]

Does MiniDumpWriteDump() lose these too?

Is there an error code that we can key this off?

[scottmg, 2015/10/15 00:17:41]

Just confirmed with MiniDumpWriteDump() via
https://codereview.chromium.org/1405443006 that it doesn't have any information
for them (i.e. can't DuplicateHandle() them either, I guess), but it does at
least include them in the list of handles with no additional information, which
is better. Switched to that on (any) duplicate failure.

+      // that we're suspended here, so there's a race between here and
+      // collecting them, though in practice the target process will always be
+      // suspended.)
+      continue;
+    }
+    ScopedKernelHANDLE scoped_dup_handle(dup_handle);
+
+    scoped_ptr<uint8_t[]> object_basic_information_buffer =
+        QueryObject(dup_handle,
+                    ObjectBasicInformation,
+                    sizeof(PUBLIC_OBJECT_BASIC_INFORMATION));
+    if (!object_basic_information_buffer)
+      continue;
+
+    scoped_ptr<uint8_t[]> object_type_information_buffer =
+        QueryObject(dup_handle,
+                    ObjectTypeInformation,
+                    sizeof(PUBLIC_OBJECT_TYPE_INFORMATION));
+    if (!object_type_information_buffer)
+      continue;
+
+    PUBLIC_OBJECT_BASIC_INFORMATION* object_basic_information =
+        reinterpret_cast<PUBLIC_OBJECT_BASIC_INFORMATION*>(
+            object_basic_information_buffer.get());
+    PUBLIC_OBJECT_TYPE_INFORMATION* object_type_information =
+        reinterpret_cast<PUBLIC_OBJECT_TYPE_INFORMATION*>(
+            object_type_information_buffer.get());
+
+    Handle result_handle;
+    result_handle.type_name =
+        std::wstring(object_type_information->TypeName.Buffer,
+                     object_type_information->TypeName.Length /

[Mark Mentovai, 2015/10/14 22:49:55]

DCHECK that Length % sizeof(result_handle.type_name[0]) is 0.

[scottmg, 2015/10/15 00:17:41]

Done.

+                         sizeof(result_handle.type_name[0]));
+    result_handle.handle = static_cast<uint32_t>(handle.HandleValue);
+    // The Attributes and GrantedAccess sometimes differ slightly between the

[Mark Mentovai, 2015/10/14 22:49:55]

Blank before. Same on 439.

[scottmg, 2015/10/15 00:17:41]

Done.

+    // data retrieved in SYSTEM_HANDLE_INFORMATION_EX and
+    // PUBLIC_OBJECT_TYPE_INFORMATION. We prefer the values in
+    // SYSTEM_HANDLE_INFORMATION_EX because they were retrieved from the target
+    // process, rather than on the duplicated handle.
+    result_handle.attributes = handle.HandleAttributes;
+    result_handle.granted_access = handle.GrantedAccess;
+    result_handle.pointer_count = object_basic_information->PointerCount;
+    // Subtract one to account for our DuplicateHandle().
+    DCHECK_GT(object_basic_information->HandleCount, 0u);

[Mark Mentovai, 2015/10/14 22:49:55]

Should this be > 1? At least one original reference, and one for the duplicate
in here?

[scottmg, 2015/10/15 00:17:41]

Yeah, done.

+    result_handle.handle_count = object_basic_information->HandleCount - 1;

[Mark Mentovai, 2015/10/14 22:49:55]

Happy to see this accounted for!

+    handles.push_back(result_handle);
+  }
+  return handles;
+}
+
 ProcessInfo::Module::Module() : name(), dll_base(0), size(0), timestamp() {
 }
 
 ProcessInfo::Module::~Module() {
 }
 
+ProcessInfo::Handle::Handle()
+    : type_name(),
+      handle(),

[Mark Mentovai, 2015/10/14 22:49:55]

Zeroes in the integer fields.

[scottmg, 2015/10/15 00:17:41]

Done.

+      attributes(),
+      granted_access(),
+      pointer_count(),
+      handle_count() {
+}
+
+ProcessInfo::Handle::~Handle() {
+}
+
 ProcessInfo::ProcessInfo()
     : process_id_(),
       inherited_from_process_id_(),
@@ -387,6 +529,11 @@ bool ProcessInfo::Initialize(HANDLE process) {
     return false;
   }
 
+  if (is_64_bit_)
+    handles_ = BuildHandleVector<process_types::internal::Traits64>(process);
+  else
+    handles_ = BuildHandleVector<process_types::internal::Traits32>(process);
+
   INITIALIZATION_STATE_SET_VALID(initialized_);
   return true;
 }