Skip to content

Commit

Permalink
[tracing] Sanitize process memory dump names for background mode
Browse files Browse the repository at this point in the history
For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
TBR=shess@chromium.org, jochen@chromium.org

Review-Url: https://codereview.chromium.org/2006943003
Cr-Commit-Position: refs/heads/master@{#397918}
  • Loading branch information
ssiddhartha authored and Commit bot committed Jun 4, 2016
1 parent 40451c5 commit 448e5ed
Show file tree
Hide file tree
Showing 26 changed files with 303 additions and 74 deletions.
2 changes: 2 additions & 0 deletions base/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -903,6 +903,8 @@ component("base") {
"trace_event/memory_dump_request_args.h",
"trace_event/memory_dump_session_state.cc",
"trace_event/memory_dump_session_state.h",
"trace_event/memory_infra_background_whitelist.cc",
"trace_event/memory_infra_background_whitelist.h",
"trace_event/process_memory_dump.cc",
"trace_event/process_memory_dump.h",
"trace_event/process_memory_maps.cc",
Expand Down
3 changes: 2 additions & 1 deletion base/trace_event/java_heap_dump_provider_android_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ namespace trace_event {

TEST(JavaHeapDumpProviderTest, JavaHeapDump) {
auto jhdp = JavaHeapDumpProvider::GetInstance();
std::unique_ptr<ProcessMemoryDump> pmd(new ProcessMemoryDump(nullptr));
MemoryDumpArgs dump_args = {MemoryDumpLevelOfDetail::DETAILED};
std::unique_ptr<ProcessMemoryDump> pmd(
new ProcessMemoryDump(nullptr, dump_args));

jhdp->OnMemoryDump(dump_args, pmd.get());
}
Expand Down
7 changes: 7 additions & 0 deletions base/trace_event/memory_allocator_dump.cc
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,13 @@ void MemoryAllocatorDump::AddScalarF(const char* name,
void MemoryAllocatorDump::AddString(const char* name,
const char* units,
const std::string& value) {
// String attributes are disabled in background mode.
if (process_memory_dump_->dump_args().level_of_detail ==
MemoryDumpLevelOfDetail::BACKGROUND) {
NOTREACHED();
return;
}

attributes_->BeginDictionary(name);
attributes_->SetString("type", kTypeString);
attributes_->SetString("units", units);
Expand Down
5 changes: 3 additions & 2 deletions base/trace_event/memory_allocator_dump_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ TEST(MemoryAllocatorDumpTest, GuidGeneration) {

TEST(MemoryAllocatorDumpTest, DumpIntoProcessMemoryDump) {
FakeMemoryAllocatorDumpProvider fmadp;
ProcessMemoryDump pmd(new MemoryDumpSessionState);
MemoryDumpArgs dump_args = {MemoryDumpLevelOfDetail::DETAILED};
ProcessMemoryDump pmd(new MemoryDumpSessionState, dump_args);

fmadp.OnMemoryDump(dump_args, &pmd);

Expand Down Expand Up @@ -176,7 +176,8 @@ TEST(MemoryAllocatorDumpTest, DumpIntoProcessMemoryDump) {
#if !defined(NDEBUG) && !defined(OS_ANDROID) && !defined(OS_IOS)
TEST(MemoryAllocatorDumpTest, ForbidDuplicatesDeathTest) {
FakeMemoryAllocatorDumpProvider fmadp;
ProcessMemoryDump pmd(new MemoryDumpSessionState);
MemoryDumpArgs dump_args = {MemoryDumpLevelOfDetail::DETAILED};
ProcessMemoryDump pmd(new MemoryDumpSessionState, dump_args);
pmd.CreateAllocatorDump("foo_allocator");
pmd.CreateAllocatorDump("bar_allocator/heap");
ASSERT_DEATH(pmd.CreateAllocatorDump("foo_allocator"), "");
Expand Down
35 changes: 8 additions & 27 deletions base/trace_event/memory_dump_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@
#include "base/memory/ptr_util.h"
#include "base/threading/thread.h"
#include "base/threading/thread_task_runner_handle.h"
#include "base/timer/timer.h"
#include "base/trace_event/heap_profiler.h"
#include "base/trace_event/heap_profiler_allocation_context_tracker.h"
#include "base/trace_event/heap_profiler_stack_frame_deduplicator.h"
#include "base/trace_event/heap_profiler_type_name_deduplicator.h"
#include "base/trace_event/malloc_dump_provider.h"
#include "base/trace_event/memory_dump_provider.h"
#include "base/trace_event/memory_dump_session_state.h"
#include "base/trace_event/memory_infra_background_whitelist.h"
#include "base/trace_event/process_memory_dump.h"
#include "base/trace_event/trace_event.h"
#include "base/trace_event/trace_event_argument.h"
Expand All @@ -49,15 +49,6 @@ const unsigned char kTraceEventArgTypes[] = {TRACE_VALUE_TYPE_CONVERTABLE};
StaticAtomicSequenceNumber g_next_guid;
MemoryDumpManager* g_instance_for_testing = nullptr;

// The names of dump providers whitelisted for background tracing. Dump
// providers can be added here only if the background mode dump has very
// less performance and memory overhead.
const char* const kDumpProviderWhitelist[] = {
// TODO(ssid): Fill this list with dump provider names which support
// background mode, crbug.com/613198.
nullptr, // End of list marker.
};

// Callback wrapper to hook upon the completion of RequestGlobalDump() and
// inject trace markers.
void OnGlobalDumpDone(MemoryDumpCallback wrapped_callback,
Expand Down Expand Up @@ -100,16 +91,6 @@ struct SessionStateConvertableProxy : public ConvertableToTraceFormat {
GetterFunctPtr const getter_function;
};

// Checks if the name is in the given |list|. Last element of the list should be
// an empty string.
bool IsNameInList(const char* name, const char* const* list) {
for (size_t i = 0; list[i] != nullptr; ++i) {
if (strcmp(name, list[i]) == 0)
return true;
}
return false;
}

} // namespace

// static
Expand Down Expand Up @@ -150,7 +131,6 @@ MemoryDumpManager::MemoryDumpManager()
: delegate_(nullptr),
is_coordinator_(false),
memory_tracing_enabled_(0),
dump_provider_whitelist_(kDumpProviderWhitelist),
tracing_process_id_(kInvalidTracingProcessId),
dumper_registrations_ignored_for_testing_(false),
heap_profiling_enabled_(false) {
Expand Down Expand Up @@ -274,8 +254,7 @@ void MemoryDumpManager::RegisterDumpProviderInternal(
if (dumper_registrations_ignored_for_testing_)
return;

bool whitelisted_for_background_mode =
IsNameInList(name, dump_provider_whitelist_);
bool whitelisted_for_background_mode = IsMemoryDumpProviderWhitelisted(name);
scoped_refptr<MemoryDumpProviderInfo> mdpinfo =
new MemoryDumpProviderInfo(mdp, name, std::move(task_runner), options,
whitelisted_for_background_mode);
Expand Down Expand Up @@ -561,9 +540,10 @@ void MemoryDumpManager::InvokeOnMemoryDump(
// process), non-zero when the coordinator process creates dumps on behalf
// of child processes (see crbug.com/461788).
ProcessId target_pid = mdpinfo->options.target_pid;
ProcessMemoryDump* pmd =
pmd_async_state->GetOrCreateMemoryDumpContainerForProcess(target_pid);
MemoryDumpArgs args = {pmd_async_state->req_args.level_of_detail};
ProcessMemoryDump* pmd =
pmd_async_state->GetOrCreateMemoryDumpContainerForProcess(target_pid,
args);
bool dump_successful = mdpinfo->dump_provider->OnMemoryDump(args, pmd);
mdpinfo->consecutive_failures =
dump_successful ? 0 : mdpinfo->consecutive_failures + 1;
Expand Down Expand Up @@ -770,11 +750,12 @@ MemoryDumpManager::ProcessMemoryDumpAsyncState::~ProcessMemoryDumpAsyncState() {
}

ProcessMemoryDump* MemoryDumpManager::ProcessMemoryDumpAsyncState::
GetOrCreateMemoryDumpContainerForProcess(ProcessId pid) {
GetOrCreateMemoryDumpContainerForProcess(ProcessId pid,
const MemoryDumpArgs& dump_args) {
auto iter = process_dumps.find(pid);
if (iter == process_dumps.end()) {
std::unique_ptr<ProcessMemoryDump> new_pmd(
new ProcessMemoryDump(session_state));
new ProcessMemoryDump(session_state, dump_args));
iter = process_dumps.insert(std::make_pair(pid, std::move(new_pmd))).first;
}
return iter->second.get();
Expand Down
12 changes: 3 additions & 9 deletions base/trace_event/memory_dump_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -141,11 +141,6 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
dumper_registrations_ignored_for_testing_ = ignored;
}

// Resets the dump provider whitelist to the list given.
void set_dump_provider_whitelist_for_testing(const char* const* list) {
dump_provider_whitelist_ = list;
}

private:
friend std::default_delete<MemoryDumpManager>; // For the testing instance.
friend struct DefaultSingletonTraits<MemoryDumpManager>;
Expand Down Expand Up @@ -230,7 +225,9 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
~ProcessMemoryDumpAsyncState();

// Gets or creates the memory dump container for the given target process.
ProcessMemoryDump* GetOrCreateMemoryDumpContainerForProcess(ProcessId pid);
ProcessMemoryDump* GetOrCreateMemoryDumpContainerForProcess(
ProcessId pid,
const MemoryDumpArgs& dump_args);

// A map of ProcessId -> ProcessMemoryDump, one for each target process
// being dumped from the current process. Typically each process dumps only
Expand Down Expand Up @@ -365,9 +362,6 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
// affinity.
std::unique_ptr<Thread> dump_thread_;

// List of names of the dump providers whitelisted for background mode.
const char* const* dump_provider_whitelist_;

// The unique id of the child process. This is created only for tracing and is
// expected to be valid only when tracing is enabled.
uint64_t tracing_process_id_;
Expand Down
3 changes: 2 additions & 1 deletion base/trace_event/memory_dump_manager_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "base/threading/thread.h"
#include "base/threading/thread_task_runner_handle.h"
#include "base/trace_event/memory_dump_provider.h"
#include "base/trace_event/memory_infra_background_whitelist.h"
#include "base/trace_event/process_memory_dump.h"
#include "base/trace_event/trace_buffer.h"
#include "base/trace_event/trace_config_memory_test_util.h"
Expand Down Expand Up @@ -1112,7 +1113,7 @@ TEST_F(MemoryDumpManagerTest, UnregisterAndDeleteDumpProviderSoonDuringDump) {

TEST_F(MemoryDumpManagerTest, TestWhitelistingMDP) {
InitializeMemoryDumpManager(false /* is_coordinator */);
mdm_->set_dump_provider_whitelist_for_testing(kTestMDPWhitelist);
SetDumpProviderWhitelistForTesting(kTestMDPWhitelist);
std::unique_ptr<MockMemoryDumpProvider> mdp1(new MockMemoryDumpProvider);
RegisterDumpProvider(mdp1.get());
std::unique_ptr<MockMemoryDumpProvider> mdp2(new MockMemoryDumpProvider);
Expand Down
6 changes: 0 additions & 6 deletions base/trace_event/memory_dump_provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,6 @@ namespace trace_event {

class ProcessMemoryDump;

// Args passed to OnMemoryDump(). This is to avoid rewriting all the subclasses
// in the codebase when extending the MemoryDumpProvider API.
struct MemoryDumpArgs {
MemoryDumpLevelOfDetail level_of_detail;
};

// The contract interface that memory dump providers must implement.
class BASE_EXPORT MemoryDumpProvider {
public:
Expand Down
7 changes: 7 additions & 0 deletions base/trace_event/memory_dump_request_args.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@ struct BASE_EXPORT MemoryDumpRequestArgs {
MemoryDumpLevelOfDetail level_of_detail;
};

// Args for ProcessMemoryDump and passed to OnMemoryDump calls for memory dump
// providers. Dump providers are expected to read the args for creating dumps.
struct MemoryDumpArgs {
// Specifies how detailed the dumps should be.
MemoryDumpLevelOfDetail level_of_detail;
};

using MemoryDumpCallback = Callback<void(uint64_t dump_guid, bool success)>;

BASE_EXPORT const char* MemoryDumpTypeToString(const MemoryDumpType& dump_type);
Expand Down
88 changes: 88 additions & 0 deletions base/trace_event/memory_infra_background_whitelist.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
// Copyright 2016 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/memory_infra_background_whitelist.h"

#include <ctype.h>
#include <string.h>

#include <string>

namespace base {
namespace trace_event {
namespace {

// The names of dump providers whitelisted for background tracing. Dump
// providers can be added here only if the background mode dump has very
// less performance and memory overhead.
const char* const kDumpProviderWhitelist[] = {
// TODO(ssid): Fill this list with dump provider names which support
// background mode, crbug.com/613198.
nullptr // End of list marker.
};

// A list of string names that are allowed for the memory allocator dumps in
// background mode.
const char* const kAllocatorDumpNameWhitelist[] = {
// TODO(ssid): Fill this list with dump names, crbug.com/613198.
nullptr // End of list marker.
};

const char* const* g_dump_provider_whitelist = kDumpProviderWhitelist;
const char* const* g_allocator_dump_name_whitelist =
kAllocatorDumpNameWhitelist;

} // namespace

bool IsMemoryDumpProviderWhitelisted(const char* mdp_name) {
for (size_t i = 0; g_dump_provider_whitelist[i] != nullptr; ++i) {
if (strcmp(mdp_name, g_dump_provider_whitelist[i]) == 0)
return true;
}
return false;
}

bool IsMemoryAllocatorDumpNameWhitelisted(const std::string& name) {
// Remove special characters, numbers (including hexadecimal which are marked
// by '0x') from the given string.
const size_t length = name.size();
std::string stripped_str;
stripped_str.reserve(length);
bool parsing_hex = false;
for (size_t i = 0; i < length; ++i) {
if (parsing_hex) {
if (isxdigit(name[i])) {
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]) ||
(name[i] == '/' && stripped_str.back() != '/')) {
// Do not add successive '/'(s) in |stripped_str|.
stripped_str.push_back(name[i]);
}
}

for (size_t i = 0; g_allocator_dump_name_whitelist[i] != nullptr; ++i) {
if (stripped_str == g_allocator_dump_name_whitelist[i]) {
return true;
}
}
return false;
}

void SetDumpProviderWhitelistForTesting(const char* const* list) {
g_dump_provider_whitelist = list;
}

void SetAllocatorDumpNameWhitelistForTesting(const char* const* list) {
g_allocator_dump_name_whitelist = list;
}

} // namespace trace_event
} // namespace base
33 changes: 33 additions & 0 deletions base/trace_event/memory_infra_background_whitelist.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright 2016 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.

#ifndef BASE_TRACE_EVENT_MEMORY_INFRA_BACKGROUND_WHITELIST_H_
#define BASE_TRACE_EVENT_MEMORY_INFRA_BACKGROUND_WHITELIST_H_

// This file contains the whitelists for background mode to limit the tracing
// overhead and remove sensitive information from traces.

#include <string>

#include "base/base_export.h"

namespace base {
namespace trace_event {

// Checks if the given |mdp_name| is in the whitelist.
bool BASE_EXPORT IsMemoryDumpProviderWhitelisted(const char* mdp_name);

// Checks if the given |name| matches any of the whitelisted patterns.
bool BASE_EXPORT IsMemoryAllocatorDumpNameWhitelisted(const std::string& name);

// The whitelist is replaced with the given list for tests. The last element of
// the list must be nullptr.
void BASE_EXPORT SetDumpProviderWhitelistForTesting(const char* const* list);
void BASE_EXPORT
SetAllocatorDumpNameWhitelistForTesting(const char* const* list);

} // namespace trace_event
} // namespace base

#endif // BASE_TRACE_EVENT_MEMORY_INFRA_BACKGROUND_WHITELIST_H_
Loading

0 comments on commit 448e5ed

Please sign in to comment.