[tracing] Sanitize process memory dumps for background mode

base/trace_event/process_memory_dump.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/process_memory_dump.h"
 
+#include <ctype.h>
 #include <errno.h>
 
 #include <vector>
 
 #include "base/memory/ptr_util.h"
 #include "base/process/process_metrics.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/heap_profiler_heap_dump_writer.h"
+#include "base/trace_event/memory_dump_request_args.h"
 #include "base/trace_event/process_memory_totals.h"
 #include "base/trace_event/trace_event_argument.h"
 #include "build/build_config.h"
 
 #if defined(OS_IOS)
 #include <sys/sysctl.h>
 #endif
 
 #if defined(OS_POSIX)
 #include <sys/mman.h>
@@ skipping 14 matching lines @@
     const MemoryAllocatorDumpGuid& guid) {
   return "global/" + guid.ToString();
 }
 
 #if defined(COUNT_RESIDENT_BYTES_SUPPORTED)
 size_t GetSystemPageCount(size_t mapped_size, size_t page_size) {
   return (mapped_size + page_size - 1) / page_size;
 }
 #endif
 
+// A list of string names that are allowed for the allocator dumps in background
+// mode.
+const char* const kDumpNameWhitelist[] = {

[Primiano Tucci (use gerrit), 2016/05/27 17:23:35]

Ok I think at this point we want a memory_infra_background_whitelist.h/cc file
so we put these constants (and the _for_testing logic) for both MDM (from the
other patch) and this one.

[ssid, 2016/05/31 22:33:20]

Done.

+    // TODO(ssid): Fill this list with dump names, crbug.com/613198.
+    "",
+};
+
+// Check if the string is in the whitelist.
+bool IsNameValidForBackgroundMode(const std::string& name,

[Primiano Tucci (use gerrit), 2016/05/27 17:23:35]

s/Valid/Whitelisted/ ... which makes then the comment above superfluos.

[ssid, 2016/05/31 22:33:20]

Acknowledged.

+                                  const char* whitelisted_name_for_testing) {

[Primiano Tucci (use gerrit), 2016/05/27 17:23:35]

see my comment in the other cl, having to pass this testing arg here is very
weird. Let's use the same approach (null terminated array) and having the
for_testing_ pointer.

[ssid, 2016/05/31 22:33:20]

Done.

+  static size_t kListSize = arraysize(kDumpNameWhitelist);
+
+  // Remove special characters, numbers (including hexadecimal which are makred

[Dmitry Skiba, 2016/05/31 06:48:46]

*marked

[ssid, 2016/05/31 22:33:20]

Done.

+  // by '0x') from the given string.
+  const size_t length = name.size();
+  std::unique_ptr<char[]> stripped_str(new char[length]);

[Dmitry Skiba, 2016/05/31 06:48:46]

Uhm, why not std::string? And push_back() instead of stripped_str[str_index++],
== instead of strcmp() == 0.

[ssid, 2016/05/31 22:33:20]

Done.

+  size_t str_index = 0;
+  bool parsing_hex = false;
+  for (size_t i = 0; i < length; ++i) {
+    if (parsing_hex) {
+      if (isxdigit(name[i])) {

[Primiano Tucci (use gerrit), 2016/05/27 17:23:35]

instead of having this state machine here, can you do something simpler and say
that: contiguous sequences of digits are collapsed into "#" (or whatever
character)
So you can write your patterns simply as foo/bar/0x#/baz

[ssid, 2016/05/27 17:42:48]

Sorry, I don't get it.
Yeah I would still need this state machine here. This is to identify the hex
numbers, which have letters form normal numbers. If I am in hex mode I would
have to skip "abcdef" if not I should include characters. This is even if I
replace the number with #.
Also if this is the case:
Strings with hex will look like:
"foo/bar_0x#/baz"
without will look like:
"foo/bar_#/baz"

Yes with a # the whitelist would looks better. Are you sure we need the #?

[Primiano Tucci (use gerrit), 2016/05/27 17:51:19]

Ok sorry this was a stupid comment from my side. I forgot that hex digits
contain also letter a-f :)
Ignore my comment. This makes sense.
Just not sure on the isalpha part below. Why stripping hex is not enough?

[ssid, 2016/05/31 22:33:20]

isalpha is there because there could be cases where name is
"foo/bar__0xab/baz_12"
We would have to write this as "foo/bar__/baz_".
Maybe if you like the strings to look pretty, i will leave the '/' and
alphabets?
So, it will look like "foo/bar/baz".

[Primiano Tucci (use gerrit), 2016/06/02 20:24:04]

Ah right, I think it's ok.

+        continue;
+      } else {
+        parsing_hex = false;
+      }
+    }
+    if (i + 1 < length && name[i] == '0' && name[i + 1] == 'x') {
+      parsing_hex = true;
+      ++i;
+    } else if (isalpha(name[i])) {
+      stripped_str[str_index++] = name[i];
+    }
+  }
+  stripped_str[str_index] = '\0';
+
+  if (whitelisted_name_for_testing)
+    return strcmp(stripped_str.get(), whitelisted_name_for_testing) == 0;
+
+  for (size_t i = 0; i < kListSize; ++i) {
+    if (strcmp(stripped_str.get(), kDumpNameWhitelist[i]) == 0)
+      return true;
+  }
+  return false;
+}
+
 }  // namespace
 
 #if defined(COUNT_RESIDENT_BYTES_SUPPORTED)
 // static
 size_t ProcessMemoryDump::GetSystemPageSize() {
 #if defined(OS_IOS)
   // On iOS, getpagesize() returns the user page sizes, but for allocating
   // arrays for mincore(), kernel page sizes is needed. sysctlbyname() should
   // be used for this. Refer to crbug.com/542671 and Apple rdar://23651782
   int pagesize;
@@ skipping 82 matching lines @@
   DCHECK(!failure);
   if (failure) {
     total_resident_size = 0;
     LOG(ERROR) << "CountResidentBytes failed. The resident size is invalid";
   }
   return total_resident_size;
 }
 #endif  // defined(COUNT_RESIDENT_BYTES_SUPPORTED)
 
 ProcessMemoryDump::ProcessMemoryDump(
-    scoped_refptr<MemoryDumpSessionState> session_state)
+    scoped_refptr<MemoryDumpSessionState> session_state,
+    MemoryDumpLevelOfDetail level_of_detail)
     : has_process_totals_(false),
       has_process_mmaps_(false),
