SafeBrowising: Read and write V4Store from/to disk

components/safe_browsing_db/v4_store.proto

Index: components/safe_browsing_db/v4_store.proto
diff --git a/components/safe_browsing_db/v4_store.proto b/components/safe_browsing_db/v4_store.proto
new file mode 100644
index 0000000000000000000000000000000000000000..bb226fa25abdf039aa59fedefd0b4ad1b356fb46
--- /dev/null
+++ b/components/safe_browsing_db/v4_store.proto
@@ -0,0 +1,27 @@
+// 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.
+
+syntax = "proto2";
+
+import "safebrowsing.proto";
+
+option optimize_for = LITE_RUNTIME;
+
+package safe_browsing;
+
+// The message that's serialized to disk when a store update is processed.
+message V4StoreFileFormat {
+  // The magic bytes to identify this file as a SafeBrowsing hash-prefix file.
+  // https://en.wikipedia.org/wiki/Magic_number_(programming)

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

It's a uint32 not a blob, so I think "magic number" is better than "magic
bytes".  Also, no need to be coy, you could just give the number, here, since
it's mostly there just to make sure you aren't reading random garbage.

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

Done.
Not sure if that's really helpful as a comment.
Magic numbers, by definition, are well defined. They can't be random garbage.

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

Well, technically the number read here _could_ be random garbage which just
happens to be the magic number.

Mostly my point is that if this could be considered documentation for the file
format, then "There's an integer 123456789 here" makes sense, rather than "Here
is a magic number which is defined somewhere else".  It could even be defined in
this file as an enum.  I suppose it can't be the default for the field, though
:-).

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

If I come across a random file that happens to have a proto with this magic
number in front, I'd bet my money that this was a real store file (or intended
to be one) but somehow something went wrong in writing it.
Fair enough. Done. Unfortunately, can't enforce a default value.
Also, having a magic number as an enum sounds principally wrong.

+  optional uint32 magic_number = 1;
+
+  // Identifies the version of the file format.
+  // Version 9 with PVer4 is very different from earlier versions.

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

Hypothetical question: Does this even need a version number and magic number? 
The earlier code wanted a magic number for validation, and a version to
determine how to read the rest of the file.  Presumably the protobuf code
already knows how to tell if it's reading a valid protobuf file, and once read
you can use introspection to see what was contained.

Hmm.  If a v10 reader sees a v9 file, it can read it and then look to see if the
file has the fields it is concerned with, so it doesn't need a version to do
that.  A version might be convenient, but the hope would be that the v10 reader
doesn't even have to think about whether it's a v9 file, it just fills things in
naturally.  Going in reverse, a v9 reader should be able to read a v10 file, but
it probably can't _write_ a v10 file, so it might strip information.  But I
don't think a v9 reader can recognize that it has a >v9 file without an explicit
version number.

That said, the magic number feels like something which belongs outside the
envelope to validate the input, or dropped entirely if protobuf validates
sufficiently.

[Nathan Parker, 2016/06/23 00:37:55]

I think the use for the version is if we want to upgrade the format, and we
expect to be able to read both new and old.  A v10 reader should be able to
recognize/read a v9 file and write a v10 file.  We should never need a v9 reader
to read a v10 file (assuming the code doesn't get reverted in a subsequent
stable release).  The value in upgrading is so we don't have all stable clients
dump their data and do a full update just because we want to change the on-disk
format.  We can also assume that the back-version reading code would need to
live only for one release (those who are skip a release will just dump their
data and do a full update).

Magic number: This is useful if the protoparser does not validate sufficiently,
such as if it silently ignores unparsable data at the end (and assumes they are
fields from a newer version of the proto). If we can guarantee the protoparser
has consumed all the data and understands it, then we don't need this.

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

The goal of the magic number is to ensure that we are reading a store file.
Just being able to parse a proto doesn't ensure that. I am going to add a test
to demonstrate that.

The goal of version is: Now that we know we are reading a store file, what
should we expect to be set and also helps us track what version files being used
in the world.
Examples:
1. How many users are on v9 vs v10..
2. Are there any users reading store files but some how getting unexpected
version numbers?
3. Since we know this is version 9, we might expect some fields to be set and
others to be ignored. This allows us to deal better with breaking changes down
the line.

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

With the earlier raw file, the magic sufficed to differentiate a SQLite file
from a raw file, as they have different magic in the first few bytes.  But that
number was logically checked _before_ the other data was read.  In this case, we
already know you're reading a valid-ish protobuf.  The magic number would add
more certainty, I guess... but does it add more certainly over just a happy
version number?

[It's cheap to put in there, mostly I'm just trying to be extra skeptical at the
creation, since once in it stays forever.]
#1 seems legit to me, it would be weird to reverse-engineer a version indicator
from field presence.  #2 seems like a stretch, I'm not sure it matters in the
end.  #3 is where I can be convinced, but as-is I'm not entirely sure.

The problem as I see it is that the kinds of notably-breaking changes you're
suggesting also imply that you're going to end up with a lot of unused optional
fields.  Like you'll add a bunch of new fields which replace the original
fields, and your code will say "For version 10, don't see all these fields, do
set all those fields".  To me, that feels like you really want to just have a
different protobuf.  Which is why I mentioned "envelope".  Like if you had a
header message which contained magic+version, then used the version to drive
selection of which kind of message to read next.

Or is this implicit?  Like you might just have messages always defined with
fields 1 and 2 a particular way, and the v9 message might have different fields
than the v10 message beyond that?  That seems brittle, to me.

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

A version number is just an integer whose value we can't assert.
If we skip the magic number, we'll be accepting any file that was a proto with
the first field as an integer, which is arguably the most common first field in
protos.
The magic number and version number serve completely different purposes and I
don't think we can overload either.
Yes, I agree it is good to have this discussion.
This is fine and acceptable with protos.
If the field is optional, it is allowed to be unused and if it is unused, it
doesn't take any space on the disk.
You probably know this, but the recommendation for doing that with protos is to
add additional fields, not replace the existing ones. The existing ones, if
unused, don't get serialized.
To be frank, by using the has_* methods defined on protobufs, it is possible to
parse any version without knowing the exact version number. What I meant is that
can be done using the version number also.

Also, since the ListUpdateResponse proto isn't strictly defined in Chromium
itself (imported from SafeBrowsing PVer4 service's definition in google3), I
think it is useful to have a way to check version number in case they make
breaking changes to the proto (which I trust them not to, but would also like to
protect us against).

+  optional uint32 version_number = 2;
+
+  // Contains the information about the store being written and the hash-prefix
+  // updates. For details about this message, see: safebrowsing.proto

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

Drop "Contains the", that's implied by defining storage.  Drop "being written"
because that's also implied.  The second sentence is degenerate, maybe ", see
safebrowsing.proto."

[Actually, you could also just drop the second sentence entirely.  There's only
one import, so it should be obvious where to look.  Even if not, the symbols are
long enough to be easy to search for.]

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

Done.

+  optional FetchThreatListUpdatesResponse.ListUpdateResponse
+      list_update_response = 3;
+}