winheap_dump: handle errors gracefully

base/trace_event/winheap_dump_provider_win.cc

Index: base/trace_event/winheap_dump_provider_win.cc
diff --git a/base/trace_event/winheap_dump_provider_win.cc b/base/trace_event/winheap_dump_provider_win.cc
index f918aafad196fce937b98d1f199e5fe00c19d0cb..2d0c5f6a94744dc524acb3f79349e8206dfee847 100644
--- a/base/trace_event/winheap_dump_provider_win.cc
+++ b/base/trace_event/winheap_dump_provider_win.cc
@@ -86,12 +86,14 @@ bool WinHeapDumpProvider::OnMemoryDump(const MemoryDumpArgs& args,
   for (size_t i = 0; i < number_of_heaps; ++i) {
     WinHeapInfo heap_info = {0};
     heap_info.heap_id = all_heaps[i];
-    GetHeapInformation(&heap_info, block_to_skip);
 
-    all_heap_info.allocated_size += heap_info.allocated_size;
-    all_heap_info.committed_size += heap_info.committed_size;
-    all_heap_info.block_count += heap_info.block_count;
+    if (GetHeapInformation(&heap_info, block_to_skip)) {
+      all_heap_info.allocated_size += heap_info.allocated_size;
+      all_heap_info.committed_size += heap_info.committed_size;
+      all_heap_info.block_count += heap_info.block_count;
+    }
   }
+  CHECK(all_heap_info.allocated_size != 0);

[danakj, 2016/08/15 23:42:03]

CHECK_NE will give better failure output

[liamjm (20p), 2016/10/11 18:57:31]

Done.

   // Report the heap dump.
   ReportHeapDump(pmd, all_heap_info);
   return true;
@@ -100,7 +102,25 @@ bool WinHeapDumpProvider::OnMemoryDump(const MemoryDumpArgs& args,
 bool WinHeapDumpProvider::GetHeapInformation(
     WinHeapInfo* heap_info,
     const std::set<void*>& block_to_skip) {
-  CHECK(::HeapLock(heap_info->heap_id) == TRUE);
+  // NOTE: bug 464430

[danakj, 2016/08/15 23:42:03]

nit: crbug.com/464430

[liamjm (20p), 2016/10/11 18:57:30]

Done.

+  // As a part of the CSRSS lockdown in the referenced bug, it will invalidate

[danakj, 2016/08/15 23:42:03]

Please explain/write out what CSRSS is here, acronyms are hard.

[liamjm (20p), 2016/10/11 18:57:31]

Done.

+  // the heap used by CSRSS. As there is no documented way to clean this up,
+  // we test the handle first. As the HEAP struct is not documented, we select
+  // a sane value of bytes to check.
+  // There are reasons not to use IsBadReadPtr
+  // (https://msdn.microsoft.com/en-us/library/aa366713(VS.85).aspx)
+  // however as we are testing if the memory is already unreadable, these
+  // reasons are not relevant.
+  if (::IsBadReadPtr(heap_info->heap_id, 0x100)) {

[danakj, 2016/08/15 23:42:03]

How confident are you that 0x100 is a good size? Why not just 1?

[danakj, 2016/08/15 23:43:51]

Also it's not clear to me after reading your patch description and the comment
here why this call is needed at all. The description says that HeapLock will
fail if the heap is the deleted CSRSS one.

And this *is* racey, a heap could become bad after this returned true, right?

[liamjm (20p), 2016/10/11 18:57:31]

Obsolete as this code has been removed.

[liamjm (20p), 2016/10/11 18:57:31]

Yes this using this function is racey. The idea was that it is not racey under
the circumstances present at this point. 

However, it doesn't really offer any protection that HeapLock doesn't provide,
so it can be removed.

+    return false;
+  }
+  // This is implicitly checks certain aspects of the HEAP structure, such as

[danakj, 2016/08/15 23:43:51]

nit: This implicitly checks

[liamjm (20p), 2016/10/11 18:57:31]

Done.

+  // the signature. If this passes, we assume that this heap is valid and is
+  // not the one owned by CSRSS.
+  if (!::HeapLock(heap_info->heap_id)) {
+    return false;
+  }
+
   PROCESS_HEAP_ENTRY heap_entry;
   heap_entry.lpData = nullptr;
   // Walk over all the entries in this heap.