From c1d1b7554e800c0bb446eea791b84493d27aee9d Mon Sep 17 00:00:00 2001 From: grt Date: Sat, 5 Sep 2015 06:26:41 -0700 Subject: [PATCH] Platform-specific prune state storage. BUG=496744 TBR=sky@chromium.org Review URL: https://codereview.chromium.org/1243293003 Cr-Commit-Position: refs/heads/master@{#347571} --- chrome/browser/BUILD.gn | 3 + .../safe_browsing/incident_reporting/BUILD.gn | 12 ++ .../incident_reporting_service_unittest.cc | 13 ++ .../platform_state_store.cc | 200 ++++++++++++++++++ .../incident_reporting/platform_state_store.h | 85 ++++++++ .../platform_state_store_unittest.cc | 77 +++++++ .../platform_state_store_win.cc | 103 +++++++++ .../platform_state_store_win_unittest.cc | 179 ++++++++++++++++ .../incident_reporting/state_store.cc | 30 ++- .../incident_reporting/state_store.h | 10 + .../incident_reporting/state_store_data.proto | 28 +++ .../state_store_unittest.cc | 45 +++- chrome/chrome_browser.gypi | 23 ++ chrome/chrome_tests_unit.gypi | 2 + tools/metrics/histograms/histograms.xml | 31 ++- 15 files changed, 835 insertions(+), 6 deletions(-) create mode 100644 chrome/browser/safe_browsing/incident_reporting/BUILD.gn create mode 100644 chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc create mode 100644 chrome/browser/safe_browsing/incident_reporting/platform_state_store.h create mode 100644 chrome/browser/safe_browsing/incident_reporting/platform_state_store_unittest.cc create mode 100644 chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc create mode 100644 chrome/browser/safe_browsing/incident_reporting/platform_state_store_win_unittest.cc create mode 100644 chrome/browser/safe_browsing/incident_reporting/state_store_data.proto diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn index 46815ea119c18..9b10cd86783e4 100644 --- a/chrome/browser/BUILD.gn +++ b/chrome/browser/BUILD.gn @@ -483,6 +483,9 @@ source_set("browser") { ".", "//chrome") deps += [ "//chrome/common/safe_browsing:proto" ] + if (is_win) { + deps += [ "//chrome/browser/safe_browsing/incident_reporting:state_store_data_proto" ] + } } else if (safe_browsing_mode == 3) { sources += rebase_path( gypi_values.chrome_browser_safe_browsing_mobile_extended_sources, diff --git a/chrome/browser/safe_browsing/incident_reporting/BUILD.gn b/chrome/browser/safe_browsing/incident_reporting/BUILD.gn new file mode 100644 index 0000000000000..d8ec0ef8438b4 --- /dev/null +++ b/chrome/browser/safe_browsing/incident_reporting/BUILD.gn @@ -0,0 +1,12 @@ +# 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. + +import("//third_party/protobuf/proto_library.gni") + +# GYP version: chrome/chrome_browser.gypi:incident_reporting_state_store_data_proto +proto_library("state_store_data_proto") { + sources = [ + "state_store_data.proto", + ] +} diff --git a/chrome/browser/safe_browsing/incident_reporting/incident_reporting_service_unittest.cc b/chrome/browser/safe_browsing/incident_reporting/incident_reporting_service_unittest.cc index 7303ea217c990..601086fb6b94b 100644 --- a/chrome/browser/safe_browsing/incident_reporting/incident_reporting_service_unittest.cc +++ b/chrome/browser/safe_browsing/incident_reporting/incident_reporting_service_unittest.cc @@ -30,6 +30,10 @@ #include "net/url_request/url_request_context_getter.h" #include "testing/gtest/include/gtest/gtest.h" +#if defined(OS_WIN) +#include "base/test/test_reg_util_win.h" +#endif + // A test fixture that sets up a test task runner and makes it the thread's // runner. The fixture implements a fake envrionment data collector and a fake // report uploader. @@ -180,6 +184,11 @@ class IncidentReportingServiceTest : public testing::Test { void SetUp() override { testing::Test::SetUp(); +#if defined(OS_WIN) + // Redirect HKCU so that the platform state store used by the test doesn't + // collide with existing Chrome installs or other tests running in parallel. + registry_override_manager_.OverrideRegistry(HKEY_CURRENT_USER); +#endif ASSERT_TRUE(profile_manager_.SetUp()); } @@ -471,6 +480,10 @@ class IncidentReportingServiceTest : public testing::Test { receiver->AddIncidentForProcess(MakeTestIncident(nullptr)); } +#if defined(OS_WIN) + registry_util::RegistryOverrideManager registry_override_manager_; +#endif + // A mapping of profile name to its corresponding properties. std::map profile_properties_; }; diff --git a/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc b/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc new file mode 100644 index 0000000000000..0c657c6587918 --- /dev/null +++ b/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc @@ -0,0 +1,200 @@ +// 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. + +// This file implements the platform-neutral Load() and Store() functions for a +// profile's safebrowsing.incidents_sent preference dictionary. The preference +// dict is converted to a protocol buffer message which is then serialized into +// a byte array. This serialized data is written to or read from some +// platform-specific storage via {Read,Write}StoreData implemented elsewhere. +// +// A pref dict like so: +// { "0": {"key1": "1235"}, {"key2": "6789"}}} +// is converted to an identical protocol buffer message, where the top-level +// mapping's keys are of type int, and the nested mappings' values are of type +// uint32_t. + +#include "chrome/browser/safe_browsing/incident_reporting/platform_state_store.h" + +#include "base/values.h" + +#if defined(USE_PLATFORM_STATE_STORE) + +#include "base/metrics/histogram_macros.h" +#include "base/strings/string_number_conversions.h" +#include "chrome/browser/safe_browsing/incident_reporting/state_store_data.pb.h" +#include "third_party/protobuf/src/google/protobuf/repeated_field.h" + +#endif // USE_PLATFORM_STATE_STORE + +namespace safe_browsing { +namespace platform_state_store { + +#if defined(USE_PLATFORM_STATE_STORE) + +namespace { + +using google::protobuf::RepeatedPtrField; + +// Copies the (key, digest) pairs from |keys_and_digests| (a dict of string +// values) to the |key_digest_pairs| protobuf. +void KeysAndDigestsToProtobuf( + const base::DictionaryValue& keys_and_digests, + RepeatedPtrField* + key_digest_pairs) { + for (base::DictionaryValue::Iterator iter(keys_and_digests); !iter.IsAtEnd(); + iter.Advance()) { + const base::StringValue* digest_value = nullptr; + if (!iter.value().GetAsString(&digest_value)) { + NOTREACHED(); + continue; + } + uint32_t digest = 0; + if (!base::StringToUint(digest_value->GetString(), &digest)) { + NOTREACHED(); + continue; + } + StateStoreData::Incidents::KeyDigestMapFieldEntry* key_digest = + key_digest_pairs->Add(); + key_digest->set_key(iter.key()); + key_digest->set_digest(digest); + } +} + +// Copies the (type, dict) pairs from |incidents_sent| (a dict of dict values) +// to the |typed_incidents| protobuf. +void IncidentsSentToProtobuf( + const base::DictionaryValue& incidents_sent, + RepeatedPtrField* + type_incidents_pairs) { + for (base::DictionaryValue::Iterator iter(incidents_sent); !iter.IsAtEnd(); + iter.Advance()) { + const base::DictionaryValue* keys_and_digests = nullptr; + if (!iter.value().GetAsDictionary(&keys_and_digests)) { + NOTREACHED(); + continue; + } + if (keys_and_digests->empty()) + continue; + int incident_type = 0; + if (!base::StringToInt(iter.key(), &incident_type)) { + NOTREACHED(); + continue; + } + StateStoreData::TypeIncidentsMapFieldEntry* entry = + type_incidents_pairs->Add(); + entry->set_type(incident_type); + KeysAndDigestsToProtobuf( + *keys_and_digests, entry->mutable_incidents()->mutable_key_to_digest()); + } +} + +// Copies the (key, digest) pairs for a specific incident type into |type_dict| +// (a dict of string values). +void RestoreOfTypeFromProtobuf( + const RepeatedPtrField& + key_digest_pairs, + base::DictionaryValue* type_dict) { + for (const auto& key_digest : key_digest_pairs) { + if (!key_digest.has_key() || !key_digest.has_digest()) + continue; + type_dict->SetStringWithoutPathExpansion( + key_digest.key(), base::UintToString(key_digest.digest())); + } +} + +// Copies the (type, dict) pairs into |value_dict| (a dict of dict values). +void RestoreFromProtobuf( + const RepeatedPtrField& + type_incidents_pairs, + base::DictionaryValue* value_dict) { + for (const auto& type_incidents : type_incidents_pairs) { + if (!type_incidents.has_type() || !type_incidents.has_incidents() || + type_incidents.incidents().key_to_digest_size() == 0) { + continue; + } + std::string type_string(base::IntToString(type_incidents.type())); + base::DictionaryValue* type_dict = nullptr; + if (!value_dict->GetDictionaryWithoutPathExpansion(type_string, + &type_dict)) { + type_dict = new base::DictionaryValue(); + value_dict->SetWithoutPathExpansion(type_string, type_dict); + } + RestoreOfTypeFromProtobuf(type_incidents.incidents().key_to_digest(), + type_dict); + } +} + +} // namespace + +#endif // USE_PLATFORM_STATE_STORE + +scoped_ptr Load(Profile* profile) { +#if defined(USE_PLATFORM_STATE_STORE) + scoped_ptr value_dict(new base::DictionaryValue()); + std::string data; + PlatformStateStoreLoadResult result = ReadStoreData(profile, &data); + if (result == PlatformStateStoreLoadResult::SUCCESS) + result = DeserializeIncidentsSent(data, value_dict.get()); + switch (result) { + case PlatformStateStoreLoadResult::SUCCESS: + case PlatformStateStoreLoadResult::CLEARED_DATA: + case PlatformStateStoreLoadResult::CLEARED_NO_DATA: + // Return a (possibly empty) dictionary for the success cases. + break; + case PlatformStateStoreLoadResult::DATA_CLEAR_FAILED: + case PlatformStateStoreLoadResult::OPEN_FAILED: + case PlatformStateStoreLoadResult::READ_FAILED: + case PlatformStateStoreLoadResult::PARSE_ERROR: + // Return null for all error cases. + value_dict.reset(); + break; + } + UMA_HISTOGRAM_ENUMERATION( + "SBIRS.PSSLoadResult", static_cast(result), + static_cast(PlatformStateStoreLoadResult::NUM_RESULTS)); + return value_dict.Pass(); +#else + return scoped_ptr(); +#endif +} + +void Store(Profile* profile, const base::DictionaryValue* incidents_sent) { +#if defined(USE_PLATFORM_STATE_STORE) + std::string data; + SerializeIncidentsSent(incidents_sent, &data); + UMA_HISTOGRAM_COUNTS("SBIRS.PSSDataStoreSize", data.size()); + WriteStoreData(profile, data); +#endif +} + +#if defined(USE_PLATFORM_STATE_STORE) + +void SerializeIncidentsSent(const base::DictionaryValue* incidents_sent, + std::string* data) { + StateStoreData store_data; + + IncidentsSentToProtobuf(*incidents_sent, + store_data.mutable_type_to_incidents()); + store_data.SerializeToString(data); +} + +PlatformStateStoreLoadResult DeserializeIncidentsSent( + const std::string& data, + base::DictionaryValue* value_dict) { + StateStoreData store_data; + if (data.empty()) { + value_dict->Clear(); + return PlatformStateStoreLoadResult::SUCCESS; + } + if (!store_data.ParseFromString(data)) + return PlatformStateStoreLoadResult::PARSE_ERROR; + value_dict->Clear(); + RestoreFromProtobuf(store_data.type_to_incidents(), value_dict); + return PlatformStateStoreLoadResult::SUCCESS; +} + +#endif // USE_PLATFORM_STATE_STORE + +} // namespace platform_state_store +} // namespace safe_browsing diff --git a/chrome/browser/safe_browsing/incident_reporting/platform_state_store.h b/chrome/browser/safe_browsing/incident_reporting/platform_state_store.h new file mode 100644 index 0000000000000..1662ba815e1fc --- /dev/null +++ b/chrome/browser/safe_browsing/incident_reporting/platform_state_store.h @@ -0,0 +1,85 @@ +// 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. + +// An interface to platform-specific storage of IncidentReportingService prune +// state. + +#ifndef CHROME_BROWSER_SAFE_BROWSING_INCIDENT_REPORTING_PLATFORM_STATE_STORE_H_ +#define CHROME_BROWSER_SAFE_BROWSING_INCIDENT_REPORTING_PLATFORM_STATE_STORE_H_ + +#include +#include + +#include "base/memory/scoped_ptr.h" +#include "build/build_config.h" + +// Certain platforms provide their own storage of protobuf-serialized prune +// state. On platforms where it is not supported, Load() and Store() are noops. +#if defined(OS_WIN) +// Store the state in the registry on Windows. +#define USE_PLATFORM_STATE_STORE +#endif + +class Profile; + +namespace base { +class DictionaryValue; +} + +namespace safe_browsing { +namespace platform_state_store { + +// Loads the platform-specific storage for |profile|. Returns null if there is +// no such storage for the current platform or in case of error; otherwise, a +// (possibly empty) dictionary. +scoped_ptr Load(Profile* profile); + +// Stores the state for |profile| in |incidents_sent| into platform-specific +// storage if there is such for the current platform. +void Store(Profile* profile, const base::DictionaryValue* incidents_sent); + +#if defined(USE_PLATFORM_STATE_STORE) + +// All declarations and definitions from this point forward are for use by +// implementations in platform-specific source files, or are exposed for the +// sake of testing. + +// The result of loading platform-specific state. This is a histogram type; do +// not reorder. +enum class PlatformStateStoreLoadResult : int32_t { + SUCCESS = 0, + CLEARED_DATA = 1, + CLEARED_NO_DATA = 2, + DATA_CLEAR_FAILED = 3, + OPEN_FAILED = 4, + READ_FAILED = 5, + PARSE_ERROR = 6, + NUM_RESULTS +}; + +// A platform-specific function to read store data for |profile| into |data|. +// Returns SUCCESS if |data| was populated, or a load result value indicating +// why no data was read. +PlatformStateStoreLoadResult ReadStoreData(Profile* profile, std::string* data); + +// A platform-specific function to write store data for |profile| from |data|. +void WriteStoreData(Profile* profile, const std::string& data); + +// Serializes the |incidents_sent| preference into |data|, replacing its +// contents. Exposed for testing. +void SerializeIncidentsSent(const base::DictionaryValue* incidents_sent, + std::string* data); + +// Deserializes |data| into |value_dict|. Returns SUCCESS if |data| is empty or +// fully processed. Exposed for testing. +PlatformStateStoreLoadResult DeserializeIncidentsSent( + const std::string& data, + base::DictionaryValue* value_dict); + +#endif // USE_PLATFORM_STATE_STORE + +} // namespace platform_state_store +} // namespace safe_browsing + +#endif // CHROME_BROWSER_SAFE_BROWSING_INCIDENT_REPORTING_PLATFORM_STATE_STORE_H_ diff --git a/chrome/browser/safe_browsing/incident_reporting/platform_state_store_unittest.cc b/chrome/browser/safe_browsing/incident_reporting/platform_state_store_unittest.cc new file mode 100644 index 0000000000000..9ee1faad45c8b --- /dev/null +++ b/chrome/browser/safe_browsing/incident_reporting/platform_state_store_unittest.cc @@ -0,0 +1,77 @@ +// 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 "chrome/browser/safe_browsing/incident_reporting/platform_state_store.h" + +#if defined(USE_PLATFORM_STATE_STORE) + +#include "base/memory/scoped_ptr.h" +#include "base/values.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace safe_browsing { +namespace platform_state_store { + +namespace { + +// The serialized form of the dict created by CreateTestIncidentsSentPref. +const uint8_t kTestData[] = { + 0x0a, 0x19, 0x08, 0x00, 0x12, 0x15, 0x0a, 0x09, 0x0a, 0x04, 0x77, + 0x68, 0x61, 0x3f, 0x10, 0x94, 0x4d, 0x0a, 0x08, 0x0a, 0x04, 0x77, + 0x68, 0x61, 0x61, 0x10, 0x00, 0x0a, 0x1a, 0x08, 0x02, 0x12, 0x16, + 0x0a, 0x09, 0x0a, 0x05, 0x62, 0x6c, 0x6f, 0x72, 0x66, 0x10, 0x05, + 0x0a, 0x09, 0x0a, 0x04, 0x73, 0x70, 0x61, 0x6d, 0x10, 0xd2, 0x09 +}; + +// Returns a dict with some sample data in it. +scoped_ptr CreateTestIncidentsSentPref() { + scoped_ptr incidents_sent(new base::DictionaryValue); + + scoped_ptr type_dict(new base::DictionaryValue); + type_dict->SetStringWithoutPathExpansion("spam", "1234"); + type_dict->SetStringWithoutPathExpansion("blorf", "5"); + incidents_sent->SetWithoutPathExpansion("2", type_dict.release()); + + type_dict.reset(new base::DictionaryValue); + type_dict->SetStringWithoutPathExpansion("whaa", "0"); + type_dict->SetStringWithoutPathExpansion("wha?", "9876"); + incidents_sent->SetWithoutPathExpansion("0", type_dict.release()); + + return incidents_sent.Pass(); +} + +} // namespace + +// Tests that DeserializeIncidentsSent handles an empty payload properly. +TEST(PlatformStateStoreTest, DeserializeEmpty) { + scoped_ptr deserialized(new base::DictionaryValue); + PlatformStateStoreLoadResult load_result = + DeserializeIncidentsSent(std::string(), deserialized.get()); + ASSERT_EQ(PlatformStateStoreLoadResult::SUCCESS, load_result); + ASSERT_TRUE(deserialized->empty()); +} + +// Tests that serialize followed by deserialize doesn't lose data. +TEST(PlatformStateStoreTest, RoundTrip) { + scoped_ptr incidents_sent( + CreateTestIncidentsSentPref()); + std::string data; + + SerializeIncidentsSent(incidents_sent.get(), &data); + + // Make sure the serialized data matches expectations to ensure compatibility. + ASSERT_EQ(std::string(reinterpret_cast(&kTestData[0]), + sizeof(kTestData)), data); + + scoped_ptr deserialized(new base::DictionaryValue); + PlatformStateStoreLoadResult load_result = + DeserializeIncidentsSent(data, deserialized.get()); + ASSERT_EQ(PlatformStateStoreLoadResult::SUCCESS, load_result); + ASSERT_TRUE(deserialized->Equals(incidents_sent.get())); +} + +} // namespace platform_state_store +} // namespace safe_browsing + +#endif // USE_PLATFORM_STATE_STORE diff --git a/chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc b/chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc new file mode 100644 index 0000000000000..da2e2f50a5cb6 --- /dev/null +++ b/chrome/browser/safe_browsing/incident_reporting/platform_state_store_win.cc @@ -0,0 +1,103 @@ +// 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. + +// State is stored in the Windows registry in a value of the key +// HKEY_CURRENT_USER\Software\\IncidentsSent for each profile. The +// value names are profile directory BaseNames. + +#include "chrome/browser/safe_browsing/incident_reporting/platform_state_store.h" + +#include + +#include "base/numerics/safe_conversions.h" +#include "base/win/registry.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/installer/util/browser_distribution.h" + +namespace safe_browsing { +namespace platform_state_store { + +namespace { + +// Returns the path to the registry key holding profile-specific state values. +base::string16 GetStateStoreKeyName() { + base::string16 key_name(L"Software\\"); + return key_name.append( + BrowserDistribution::GetDistribution()->GetInstallSubDir()) + .append(L"\\IncidentsSent"); +} + +// Returns the name of the registry value for |profile|'s state. +base::string16 GetValueNameForProfile(Profile* profile) { + return profile->GetPath().BaseName().value(); +} + +// Clears |profile|'s state. +PlatformStateStoreLoadResult ClearStoreData(Profile* profile) { + base::win::RegKey key; + + if (key.Open(HKEY_CURRENT_USER, GetStateStoreKeyName().c_str(), + KEY_QUERY_VALUE | KEY_SET_VALUE | KEY_WOW64_32KEY) == + ERROR_SUCCESS) { + base::string16 value_name(GetValueNameForProfile(profile)); + if (key.HasValue(value_name.c_str())) { + if (key.DeleteValue(value_name.c_str()) == ERROR_SUCCESS) + return PlatformStateStoreLoadResult::CLEARED_DATA; + return PlatformStateStoreLoadResult::DATA_CLEAR_FAILED; + } + } + return PlatformStateStoreLoadResult::CLEARED_NO_DATA; +} + +} // namespace + +PlatformStateStoreLoadResult ReadStoreData(Profile* profile, + std::string* data) { + // Clear any old state if this is a new profile. + if (profile->IsNewProfile()) { + data->clear(); + return ClearStoreData(profile); + } + + base::string16 value_name(GetValueNameForProfile(profile)); + base::win::RegKey key; + if (key.Open(HKEY_CURRENT_USER, GetStateStoreKeyName().c_str(), + KEY_QUERY_VALUE | KEY_WOW64_32KEY) == ERROR_SUCCESS) { + while (true) { + void* buffer = data->empty() ? nullptr : &(*data)[0]; + DWORD buffer_size = base::saturated_cast(data->size()); + LONG result = + key.ReadValue(value_name.c_str(), buffer, &buffer_size, nullptr); + // Trim the output string and return if all data was read. + if (result == ERROR_SUCCESS && buffer_size <= data->size()) { + data->resize(buffer_size); + return PlatformStateStoreLoadResult::SUCCESS; + } + // Increase the buffer and retry if more space was needed. Otherwise bail. + if (buffer_size > data->size()) + data->resize(buffer_size); + else + return PlatformStateStoreLoadResult::READ_FAILED; + } + } + + return PlatformStateStoreLoadResult::OPEN_FAILED; +} + +void WriteStoreData(Profile* profile, const std::string& data) { + base::win::RegKey key; + if (key.Create(HKEY_CURRENT_USER, GetStateStoreKeyName().c_str(), + KEY_SET_VALUE | KEY_WOW64_32KEY) == ERROR_SUCCESS) { + if (data.empty()) { + key.DeleteValue(GetValueNameForProfile(profile).c_str()); + } else { + key.WriteValue(GetValueNameForProfile(profile).c_str(), &data[0], + base::saturated_cast(data.size()), + REG_BINARY); + } + } +} + +} // namespace platform_state_store +} // namespace safe_browsing diff --git a/chrome/browser/safe_browsing/incident_reporting/platform_state_store_win_unittest.cc b/chrome/browser/safe_browsing/incident_reporting/platform_state_store_win_unittest.cc new file mode 100644 index 0000000000000..055ef58b7c0ca --- /dev/null +++ b/chrome/browser/safe_browsing/incident_reporting/platform_state_store_win_unittest.cc @@ -0,0 +1,179 @@ +// 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 "chrome/browser/safe_browsing/incident_reporting/platform_state_store.h" + +#include "base/prefs/pref_notifier_impl.h" +#include "base/prefs/testing_pref_store.h" +#include "base/strings/utf_string_conversions.h" +#include "base/test/test_reg_util_win.h" +#include "base/test/test_simple_task_runner.h" +#include "base/thread_task_runner_handle.h" +#include "base/win/registry.h" +#include "chrome/browser/prefs/browser_prefs.h" +#include "chrome/test/base/testing_browser_process.h" +#include "chrome/test/base/testing_pref_service_syncable.h" +#include "chrome/test/base/testing_profile.h" +#include "chrome/test/base/testing_profile_manager.h" +#include "components/pref_registry/pref_registry_syncable.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace safe_browsing { +namespace platform_state_store { + +namespace { + +const char kTestData_[] = "comme un poisson"; +const DWORD kTestDataSize_ = sizeof(kTestData_) - 1; + +} // namespace + +class PlatformStateStoreWinTest : public ::testing::Test { + protected: + PlatformStateStoreWinTest() + : profile_(nullptr), + task_runner_(new base::TestSimpleTaskRunner()), + thread_task_runner_handle_(task_runner_), + profile_manager_(TestingBrowserProcess::GetGlobal()) {} + + void SetUp() override { + ::testing::Test::SetUp(); + registry_override_manager_.OverrideRegistry(HKEY_CURRENT_USER); + ASSERT_TRUE(profile_manager_.SetUp()); + } + + // Creates/resets |profile_|. If |new_profile| is true, the profile will + // believe that it is new (Profile::IsNewProfile() will return true). + void ResetProfile(bool new_profile) { + if (profile_) { + profile_manager_.DeleteTestingProfile(kProfileName_); + profile_ = nullptr; + } + // Create a profile with a user PrefStore that can be manipulated. + TestingPrefStore* user_pref_store = new TestingPrefStore(); + // Profile::IsNewProfile() returns true/false on the basis of the pref + // store's read_error property. A profile is considered "New" if it didn't + // have a user prefs file. + user_pref_store->set_read_error( + new_profile ? PersistentPrefStore::PREF_READ_ERROR_NO_FILE + : PersistentPrefStore::PREF_READ_ERROR_NONE); + // Ownership of |user_pref_store| is passed to the service. + scoped_ptr prefs(new TestingPrefServiceSyncable( + new TestingPrefStore(), user_pref_store, new TestingPrefStore(), + new user_prefs::PrefRegistrySyncable(), new PrefNotifierImpl())); + chrome::RegisterUserProfilePrefs(prefs->registry()); + profile_ = profile_manager_.CreateTestingProfile( + kProfileName_, prefs.Pass(), base::UTF8ToUTF16(kProfileName_), 0, + std::string(), TestingProfile::TestingFactories()); + if (new_profile) + ASSERT_TRUE(profile_->IsNewProfile()); + else + ASSERT_FALSE(profile_->IsNewProfile()); + } + + void WriteTestData() { + base::win::RegKey key; + ASSERT_EQ(ERROR_SUCCESS, key.Create(HKEY_CURRENT_USER, kStoreKeyName_, + KEY_SET_VALUE | KEY_WOW64_32KEY)); + ASSERT_EQ(ERROR_SUCCESS, + key.WriteValue(base::UTF8ToUTF16(kProfileName_).c_str(), + &kTestData_[0], kTestDataSize_, REG_BINARY)); + } + + void AssertTestDataIsAbsent() { + base::win::RegKey key; + ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, kStoreKeyName_, + KEY_QUERY_VALUE | KEY_WOW64_32KEY)); + ASSERT_FALSE(key.HasValue(base::UTF8ToUTF16(kProfileName_).c_str())); + } + + void AssertTestDataIsPresent() { + char buffer[kTestDataSize_] = {}; + base::win::RegKey key; + ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, kStoreKeyName_, + KEY_QUERY_VALUE | KEY_WOW64_32KEY)); + DWORD data_size = kTestDataSize_; + DWORD data_type = REG_NONE; + ASSERT_EQ(ERROR_SUCCESS, + key.ReadValue(base::UTF8ToUTF16(kProfileName_).c_str(), + &buffer[0], &data_size, &data_type)); + EXPECT_EQ(REG_BINARY, data_type); + ASSERT_EQ(kTestDataSize_, data_size); + EXPECT_EQ(std::string(&buffer[0], data_size), + std::string(&kTestData_[0], kTestDataSize_)); + } + + static const char kProfileName_[]; + static const base::char16 kStoreKeyName_[]; + TestingProfile* profile_; + + private: + registry_util::RegistryOverrideManager registry_override_manager_; + scoped_refptr task_runner_; + base::ThreadTaskRunnerHandle thread_task_runner_handle_; + TestingProfileManager profile_manager_; + + DISALLOW_COPY_AND_ASSIGN(PlatformStateStoreWinTest); +}; + +// static +const char PlatformStateStoreWinTest::kProfileName_[] = "test_profile"; +#if defined(GOOGLE_CHROME_BUILD) +const base::char16 PlatformStateStoreWinTest::kStoreKeyName_[] = + L"Software\\Google\\Chrome\\IncidentsSent"; +#else +const base::char16 PlatformStateStoreWinTest::kStoreKeyName_[] = + L"Software\\Chromium\\IncidentsSent"; +#endif + +// Tests that store data is written correctly to the proper location. +TEST_F(PlatformStateStoreWinTest, WriteStoreData) { + ResetProfile(false /* !new_profile */); + + ASSERT_FALSE(base::win::RegKey(HKEY_CURRENT_USER, kStoreKeyName_, + KEY_QUERY_VALUE | KEY_WOW64_32KEY) + .HasValue(base::UTF8ToUTF16(kProfileName_).c_str())); + WriteStoreData(profile_, kTestData_); + AssertTestDataIsPresent(); +} + +// Tests that store data is read from the proper location. +TEST_F(PlatformStateStoreWinTest, ReadStoreData) { + // Put some data in the registry. + WriteTestData(); + + ResetProfile(false /* !new_profile */); + std::string data; + PlatformStateStoreLoadResult result = ReadStoreData(profile_, &data); + EXPECT_EQ(PlatformStateStoreLoadResult::SUCCESS, result); + EXPECT_EQ(std::string(&kTestData_[0], kTestDataSize_), data); +} + +// Tests that an empty write clears the stored data. +TEST_F(PlatformStateStoreWinTest, WriteEmptyStoreData) { + // Put some data in the registry. + WriteTestData(); + + ResetProfile(false /* !new_profile */); + + WriteStoreData(profile_, std::string()); + AssertTestDataIsAbsent(); +} + +// Tests that data in the registry is ignored if the profile is new. +TEST_F(PlatformStateStoreWinTest, ReadNewProfileClearData) { + ResetProfile(true /* new_profile */); + + // Put some data in the registry. + WriteTestData(); + + std::string data; + PlatformStateStoreLoadResult result = ReadStoreData(profile_, &data); + EXPECT_EQ(PlatformStateStoreLoadResult::CLEARED_DATA, result); + EXPECT_EQ(std::string(), data); + AssertTestDataIsAbsent(); +} + +} // namespace platform_state_store +} // namespace safe_browsing diff --git a/chrome/browser/safe_browsing/incident_reporting/state_store.cc b/chrome/browser/safe_browsing/incident_reporting/state_store.cc index 1822ab6117626..51e125cea1c77 100644 --- a/chrome/browser/safe_browsing/incident_reporting/state_store.cc +++ b/chrome/browser/safe_browsing/incident_reporting/state_store.cc @@ -9,6 +9,7 @@ #include "base/values.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/safe_browsing/incident_reporting/incident.h" +#include "chrome/browser/safe_browsing/incident_reporting/platform_state_store.h" #include "chrome/common/pref_names.h" namespace safe_browsing { @@ -26,6 +27,8 @@ StateStore::Transaction::~Transaction() { #if DCHECK_IS_ON() store_->has_transaction_ = false; #endif + if (pref_update_) + platform_state_store::Store(store_->profile_, store_->incidents_sent_); } void StateStore::Transaction::MarkAsReported(IncidentType type, @@ -59,6 +62,15 @@ void StateStore::Transaction::ClearForType(IncidentType type) { } } +void StateStore::Transaction::ClearAll() { + if (pref_update_) + pref_update_.reset(); + if (store_->incidents_sent_) { + store_->incidents_sent_ = nullptr; + store_->profile_->GetPrefs()->ClearPref(prefs::kSafeBrowsingIncidentsSent); + } +} + base::DictionaryValue* StateStore::Transaction::GetPrefDict() { if (!pref_update_) { pref_update_.reset(new DictionaryPrefUpdate( @@ -71,6 +83,11 @@ base::DictionaryValue* StateStore::Transaction::GetPrefDict() { return pref_update_->Get(); } +void StateStore::Transaction::ReplacePrefDict( + scoped_ptr pref_dict) { + GetPrefDict()->Swap(pref_dict.get()); +} + // StateStore ------------------------------------------------------------------ @@ -88,8 +105,19 @@ StateStore::StateStore(Profile* profile) if (value) value->GetAsDictionary(&incidents_sent_); + // Apply the platform data. Transaction transaction(this); - CleanLegacyValues(&transaction); + scoped_ptr value_dict( + platform_state_store::Load(profile_)); + if (value_dict) { + if (value_dict->empty()) + transaction.ClearAll(); + else if (!incidents_sent_ || !incidents_sent_->Equals(value_dict.get())) + transaction.ReplacePrefDict(value_dict.Pass()); + } + + if (incidents_sent_) + CleanLegacyValues(&transaction); } StateStore::~StateStore() { diff --git a/chrome/browser/safe_browsing/incident_reporting/state_store.h b/chrome/browser/safe_browsing/incident_reporting/state_store.h index 91b56ad215bc2..c2226170f4c7e 100644 --- a/chrome/browser/safe_browsing/incident_reporting/state_store.h +++ b/chrome/browser/safe_browsing/incident_reporting/state_store.h @@ -5,6 +5,8 @@ #ifndef CHROME_BROWSER_SAFE_BROWSING_INCIDENT_REPORTING_STATE_STORE_H_ #define CHROME_BROWSER_SAFE_BROWSING_INCIDENT_REPORTING_STATE_STORE_H_ +#include + #include "base/logging.h" #include "base/memory/scoped_ptr.h" #include "base/prefs/scoped_user_pref_update.h" @@ -39,13 +41,21 @@ class StateStore { // Clears all data associated with an incident type. void ClearForType(IncidentType type); + // Clears all data in the store. + void ClearAll(); + private: + friend class StateStore; + // Returns a writable view on the incidents_sent preference. The act of // obtaining this view will cause a serialize-and-write operation to be // scheduled when the transaction terminates. Use the store's // |incidents_sent_| member directly to simply query the preference. base::DictionaryValue* GetPrefDict(); + // Replaces the contents of the underlying dictionary value. + void ReplacePrefDict(scoped_ptr pref_dict); + // The store corresponding to this transaction. StateStore* store_; diff --git a/chrome/browser/safe_browsing/incident_reporting/state_store_data.proto b/chrome/browser/safe_browsing/incident_reporting/state_store_data.proto new file mode 100644 index 0000000000000..f81e7a117867f --- /dev/null +++ b/chrome/browser/safe_browsing/incident_reporting/state_store_data.proto @@ -0,0 +1,28 @@ +// 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. + +syntax = "proto2"; + +option optimize_for = LITE_RUNTIME; + +package safe_browsing; + +message StateStoreData { + message Incidents { + // The set of (key,digest) pairs for incidents of a specific type. + // This is wire-compatible with map key_to_digest = 1; + message KeyDigestMapFieldEntry { + optional string key = 1; + optional uint32 digest = 2; + } + repeated KeyDigestMapFieldEntry key_to_digest = 1; + } + // The set of all (key,digest) pairs for all incident types reported. + // This is wire-compatible with map type_to_incidents = 1; + message TypeIncidentsMapFieldEntry { + optional int32 type = 1; + optional Incidents incidents = 2; + } + repeated TypeIncidentsMapFieldEntry type_to_incidents = 1; +} diff --git a/chrome/browser/safe_browsing/incident_reporting/state_store_unittest.cc b/chrome/browser/safe_browsing/incident_reporting/state_store_unittest.cc index 5ec58bf226aaf..ff12661356e90 100644 --- a/chrome/browser/safe_browsing/incident_reporting/state_store_unittest.cc +++ b/chrome/browser/safe_browsing/incident_reporting/state_store_unittest.cc @@ -16,6 +16,7 @@ #include "chrome/browser/prefs/pref_service_syncable.h" #include "chrome/browser/prefs/pref_service_syncable_factory.h" #include "chrome/browser/safe_browsing/incident_reporting/incident.h" +#include "chrome/browser/safe_browsing/incident_reporting/platform_state_store.h" #include "chrome/common/pref_names.h" #include "chrome/test/base/testing_browser_process.h" #include "chrome/test/base/testing_profile.h" @@ -23,11 +24,41 @@ #include "components/pref_registry/pref_registry_syncable.h" #include "testing/gtest/include/gtest/gtest.h" +#if defined(OS_WIN) +#include "base/test/test_reg_util_win.h" +#endif + namespace safe_browsing { +#if defined(OS_WIN) + +// A base test fixture that redirects HKCU for testing the platform state store +// backed by the Windows registry to prevent interference with existing Chrome +// installs or other tests. +class PlatformStateStoreTestBase : public ::testing::Test { + protected: + PlatformStateStoreTestBase() {} + + void SetUp() override { + ::testing::Test::SetUp(); + registry_override_manager_.OverrideRegistry(HKEY_CURRENT_USER); + } + + private: + registry_util::RegistryOverrideManager registry_override_manager_; + + DISALLOW_COPY_AND_ASSIGN(PlatformStateStoreTestBase); +}; + +#else // OS_WIN + +using PlatformStateStoreTestBase = ::testing::Test; + +#endif // !OS_WIN + // A test fixture with a testing profile that writes its user prefs to a json // file. -class StateStoreTest : public ::testing::Test { +class StateStoreTest : public PlatformStateStoreTestBase { protected: struct TestData { IncidentType type; @@ -42,7 +73,7 @@ class StateStoreTest : public ::testing::Test { profile_manager_(TestingBrowserProcess::GetGlobal()) {} void SetUp() override { - testing::Test::SetUp(); + PlatformStateStoreTestBase::SetUp(); ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); ASSERT_TRUE(profile_manager_.SetUp()); CreateProfile(); @@ -199,10 +230,16 @@ TEST_F(StateStoreTest, PersistenceWithStoreDelete) { // Recreate the profile. CreateProfile(); - // Verify that the state did not survive. StateStore state_store(profile_); - for (const auto& data : kTestData_) + for (const auto& data : kTestData_) { +#if defined(USE_PLATFORM_STATE_STORE) + // Verify that the state survived. + ASSERT_TRUE(state_store.HasBeenReported(data.type, data.key, data.digest)); +#else + // Verify that the state did not survive. ASSERT_FALSE(state_store.HasBeenReported(data.type, data.key, data.digest)); +#endif + } } } // namespace safe_browsing diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 6e1c6efc1bf01..5636b33c2bb16 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -2514,6 +2514,9 @@ 'browser/safe_browsing/incident_reporting/module_integrity_verifier_win.h', 'browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc', 'browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h', + 'browser/safe_browsing/incident_reporting/platform_state_store.cc', + 'browser/safe_browsing/incident_reporting/platform_state_store.h', + 'browser/safe_browsing/incident_reporting/platform_state_store_win.cc', 'browser/safe_browsing/incident_reporting/preference_validation_delegate.cc', 'browser/safe_browsing/incident_reporting/preference_validation_delegate.h', 'browser/safe_browsing/incident_reporting/resource_request_detector.cc', @@ -3497,6 +3500,13 @@ 'dependencies': [ 'safe_browsing_proto', ], + 'conditions': [ + ['OS=="win"', { + 'dependencies': [ + 'incident_reporting_state_store_data_proto', + ], + }], + ], }], ['safe_browsing == 3', { 'sources': [ '<@(chrome_browser_safe_browsing_mobile_extended_sources)' ], @@ -3960,6 +3970,19 @@ }, 'includes': [ '../build/protoc.gypi' ] }, + { + # Protobuf compiler / generator for the safebrowsing incident reporting + # service state store data protocol buffer. + # GN version: //chrome/browser/safe_browsing/incident_reporting:state_store_data_proto + 'target_name': 'incident_reporting_state_store_data_proto', + 'type': 'static_library', + 'sources': [ 'browser/safe_browsing/incident_reporting/state_store_data.proto' ], + 'variables': { + 'proto_in_dir': 'browser/safe_browsing/incident_reporting', + 'proto_out_dir': 'chrome/browser/safe_browsing/incident_reporting', + }, + 'includes': [ '../build/protoc.gypi' ], + }, ], 'conditions': [ ['OS=="android"', { diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index c04ffe540e8ee..fc67ef662e962 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/chrome/chrome_tests_unit.gypi @@ -999,6 +999,8 @@ 'browser/safe_browsing/incident_reporting/module_integrity_unittest_util_win.h', 'browser/safe_browsing/incident_reporting/module_integrity_verifier_win_unittest.cc', 'browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc', + 'browser/safe_browsing/incident_reporting/platform_state_store_unittest.cc', + 'browser/safe_browsing/incident_reporting/platform_state_store_win_unittest.cc', 'browser/safe_browsing/incident_reporting/preference_validation_delegate_unittest.cc', 'browser/safe_browsing/incident_reporting/resource_request_detector_unittest.cc', 'browser/safe_browsing/incident_reporting/state_store_unittest.cc', diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 99c15103df8c1..28bf42260300f 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -22300,7 +22300,7 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. bengr@chromium.org The difference between the size specified in the X-Original-Content-Length - header and the size of teh response body. This is zero if the + header and the size of the response body. This is zero if the X-Original-Content-Length header is not present in the response. @@ -38934,6 +38934,19 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. + + grt@google.com + + The size, in bytes, of a profile's platform state store. This hisogram is + logged on each write, which always replaces any previous contents. + + + + + grt@google.com + The result of loading data from the platform state store. + + caitkp@google.com @@ -67944,6 +67957,22 @@ To add a new entry, add it with any value and run test to compute valid value. + + + + Platform data cleared for new profile. + + + Empty platform data cleared for new profile. + + + Failed to clear data for new profile. + + Failed to open data store. + Failed to read from data store. + Data could not be parsed. + +