-      session_state_(std::move(session_state)) {}
+      session_state_(std::move(session_state)),
+      level_of_detail_(level_of_detail),
+      whitelisted_name_for_testing_(nullptr) {
+  if (level_of_detail_ == MemoryDumpLevelOfDetail::BACKGROUND) {
+    dummy_mad_.reset(new MemoryAllocatorDump("dummy", this));
+  }
+}
 
 ProcessMemoryDump::~ProcessMemoryDump() {}
 
 MemoryAllocatorDump* ProcessMemoryDump::CreateAllocatorDump(
     const std::string& absolute_name) {
   return AddAllocatorDumpInternal(
       WrapUnique(new MemoryAllocatorDump(absolute_name, this)));
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::CreateAllocatorDump(
     const std::string& absolute_name,
     const MemoryAllocatorDumpGuid& guid) {
   return AddAllocatorDumpInternal(
       WrapUnique(new MemoryAllocatorDump(absolute_name, this, guid)));
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::AddAllocatorDumpInternal(
     std::unique_ptr<MemoryAllocatorDump> mad) {
+  // In background mode return dummy dump if invalid dump name is given.
+  if (level_of_detail_ == MemoryDumpLevelOfDetail::BACKGROUND &&
+      !IsNameValidForBackgroundMode(mad->absolute_name(),
+                                    whitelisted_name_for_testing_)) {
+    return dummy_mad_.get();

[Primiano Tucci (use gerrit), 2016/05/27 17:23:35]

maybe NOTREACHED also here to help debugging?

[ssid, 2016/05/31 22:33:20]

I have changed the tests to use AddAllocatorDumpInternal instead of
CreateAllocatorDump and moved the dcheck to create methods. This is because I
can just test the non-insertion of the strings. Also tests that the black hole
dump is returned.
If not, I could create a IsValidDumpName method and just test that one, without
checking the return value being black hole.

[Primiano Tucci (use gerrit), 2016/06/02 20:24:03]

You can just check the allocator_dumps().count("name") as the black hole is not
in the map

+  }
+
   auto insertion_result = allocator_dumps_.insert(
       std::make_pair(mad->absolute_name(), std::move(mad)));
   MemoryAllocatorDump* inserted_mad = insertion_result.first->second.get();
   DCHECK(insertion_result.second) << "Duplicate name: "
                                   << inserted_mad->absolute_name();
   return inserted_mad;
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::GetAllocatorDump(
     const std::string& absolute_name) const {
   auto it = allocator_dumps_.find(absolute_name);
-  return it == allocator_dumps_.end() ? nullptr : it->second.get();
+  if (it != allocator_dumps_.end())
+    return it->second.get();

[Primiano Tucci (use gerrit), 2016/05/27 17:23:35]

should you just leave this as it is and return nullptr?

[ssid, 2016/05/27 17:42:48]

I can't do that. Some dump providers assume that the dump is created because
they created from previous func and they just do a get and use it. But, if we
created a dummy one, we will crash if we return null. This is safer on the field

[Primiano Tucci (use gerrit), 2016/06/02 20:24:03]

Acknowledged.

+  return level_of_detail_ == MemoryDumpLevelOfDetail::BACKGROUND
+             ? dummy_mad_.get()
+             : nullptr;
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::GetOrCreateAllocatorDump(
     const std::string& absolute_name) {
   MemoryAllocatorDump* mad = GetAllocatorDump(absolute_name);
   return mad ? mad : CreateAllocatorDump(absolute_name);
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::CreateSharedGlobalAllocatorDump(
     const MemoryAllocatorDumpGuid& guid) {
+  // Global dumps are disabled in background mode.
+  if (level_of_detail_ == MemoryDumpLevelOfDetail::BACKGROUND)

[Primiano Tucci (use gerrit), 2016/05/27 17:23:35]

I don't think global dumps are a problem right? the guid is a uint64 hardly
contains any pii :)

[ssid, 2016/05/27 17:42:48]

Global dumps are not required. Anyway we report just the totals, it just makes
the trace bigger with no information.
Is there any specific reason we should have them?

Also the global dumps create dumps with hex numbers and they dont use 0x. Makes
it hard to parse them. I would have to add a state that if "0x" or "__" then we
are in hex mode.

+    return dummy_mad_.get();
+
   // A shared allocator dump can be shared within a process and the guid could
   // have been created already.
   MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
   if (mad) {
     // The weak flag is cleared because this method should create a non-weak
     // dump.
     mad->clear_flags(MemoryAllocatorDump::Flags::WEAK);
     return mad;
   }
   return CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid);
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::CreateWeakSharedGlobalAllocatorDump(
     const MemoryAllocatorDumpGuid& guid) {
+  // Global dumps are disabled in background mode.
+  if (level_of_detail_ == MemoryDumpLevelOfDetail::BACKGROUND)

[Primiano Tucci (use gerrit), 2016/05/27 17:23:35]

ditto don't think you need this

+    return dummy_mad_.get();
+
   MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
   if (mad)
     return mad;
   mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid);
   mad->set_flags(MemoryAllocatorDump::Flags::WEAK);
   return mad;
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::GetSharedGlobalAllocatorDump(
     const MemoryAllocatorDumpGuid& guid) const {
@@ skipping 107 matching lines @@
 }
 
 void ProcessMemoryDump::AddOwnershipEdge(
     const MemoryAllocatorDumpGuid& source,
     const MemoryAllocatorDumpGuid& target) {
   AddOwnershipEdge(source, target, 0 /* importance */);
 }
 
 void ProcessMemoryDump::AddSuballocation(const MemoryAllocatorDumpGuid& source,
                                          const std::string& target_node_name) {
+  // Do not create new dumps for suballocations in background mode.
+  if (level_of_detail_ == MemoryDumpLevelOfDetail::BACKGROUND)

[Primiano Tucci (use gerrit), 2016/05/27 17:23:35]

ditto

[ssid, 2016/05/27 17:42:48]

Why create unnecessary dumps when we are not interested in them in background
mode?

+    return;
+
   std::string child_mad_name = target_node_name + "/__" + source.ToString();
   MemoryAllocatorDump* target_child_mad = CreateAllocatorDump(child_mad_name);
   AddOwnershipEdge(source, target_child_mad->guid());
 }
 
 }  // namespace trace_event
 }  // namespace base