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 c32f42009865f756bb3d42077c431aaa8490d655..bcceda49bc09d7a66a82e6f428fb762f6a1e5185 100644
--- a/components/safe_browsing_db/v4_store.cc
+++ b/components/safe_browsing_db/v4_store.cc
@@ -4,11 +4,40 @@
 
 #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.

[Scott Hess - ex-Googler, 2016/06/22 22:58:38]

Protobuf would handle byte-ordering issues, so this wouldn't hold if the magic
number is inside the protobuf envelope.

OTOH, protobuf would handle byte-ordering issues, so it also doesn't need to
flag byte-order changes.

[vakh (use Gerrit instead), 2016/06/23 23:09:31]

Removed the comment.

+const uint32_t kFileMagic = 0x600D71FE;
+
+const uint32_t kFileVersion = 9;
+
+void RecordStoreReadResult(StoreReadResult result) {
+  UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.V4StoreReadResult", result,
+                            STORE_READ_RESULT_MAX);
+}
+
+void RecordStoreWriteResult(StoreWriteResult result) {
+  UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.V4StoreWriteResult", result,
+                            STORE_WRITE_RESULT_MAX);
+}
+
+// Returns the name of the temporary file used to buffer data for
+// |filename|.  Exported for unit tests.
+const base::FilePath TemporaryFileForFilename(const base::FilePath& filename) {
+  return base::FilePath(filename.value() + FILE_PATH_LITERAL("_new"));
+}
+
+}  // namespace
+
 std::ostream& operator<<(std::ostream& os, const V4Store& store) {
   os << store.DebugString();
   return os;
@@ -17,7 +46,17 @@ std::ostream& operator<<(std::ostream& os, const V4Store& store) {
 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();

[Scott Hess - ex-Googler, 2016/06/22 22:58:39]

If this is only called from the factory, it might make sense to make the factory
a friend and the method private.  Or to friend and call ReadFromDisk() directly.

OTOH, if it's paired with the constructor and the return value is void, why is
it even broken out from the constructor?

[vakh (use Gerrit instead), 2016/06/23 23:09:32]

FakeV4StoreFactory, derived from V4StoreFactory, also calls this constructor so
I'll need to make both of them friend.
I like to make as few test friends as possible. Including in code ;-) 
So that a test can create a V4Store object but can skip reading the disk file.

[Scott Hess - ex-Googler, 2016/06/24 04:14:11]

Acknowledged.  But it kind of feels like your inheritence is upside down, in
that case.

+  return new_store;
+}
+
+void V4Store::Initialize() {
+  // If a state already exists, don't re-initilize.

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

Nit: remove this comment.

[Scott Hess - ex-Googler, 2016/06/22 22:58:38]

I'm fine with a comment, but the comment makes it sound like it's saying how the
control flows, and what it's actually saying "Only ever initialize once."

[vakh (use Gerrit instead), 2016/06/24 18:03:50]

Acknowledged.

+  DCHECK(state_.empty());
+
+  StoreReadResult store_read_result = ReadFromDisk();
+  RecordStoreReadResult(store_read_result);
 }
 
 V4Store::V4Store(const scoped_refptr<base::SequencedTaskRunner>& task_runner,
@@ -31,7 +70,7 @@ std::string V4Store::DebugString() const {
   base::Base64Encode(state_, &state_base64);
 
   std::string value;
-  return base::SStringPrintf(&value, "path: %s; state: %s",
+  return base::SStringPrintf(&value, "{path: %s; state: %s}",
                              store_path_.value().c_str(), state_base64.c_str());

[Scott Hess - ex-Googler, 2016/06/22 22:58:38]

Thanks!

This can be base::StringPrintf() without |value|.  I feel like I said that on a
different review, so maybe this change broke a glass ceiling somewhere?

[vakh (use Gerrit instead), 2016/06/23 23:09:32]

Done.

 }
 
@@ -51,9 +90,101 @@ void V4Store::ApplyUpdate(
   // TODO(vakh): The new store currently only picks up the new state. Do more.
   new_store->state_ = response.new_client_state();
 
+  // 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.
+  StoreWriteResult result = new_store->WriteToDisk(response);
+  RecordStoreWriteResult(result);
+
   // new_store is done updating, pass it to the callback.
   callback_task_runner->PostTask(
       FROM_HERE, base::Bind(callback, base::Passed(&new_store)));
 }
 
+StoreReadResult V4Store::ReadFromDisk() {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+  std::string contents;
+  bool read_success = base::ReadFileToString(store_path_, &contents);
+  V4StoreFileFormat file_format;

[Scott Hess - ex-Googler, 2016/06/22 22:58:39]

Push file_format down to where it's used.

[vakh (use Gerrit instead), 2016/06/23 23:09:31]

Done.

+  if (!read_success) {
+    return FILE_UNREADABLE_FAILURE;
+  }
+
+  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();

[Scott Hess - ex-Googler, 2016/06/22 22:58:38]

What does this DVLOG do for !has_magic_number()?

Are the cases of not having the right fields maybe something to rollup in a
"wrong proto" case?

Maybe this should be a DCHECK, I can't think of any cases where you'd actually
expect something else here.

[vakh (use Gerrit instead), 2016/06/23 23:09:31]

You probably meant something else, but if you asking what would DVLOG print,
it'd be an empty string.
I think it'd be a useful metric to track: what exact error are we having while
reading the store files.
UMA tracking of common error cases.

[Scott Hess - ex-Googler, 2016/06/24 04:14:11]

Mostly I meant that this case is both !has_magic_number() and magic_number() is
wrong, and if !has_magic_number() holds I would assume that calling
magic_number() may be undefined.

Flipping that upside-down - if magic_number() is well-defined when
!has_magic_number(), then why bother to check has_magic_number() at all?  The
well-defined value will presumably be != kFileMagic, which simplifies the
condition.
I suspect that you'll either get an entirely-valid file, or an entirely-invalid
file.
I meant the DVLOG() could be a DCHECK().  In production, you'll get the UMA
stats, but it's unclear why you'd ever have an invalid file on your development
machine.  EXCEPT that you're doing testing, so you can't be DCHECK'ing in here,
so answered.

[vakh (use Gerrit instead), 2016/06/24 18:03:50]

Done.
Yes, that's my expectation also, but I think it is better to be explicit about
this.
Ack.

+    return UNEXPECTED_MAGIC_NUMBER_FAILURE;
+  }
+
+  UMA_HISTOGRAM_SPARSE_SLOWLY("SafeBrowsing.V4StoreVersionRead",
+                              file_format.version_number());

[Scott Hess - ex-Googler, 2016/06/22 22:58:38]

Should probably check if you have a version number before logging it.  Unless it
returns 0?

If it returns 0, then don't even check if it has a version number, just check if
the version number matches.  Hmm, that applies to magic number, too.

[vakh (use Gerrit instead), 2016/06/23 23:09:31]

Yes, it returns 0 if it isn't set.
We never expect to see version '0' so seeing this value in the logs would
indicate that we found something totally unexpected.
Magic number is the first check and we expect exactly one value. If we get a
wrong or unset magic number, then we are reading the wrong file. So that's a
boolean case: right file or wrong file.

If that check passes, then we are reading the right file. Now, if the version
doesn't match our expectation, that's an interesting event. So we want to know
what version was read, if any.

+  if (!file_format.has_version_number() ||
+      file_format.version_number() != kFileVersion) {
+    DVLOG(1) << "File version incompatible: " << file_format.version_number()
+             << "; expected: " << kFileVersion;

[Scott Hess - ex-Googler, 2016/06/22 22:58:38]

Maybe this should be a DCHECK, I can't think of any cases where you'd actually
expect something else here.

[vakh (use Gerrit instead), 2016/06/23 23:09:31]

DCHECKs make it hard to test conditions. I feel tests are a better way to
enforce conditions, but I may be wrong.

[Scott Hess - ex-Googler, 2016/06/24 04:14:11]

Acknowledged.

+    return FILE_VERSION_INCOMPATIBLE_FAILURE;
+  }
+
+  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 READ_SUCCESS;
+}
+
+StoreWriteResult 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) {
+    DVLOG(1) << "response.has_response_type(): "
+             << response.has_response_type();
+    DVLOG(1) << "response.response_type(): " << response.response_type();

[Scott Hess - ex-Googler, 2016/06/22 22:58:39]

I think just going with a DCHECK would be fair for these.

[vakh (use Gerrit instead), 2016/06/23 23:09:31]

DCHECK vs tests, as above.

+    return INVALID_RESPONSE_TYPE_FAILURE;
+  }
+
+  // Attempt writing to a temporary file first and at the end, swap the files.
+  const base::FilePath new_filename = TemporaryFileForFilename(store_path_);
+
+  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;

