SafeBrowising: Read and write V4Store from/to disk

components/safe_browsing_db/v4_store.cc

Index: components/safe_browsing_db/v4_store.cc
diff --git a/components/safe_browsing_db/v4_store.cc b/components/safe_browsing_db/v4_store.cc
index a887185422590057e0163b2ed0f1eefeed2111fa..e7b64cf50c5e2cb37f90bc45c52ea86d278c775b 100644
--- a/components/safe_browsing_db/v4_store.cc
+++ b/components/safe_browsing_db/v4_store.cc
@@ -4,15 +4,47 @@
 
 #include "base/base64.h"
 #include "base/bind.h"
+#include "base/files/file_util.h"
+#include "base/metrics/histogram.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/strings/stringprintf.h"
 #include "components/safe_browsing_db/v4_store.h"
+#include "components/safe_browsing_db/v4_store.pb.h"
 
 namespace safe_browsing {
 
+namespace {
+// NOTE(vakh): kFileMagic should not be a byte-wise palindrome, so
+// that byte-order changes force corruption.
+const uint32_t kFileMagic = 0x600D71FE;
+
+const uint32_t kFileVersion = 9;
+
+void RecordStoreReadFailure(StoreReadFailureType event_type) {
+  UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.V4StoreReadEvent", event_type,
+                            FORMAT_FAILURE_MAX);
+}
+
+}  // namespace
+
 V4Store* V4StoreFactory::CreateV4Store(
     const scoped_refptr<base::SequencedTaskRunner>& task_runner,
     const base::FilePath& store_path) {
-  return new V4Store(task_runner, store_path);
+  V4Store* new_store = new V4Store(task_runner, store_path);
+  new_store->Initialize();
+  return new_store;
+}
+
+void V4Store::Initialize() {
+  // If a state already exists, don't re-initilize.

[Nathan Parker, 2016/06/16 00:07:50]

Would this indicate a bug?  You could dcheck then.

[vakh (use Gerrit instead), 2016/06/16 08:25:18]

Done.

+  if (!state_.empty()) {
+    return;
+  }
+
+  StoreReadFailureType store_read_failure_type = ReadFromDisk();

[Nathan Parker, 2016/06/16 00:07:50]

nit:How about StoreReadResult?  FailureType doesn't fit when it's a success.

[vakh (use Gerrit instead), 2016/06/16 08:25:18]

Done.

+  RecordStoreReadFailure(store_read_failure_type);
+  DVLOG(1) << "Loaded V4Store: " << *this;
+  DVLOG(1) << "store_read_failure_type: " << store_read_failure_type;
 }
 
 V4Store::V4Store(const scoped_refptr<base::SequencedTaskRunner>& task_runner,
@@ -48,8 +80,12 @@ void V4Store::ApplyUpdate(
 
   // TODO(vakh): The new store currently only picks up the new state. Do more.
   new_store->state_ = response.new_client_state();
+  DVLOG(1) << "Created V4Store: " << *new_store;
 
-  DVLOG(1) << "Created V4Store: " << new_store->DebugString();
+  // TODO(vakh): Merge the old store and the new update in new_store.
+  // Then, create a ListUpdateResponse containing RICE encoded hash-prefixes and
+  // response_type as FULL_UPDATE, and write that to disk.
+  new_store->WriteToDisk(response);
 
   // new_store is done updataing, pass it to the callback.
   callback_task_runner->PostTask(
@@ -62,4 +98,72 @@ std::string V4Store::GetHumanReadableState() const {
   return state_base64;
 }
 
+StoreReadFailureType V4Store::ReadFromDisk() {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+  std::string contents;
+  bool read_success = base::ReadFileToString(store_path_, &contents);
+  V4StoreFileFormat file_format;
+  if (!read_success) {
+    return FILE_UNREADABLE_FAILURE;

[Nathan Parker, 2016/06/16 00:07:50]

It would be good to differentiate "non existent" from other read failures. 
Otherwise, we'll have a guaranteed background rate of failures due to new
installs.   Could do it in a separate CL.

We may be able to better than ReadFileToString, since it reads in chunks and
expands the string incrementally, rather than reading / allocating in one block.
 Something to optimize later.

[vakh (use Gerrit instead), 2016/06/16 08:25:18]

Acknowledged.

+  }
+
+  if (contents.empty()) {
+    return FILE_EMPTY_FAILURE;
+  }
+
+  if (!file_format.ParseFromString(contents)) {
+    return PROTO_PARSING_FAILURE;
+  }
+
+  if (!file_format.has_magic_number() ||
+      file_format.magic_number() != kFileMagic) {
+    DVLOG(1) << "Unexpected magic number found in file: "
+             << file_format.magic_number();
+    return UNEXPECTED_MAGIC_NUMBER_FAILURE;
+  }
+
+  UMA_HISTOGRAM_SPARSE_SLOWLY("SafeBrowsing.V4StoreVersionRead",
+                              file_format.version_number());
+  if (!file_format.has_version_number() ||
+      file_format.version_number() < kFileVersion) {

[Nathan Parker, 2016/06/16 00:07:50]

What does this protect against?  It might be better to check != for now, and if
we later want to increment it, we can decide for that specific version
transition then how to handle older versions.  We'd probably want an upgrade
path, to avoid a sudden erase/reload of all clients.  But for now, we should
assume this can't read any other versions.

[vakh (use Gerrit instead), 2016/06/16 08:25:18]

It provides the guarantee that we know we can parse some/all of this file.
version 9 knows to look for new_client_state, additions, etc in
ListUpdateResponse.
version 10 could possibly look for other fields in ListUpdateResponse.
Fair enough. Done.

+    DVLOG(1) << "File version too low: " << file_format.version_number()
+             << "; expected: " << kFileVersion;
+    return FILE_VERSION_TOO_LOW_FAILURE;

[Nathan Parker, 2016/06/16 00:07:50]

Then this would be FILE_VERSION_INCOMPATIBLE

[vakh (use Gerrit instead), 2016/06/16 08:25:18]

Done.

+  }
+
+  if (!file_format.has_list_update_response()) {
+    return HASH_PREFIX_INFO_MISSING_FAILURE;
+  }
+
+  ListUpdateResponse list_update_response = file_format.list_update_response();
+  state_ = list_update_response.new_client_state();
+  // TODO(vakh): Do more with what's read from the disk.
+
+  return SUCCESS;
+}
+
+bool V4Store::WriteToDisk(const ListUpdateResponse& response) const {
+  // Do not write partial updates to the disk.
+  // After merging the updates, the ListUpdateResponse passed to this method
+  // should be a FULL_UPDATE.
+  if (!response.has_response_type() ||
+      response.response_type() != ListUpdateResponse::FULL_UPDATE) {
+    return false;
+  }
+
+  V4StoreFileFormat file_format;
+  file_format.set_magic_number(kFileMagic);
+  file_format.set_version_number(kFileVersion);
+  ListUpdateResponse* response_to_write =
+      file_format.mutable_list_update_response();
+  *response_to_write = response;

[Nathan Parker, 2016/06/16 00:07:50]

This is going to make a copy (big).  Instead, you could make |response|
non-const and do response_to_write->swap(response).  That'll empty |response|,
which is probably fine.  If not, you could swap it back after. Maybe there's a
better pattern.

[vakh (use Gerrit instead), 2016/06/24 01:04:49]

Acknowledged. I am going to try that as a separate CL since it requires function
signatures over multiple files so it's probably better done as a separate
independent change. Sounds good?

[Scott Hess - ex-Googler, 2016/06/24 19:51:17]

I'd say to look at this comprehensively, starting from passing ownership into
V4Database::ApplyUpdate(), which could take ownership of the vector then parcel
it out into individual responses (or post the entire thing to the background as
I suggested elsewhere), which in turn pass ownership into the store's
ApplyUpdate(), perhaps using unique_ptr<> to manage ownership, etc.

That said, the step merging the response into the existing data is probably
where the majority of the memory overhead will be in the long-term, once the
database has reached a steady state.

[vakh (use Gerrit instead), 2016/06/27 19:54:21]

I'm working on this. CL coming today (or tomorrow, some tests failing).

[vakh (use Gerrit instead), 2016/06/28 01:34:43]

https://codereview.chromium.org/2103693002/

[vakh (use Gerrit instead), 2016/06/28 01:34:43]

Here's the CL: https://codereview.chromium.org/2103693002/
That one's also ready for review now.

+  std::string file_format_string;
+  file_format.SerializeToString(&file_format_string);
+
+  size_t written = base::WriteFile(store_path_, file_format_string.c_str(),

[Nathan Parker, 2016/06/16 00:07:50]

We should write to a tmp file in the same dir (maybe store_path_ + ".tmp"), and
then do an atomic rename.  Otherwise, any interruption in the writing will
result in a bad file.

[vakh (use Gerrit instead), 2016/06/22 07:26:06]

Done.

+                                   file_format_string.size());
+  return file_format_string.size() == written;

[Nathan Parker, 2016/06/16 00:07:50]

TODO: Add UMA metrics:
  * File size
  * Time it takes to write, and read.  Maybe it could be just one timing metric
for reading all stores.  
  * Write success

+}
+
 }  // namespace safe_browsing