Double-check safe-browsing database validity on update failure.

Double-check safe-browsing database validity on update failure.

A user reported a case where their safe-browsing database was not
updating.  Corrupt data in the add_chunks and sub_chunks area was
causing a 413 server response (even though the post was not
excessively large).  This change explicitly checks the checksums,
causing invalid databases to be reset.

This case could happen if the user's computer crashes before a
previous update was entirely committed to disk.  In most cases,
finalizing the next update would detect the corrupt data, but in this
case he error response from the server prevented the corruption from
being detected.

BUG=120219
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133618

[Scott Hess - ex-Googler, 2012-04-15 06:21:47]

Obviously this'll only fix up-to-date users - I'll drop some color on the bug.

[Scott Hess - ex-Googler, 2012-04-15 06:35:09]

On 2012/04/15 06:21:47, shess wrote:
> Obviously this'll only fix up-to-date users - I'll drop some color on the bug.

Hmm, well, no tests because the corruption handling throws a DCHECK so that
developers don't add systematic corruptions, but Pawel didn't let me handle
detecting that in a fine-grained way.  I should re-visit that to see if there's
an alternative, but meanwhile I'm loath to write another DISABLED_ test.

[noelutz, 2012-04-16 17:31:15]

lgtm

[Scott Hess - ex-Googler, 2012-04-16 17:36:15]

On 2012/04/16 17:31:15, noelutz wrote:
> lgtm

Before I push the commit button ... the bug was created because you noticed on
the server side that there was a problem.  Will you be able to track things from
that end, or should I add an additional histogram specifically for "Detected an
update failure combined with a corrupt database?"  The histogram I added at this
point tracks checksum failures whether the update request to the server
succeeded or failed.

[Scott Hess - ex-Googler, 2012-04-17 04:26:58]

On 2012/04/16 17:36:15, shess wrote:
> On 2012/04/16 17:31:15, noelutz wrote:
> > lgtm
> 
> Before I push the commit button ... the bug was created because you noticed on
> the server side that there was a problem.  Will you be able to track things
from
> that end, or should I add an additional histogram specifically for "Detected
an
> update failure combined with a corrupt database?"  The histogram I added at
this
> point tracks checksum failures whether the update request to the server
> succeeded or failed.

Added distinct histogram for the two detection cases.

[commit-bot: I haz the power, 2012-04-17 04:27:08]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Scott Hess - ex-Googler, 2012-04-17 06:49:34]

I do not quite know what's going on, here.  Looping in another OWNER just to be
sure.

[noelutz, 2012-04-17 16:00:47]

Am I not a full committer?  Maybe my rights were stripped at some point?

To answer your question: it's a good idea to add histograms in the SafeBrowsing
Chrome code.  I had to hack together some analysis code to detect this behavior.
 We don't currently monitor these kinds of failures on the server-side.

[mattm, 2012-04-18 00:26:01]

lgtm

[commit-bot: I haz the power, 2012-04-18 00:30:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/10093004/4001

[commit-bot: I haz the power, 2012-04-18 01:03:33]

Try job failure for 10093004-4001 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Scott Hess - ex-Googler, 2012-04-23 22:29:28]

Sigh.  I apologize, but ...

First time up, signed/unsigned compare warning, which didn't show up on my OSX
machine.  So I fixed it, and saw another.  So I broke down and built on Linux,
no warnings.  Then I thought of a better way to phrase it, but it differed
enough that I needed to re-test it, during which I made the mistake of running
against a valid database, and it said it was invalid (because |file| and |file_|
were different handles).  So I looked harder at unit testing and realized that
the harder-to-test code is a layer above this code.  So I wrote some tests. 
Writing the tests made me realize that I didn't need another file handle at all,
and also that validating the header was pointless, so I simplified the check.

SO, someone should review it again.  Again, apologies.  My first inclination had
been to stick to my current policy of not working on stuff other than flapper, I
should have went with that.

https://chromiumcodereview.appspot.com/10093004/diff/25001/chrome/browser/saf...
File chrome/browser/safe_browsing/safe_browsing_store_file.cc (right):

https://chromiumcodereview.appspot.com/10093004/diff/25001/chrome/browser/saf...
chrome/browser/safe_browsing/safe_browsing_store_file.cc:287: if
(!ReadItem(&file_digest, file_.get(), NULL))
|calculated_digest| was from |file|, while |file_digest| is being read from
|file_|.  :-(.

[mattm, 2012-04-23 23:47:48]

one issue with the unsigned arithmetic you might want to fix, otherwise lgtm.  
Thanks for working on this!

https://chromiumcodereview.appspot.com/10093004/diff/25001/chrome/browser/saf...
File chrome/browser/safe_browsing/safe_browsing_store_file.cc (right):

https://chromiumcodereview.appspot.com/10093004/diff/25001/chrome/browser/saf...
chrome/browser/safe_browsing/safe_browsing_store_file.cc:266: bytes_left -=
sizeof(base::MD5Digest);
if the file size is < sizeof(base::MD5Digest) this would wrap. But I guess it
would just fail on the fread return value test, so not too bad.

[Scott Hess - ex-Googler, 2012-04-24 01:37:21]

https://chromiumcodereview.appspot.com/10093004/diff/25001/chrome/browser/saf...
File chrome/browser/safe_browsing/safe_browsing_store_file.cc (right):

https://chromiumcodereview.appspot.com/10093004/diff/25001/chrome/browser/saf...
chrome/browser/safe_browsing/safe_browsing_store_file.cc:266: bytes_left -=
sizeof(base::MD5Digest);
On 2012/04/23 23:47:48, mattm wrote:
> if the file size is < sizeof(base::MD5Digest) this would wrap. But I guess it
> would just fail on the fread return value test, so not too bad.

Whoa, totally valid, thanks for catching that!

[commit-bot: I haz the power, 2012-04-24 01:37:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/10093004/36001

[commit-bot: I haz the power, 2012-04-24 03:04:02]

Change committed as 133618