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..2fa8ce4bee9d97a0aa10c3656ec011280eb40c14 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,31 @@ 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,
+    ULONG 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);

[Mark Mentovai, 2015/10/15 05:25:17]

DCHECK_GT, not DCHECK_GE.

[scottmg, 2015/10/15 21:38:45]

Done.

+    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[]>();
+  }
+
+  return buffer.Pass();

[Mark Mentovai, 2015/10/15 05:25:17]

buffer.resize(return_length) to trim any excess. (The hint may have been big.)

[Mark Mentovai, 2015/10/15 15:07:22]

Mark Mentovai wrote:
Forget this comment and any other comment where I might have confused
scoped_ptr<uint8_t[]> with vector<uint8_t> and suggested resizing a region
created with new uint8_t[]. Don’t trim those. However, I still think it’s useful
to make return_length available to the caller so that it can protect against
accesses outside of the buffer.

[Mark Mentovai, 2015/10/15 15:28:53]

I wrote:
Since this information is coming from the OS which we should be able to trust,
these checks can be quick debug-only affairs. And since we’re treating |size|
not only as a hint but also a minimum size given the existing DCHECK on line
143, we can simply add another DCHECK right here, and not have to worry about
getting the size back to the caller. This function’s comment should be updated
to reflect size’s use as a minimum, or maybe the argument should even be
renamed.

[scottmg, 2015/10/15 21:38:45]

Done.

+}
+
 }  // namespace
 
 template <class Traits>
