Replace SharedMemory::UniqueID usages with SharedMemoryHandle's guid

base/memory/shared_memory_tracker.cc

Index: base/memory/shared_memory_tracker.cc
diff --git a/base/memory/shared_memory_tracker.cc b/base/memory/shared_memory_tracker.cc
index 8613f595336ada19357c900cf0ab9bf4d243b7f4..c448562862fca51e26d2f2267fe72400d691f550 100644
--- a/base/memory/shared_memory_tracker.cc
+++ b/base/memory/shared_memory_tracker.cc
@@ -5,18 +5,11 @@
 #include "base/memory/shared_memory_tracker.h"
 
 #include "base/memory/shared_memory.h"
-#include "base/strings/stringprintf.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "base/trace_event/process_memory_dump.h"
 
 namespace base {
 
-SharedMemoryTracker::Usage::Usage() = default;
-
-SharedMemoryTracker::Usage::Usage(const Usage& rhs) = default;
-
-SharedMemoryTracker::Usage::~Usage() = default;
-
 // static
 SharedMemoryTracker* SharedMemoryTracker::GetInstance() {
   static SharedMemoryTracker* instance = new SharedMemoryTracker;
@@ -25,19 +18,8 @@ SharedMemoryTracker* SharedMemoryTracker::GetInstance() {
 
 void SharedMemoryTracker::IncrementMemoryUsage(
     const SharedMemory& shared_memory) {
-  Usage usage;
-  // |shared_memory|'s unique ID must be generated here and it'd be too late at
-  // OnMemoryDump. An ID is generated with a SharedMemoryHandle, but the handle
-  // might already be closed at that time. Now IncrementMemoryUsage is called
-  // just after mmap and the handle must live then. See the discussion at
-  // crbug.com/604726#c30.
-  SharedMemory::UniqueId id;
-  if (!shared_memory.GetUniqueId(&id))
-    return;
-  usage.unique_id = id;
-  usage.size = shared_memory.mapped_size();
   AutoLock hold(usages_lock_);
-  usages_[&shared_memory] = usage;
+  usages_[&shared_memory] = shared_memory.mapped_size();
 }
 
 void SharedMemoryTracker::DecrementMemoryUsage(
@@ -48,31 +30,35 @@ void SharedMemoryTracker::DecrementMemoryUsage(
 
 bool SharedMemoryTracker::OnMemoryDump(const trace_event::MemoryDumpArgs& args,
                                        trace_event::ProcessMemoryDump* pmd) {
-  std::unordered_map<SharedMemory::UniqueId, size_t, SharedMemory::UniqueIdHash>
-      sizes;
+  std::unordered_map<const SharedMemory*, size_t> usages;

[Primiano Tucci (use gerrit), 2017/05/10 18:33:55]

see brett 's PSA on [chromium-dev] Please avoid std::unordered_map, looks like
unordered_map is mostly a lose-lose.

[hajimehoshi, 2017/05/11 10:26:00]

Thanks, I adopted std::map since I couldn't estimate the number of elements.

   {
     AutoLock hold(usages_lock_);
-    for (const auto& usage : usages_)
-      sizes[usage.second.unique_id] += usage.second.size;
+    usages = usages_;
   }
-  for (auto& size : sizes) {
-    const SharedMemory::UniqueId& id = size.first;
-    std::string dump_name = StringPrintf("%s/%lld.%lld", "shared_memory",
-                                         static_cast<long long>(id.first),
-                                         static_cast<long long>(id.second));
-    auto guid = trace_event::MemoryAllocatorDumpGuid(dump_name);
+  for (auto& usage : usages) {
+    auto guid = usage.first->GetGUID();

[Primiano Tucci (use gerrit), 2017/05/10 18:33:55]

This seems racy. since you don't own the SharedMemory object, at this point you
have no guarantee that the SharedMemory* pointer will be valid. If you get a
call in the meantime DecrementMemoryUsage this will crash for UAF.
I think you actually have to iterate while holding the lock, and maybe populate
a vector of (GUID, address, size) tuples, and then fill the dump after releasing
the lock (which I think is what implicitly Dana was suggesting)

[hajimehoshi, 2017/05/11 10:26:00]

Done.

+    // TODO(hajimehoshi): As passing ID across mojo is not implemented yet
+    // (crbug/713763), ID can be empty.
+    if (guid.is_empty())
+      continue;
+    std::string dump_name = "shared_memory/" + guid.ToString();
+    auto dump_guid = trace_event::MemoryAllocatorDumpGuid(dump_name);
+    // The dump might already be created on single-process mode.
     trace_event::MemoryAllocatorDump* local_dump =
-        pmd->CreateAllocatorDump(dump_name);
+        pmd->GetAllocatorDump(dump_name);
+    if (local_dump)
+      continue;
+    local_dump = pmd->GetOrCreateAllocatorDump(dump_name);
     // TODO(hajimehoshi): The size is not resident size but virtual size so far.
     // Fix this to record resident size.
     local_dump->AddScalar(trace_event::MemoryAllocatorDump::kNameSize,
                           trace_event::MemoryAllocatorDump::kUnitsBytes,
-                          size.second);
+                          usage.second);
     trace_event::MemoryAllocatorDump* global_dump =
-        pmd->CreateSharedGlobalAllocatorDump(guid);
+        pmd->CreateSharedGlobalAllocatorDump(dump_guid);
     global_dump->AddScalar(trace_event::MemoryAllocatorDump::kNameSize,
                            trace_event::MemoryAllocatorDump::kUnitsBytes,
-                           size.second);
+                           usage.second);
     // TOOD(hajimehoshi): Detect which the shared memory comes from browser,
     // renderer or GPU process.
     // TODO(hajimehoshi): Shared memory reported by GPU and discardable is