winheap_dump: handle errors gracefully

base/trace_event/winheap_dump_provider_win.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/trace_event/winheap_dump_provider_win.h"
 
 #include <windows.h>
 
 #include "base/debug/profiler.h"
 #include "base/strings/string_util.h"
@@ skipping 68 matching lines @@
 
   // Skip the pointer to the heap array to avoid accounting the memory used by
   // this dump provider.
   std::set<void*> block_to_skip;
   block_to_skip.insert(all_heaps.get());
 
   // Retrieves some metrics about each heap.
   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;
 }
 
 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.
   while (::HeapWalk(heap_info->heap_id, &heap_entry) != FALSE) {
     if (block_to_skip.count(heap_entry.lpData) == 1)
       continue;
     if ((heap_entry.wFlags & PROCESS_HEAP_ENTRY_BUSY) != 0) {
       heap_info->allocated_size += heap_entry.cbData;
       heap_info->block_count++;
     } else if ((heap_entry.wFlags & PROCESS_HEAP_REGION) != 0) {
       heap_info->committed_size += heap_entry.Region.dwCommittedSize;
     }
   }
   CHECK(::HeapUnlock(heap_info->heap_id) == TRUE);
   return true;
 }
 
 }  // namespace trace_event
 }  // namespace base