@@ -314,12 +342,134 @@ 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 {
+  ULONG buffer_size = 2 * 1024 * 1024;

[Mark Mentovai, 2015/10/15 05:25:17]

Pretty huge buffer. At its minimum size, it’s room for over 52,000 handles when
queried from a 64-bit process. What are we expecting to see here?

[scottmg, 2015/10/15 21:38:44]

Unfortunately yes. I have ~110k handles on my system at the moment. There's no
API to retrieve the handles for only the process we care about, so the number
retrieved can be quite large.

+  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

[Mark Mentovai, 2015/10/15 05:25:17]

“in for this” → “for”

[scottmg, 2015/10/15 21:38:45]

Done.

+  // SYSTEM_INFORMATION_CLASS, so we loop and attempt larger sizes.

[Mark Mentovai, 2015/10/15 05:25:17]

Is the returned size correct when this returns success? If not, there’s no need
to take any action. If it is, resize the buffer precisely after a successful
call.

[scottmg, 2015/10/15 21:38:45]

Not doing anything here per vector/scoped_ptr.

+  NTSTATUS status;
+  for (int tries = 0; tries < 5; ++tries) {
+    status = crashpad::NtQuerySystemInformation(
+        static_cast<SYSTEM_INFORMATION_CLASS>(SystemExtendedHandleInformation),
+        buffer.get(),
+        buffer_size,
+        nullptr);
+    if (NT_SUCCESS(status)) {
+      break;
+    } else if (status == STATUS_INFO_LENGTH_MISMATCH) {

[Mark Mentovai, 2015/10/15 05:25:17]

If status is not this and not success, I don’t think we need to bother retrying
with the same size buffer. So:

  if (status != STATUS_INFO_LENGTH_MISMATCH)
    break;
  // resize buffer

[scottmg, 2015/10/15 21:38:45]

Done.

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

[Mark Mentovai, 2015/10/15 05:25:17]

Could be & instead of *.

[scottmg, 2015/10/15 21:38:45]

Done.

+      reinterpret_cast<process_types::SYSTEM_HANDLE_INFORMATION_EX<Traits>*>(

[Mark Mentovai, 2015/10/15 05:25:17]

Since this is system-specific and not process-specific, does
SYSTEM_HANDLE_INFORMATION_EX need to be traits-ed? Won’t
NtQuerySystemInformation() return something in this process’ bitness? We ought
to be able to get away with declaring the pair of structs more naturally. Then
this whole function wouldn’t need to be traits-ed either.

[scottmg, 2015/10/15 21:38:45]

Done.

+          buffer.get());
+
+  std::vector<Handle> handles;
+
+  for (size_t i = 0; i < system_handle_information_ex->NumberOfHandles; ++i) {

[Mark Mentovai, 2015/10/15 05:25:17]

Before entering the loop, make sure that offsetof(system_handle_information_ex,
Handles) + system_handle_information_ex->NumberOfHandles *
sizeof(system_handle_information_ex->Handles[0]) <= buffer.size().

[Mark Mentovai, 2015/10/15 15:28:53]

I wrote:
By the same logic, this can be expressed in a DCHECK too. You’d use the returned
size instead of buffer.size() if the returned size is correct when
NtQuerySystemInformation() is successful.

[scottmg, 2015/10/15 21:38:45]

Done.

+    const auto& handle = system_handle_information_ex->Handles[i];
+    if (handle.UniqueProcessId != process_id_)

[Mark Mentovai, 2015/10/15 05:25:16]

Noticing that handle.UniqueProcessId might be 64 bits while process_id_ is 32.
I’m assuming that the high 32 bits of UniqueProcessId will always be clear, and
they just designed the struct that way for alignment/packing. (Then again, the
could have made UniqueProcessId and HandleValue both 32 bits, and it’d pack just
fine that way too.)

[scottmg, 2015/10/15 21:38:45]

Yeah, good point, it's a bit weird. Since they're numbers it seems weird to mask
the top of UniqueProcessId, but as far as I know there can't be anything >
0xffffffff in there.

+      continue;
+
+    // TODO(scottmg): Could special case for self.
+    HANDLE dup_handle;
+    if (!DuplicateHandle(process,
+                         reinterpret_cast<HANDLE>(handle.HandleValue),
+                         GetCurrentProcess(),
+                         &dup_handle,
+                         0,
+                         false,
+                         DUPLICATE_SAME_ACCESS)) {
+      // Some handles cannot be duplicated, for example, handles of type
+      // EtwRegistration. If we fail to duplicate, then we can't gather any more
+      // information, so just include the handle the list with what we do have.
+      Handle result_handle;
+      result_handle.handle = static_cast<uint32_t>(handle.HandleValue);
+      result_handle.attributes = handle.HandleAttributes;
+      result_handle.granted_access = handle.GrantedAccess;
+      handles.push_back(result_handle);
+      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;

[Mark Mentovai, 2015/10/15 05:25:17]

Worthwhile to add the data we could get from SYSTEM_HANDLE_TABLE_ENTRY_INFO_EX
here?

Maybe this ought to always populate as much as possible, never skipping a handle
found in the process:

    Handle result_handle;
    result_handle.handle = static_cast<uint32_t>(handle.HandleValue);
    result_handle.attributes = handle.HandleAttributes;
    result_handle.granted_access = handle.GrantedAccess;
    if (DuplicateHandle(…)) {
      ScopedKernelHandle scoped_dup_handle(dup_handle);
      scoped_ptr<uint8_t[]> buffer = QueryObject(…);
      if (buffer) {
        result_handle.pointer_count = object_basic_information->PointerCount;
        result_handle.handle_count = object_basic_information->HandleCount - 1;
      }
      buffer.reset();  // because you don’t need both at the same time
      buffer.reset(QueryObject(…));
      if (buffer) {
        result_handle.type_name = std::wstring(…);
      }
    }
    handles.push_back(result_handle);

[scottmg, 2015/10/15 21:38:45]

Done.

+
+    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;

[Mark Mentovai, 2015/10/15 05:25:17]

Didn’t you find that MSVS 2015 didn’t like shadowing variables? (See line 398.)
If so, you could just declare this before the DuplicateHandle().

[scottmg, 2015/10/15 21:38:45]

With your better version above, this disappears.

+    DCHECK_EQ(object_type_information->TypeName.Length %
+                  sizeof(result_handle.type_name[0]),
+              0u);
+    result_handle.type_name =
+        std::wstring(object_type_information->TypeName.Buffer,
+                     object_type_information->TypeName.Length /
+                         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/15 05:25:17]

Blank before. And also a blank before pointer_count, since this comment doesn’t
apply to that.

[scottmg, 2015/10/15 21:38:44]

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;

[Mark Mentovai, 2015/10/15 05:25:17]

Just confirming: making a duplicate doesn’t actually affect this number at all,
right?

[scottmg, 2015/10/15 21:38:45]

That one's harder to test, but I believe it's actually two too big.

+
+    // Subtract one to account for our DuplicateHandle().
+    DCHECK_GT(object_basic_information->HandleCount, 1u);
+    result_handle.handle_count = object_basic_information->HandleCount - 1;
+    handles.push_back(result_handle);

[Mark Mentovai, 2015/10/15 05:25:16]

Blank before this, too.

[scottmg, 2015/10/15 21:38:45]

Done.

+  }
+  return handles;
+}
+
 ProcessInfo::Module::Module() : name(), dll_base(0), size(0), timestamp() {
 }
 
 ProcessInfo::Module::~Module() {
 }
 
+ProcessInfo::Handle::Handle()
+    : type_name(),
+      handle(0),
+      attributes(0),
+      granted_access(0),
+      pointer_count(0),
+      handle_count(0) {
+}
+
+ProcessInfo::Handle::~Handle() {
+}
+
 ProcessInfo::ProcessInfo()
     : process_id_(),
       inherited_from_process_id_(),
@@ -328,6 +478,7 @@ ProcessInfo::ProcessInfo()
       peb_size_(0),
       modules_(),
       memory_info_(),
+      handles_(),
       is_64_bit_(false),
       is_wow64_(false),
       initialized_() {
@@ -387,6 +538,11 @@ bool ProcessInfo::Initialize(HANDLE process) {
     return false;
   }
 
+  if (is_64_bit_)
+    handles_ = BuildHandleVector<process_types::internal::Traits64>(process);

[Mark Mentovai, 2015/10/15 05:25:17]

Since this can’t do anything but get all handles system-wide, and because we may
not always want to get handles (like if we ever want to write a
handle-stream-less minidump), it makes some sense to do this lazily in
Handles(), as opposed to here in Initialize().

[scottmg, 2015/10/15 21:38:45]

Done.

+  else
+    handles_ = BuildHandleVector<process_types::internal::Traits32>(process);
+
   INITIALIZATION_STATE_SET_VALID(initialized_);
   return true;
 }