[Scott Hess - ex-Googler, 2016/06/22 22:58:38]

Is there no set_list_update_response() method?

[vakh (use Gerrit instead), 2016/06/23 23:09:31]

No, it is a repeated field so no set_X only add_X

[Scott Hess - ex-Googler, 2016/06/24 04:14:11]

Acknowledged.

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

[Scott Hess - ex-Googler, 2016/06/22 22:58:38]

file_format_string.c_str() has to nul-terminate.  You're passing size(), so
maybe data() instead?

[vakh (use Gerrit instead), 2016/06/23 23:09:32]

I didn't know about data(), but it looks like it's the same as c_str()
From: http://www.cplusplus.com/reference/string/string/data/
Both string::data and string::c_str are synonyms and return the same value.

[Scott Hess - ex-Googler, 2016/06/24 04:14:11]

Sigh.  This changed between C++98 and C++11.

I think I'd still prefer data(), because you're not interested in a c-string
(nul characters aren't special), so it more closely describes what you're doing.
 But I think there's room for preference here.

[vakh (use Gerrit instead), 2016/06/24 18:03:50]

Done.

+  if (file_format_string.size() != written) {
+    DVLOG(1) << "file_format_string.size(): " << file_format_string.size();
+    DVLOG(1) << "written: " << written;

[Scott Hess - ex-Googler, 2016/06/22 22:58:39]

DCHECK_EQ() would seem reasonable, this should never fail.

[vakh (use Gerrit instead), 2016/06/23 23:09:31]

Done.

+    return UNEXPECTED_BYTES_WRITTEN_FAILURE;
+  }
+
+  if (!base::Move(new_filename, store_path_)) {
+    DVLOG(1) << "store_path_: " << store_path_.value();
+    DVLOG(1) << "new_filename: " << new_filename.value();

[Scott Hess - ex-Googler, 2016/06/22 22:58:38]

new_filename can be derived (or store_path_ can be derived).  So maybe switch to
a single DVPLOG(1) statement to give error info.

[vakh (use Gerrit instead), 2016/06/23 23:09:31]

Done.

+    return UNABLE_TO_RENAME_FAILURE;
+  }
+
+  return WRITE_SUCCESS;
+}
+
 }  // namespace safe_browsing