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

chrome/browser/safe_browsing/safe_browsing_store_file.cc

 // Copyright (c) 2011 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/safe_browsing_store_file.h"
 
 #include "base/md5.h"
 #include "base/metrics/histogram.h"
 
 namespace {
@@ skipping 225 matching lines @@
   // existing file is a SQLite file.  Make sure the journal file is
   // also removed.
   const FilePath journal_filename(
       filename_.value() + FILE_PATH_LITERAL("-journal"));
   if (file_util::PathExists(journal_filename))
     file_util::Delete(journal_filename, false);
 
   return true;
 }
 
+bool SafeBrowsingStoreFile::CheckValidity() {
+  if (empty_)
+    return true;
+
+  // If the file was not empty, it should be open.
+  DCHECK(file_.get());
+
+  if (!FileRewind(file_.get()))
+    return OnCorruptDatabase();
+
+  int64 size = 0;
+  if (!file_util::GetFileSize(filename_, &size))
+    return OnCorruptDatabase();
+
+  base::MD5Context context;
+  base::MD5Init(&context);
+
+  // Read everything except the final digest.
+  size_t bytes_left = static_cast<size_t>(size);
+  CHECK(size == static_cast<int64>(bytes_left));
+  bytes_left -= sizeof(base::MD5Digest);

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

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]

Whoa, totally valid, thanks for catching that!

+
+  // Fold the contents of the file into the checksum.
+  while (bytes_left > 0) {
+    char buf[4096];
+    const size_t c = std::min(sizeof(buf), bytes_left);
+    const size_t ret = fread(buf, 1, c, file_.get());
+
+    // The file's size changed while reading, give up.
+    if (ret != c)
+      return OnCorruptDatabase();
+    base::MD5Update(&context, base::StringPiece(buf, c));
+    bytes_left -= c;
+  }
+
+  // Calculate the digest to this point.
+  base::MD5Digest calculated_digest;
+  base::MD5Final(&calculated_digest, &context);
+
+  // Read the stored digest and verify it.
+  base::MD5Digest file_digest;
+  if (!ReadItem(&file_digest, file_.get(), NULL))

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

|calculated_digest| was from |file|, while |file_digest| is being read from
|file_|.  :-(.

+    return OnCorruptDatabase();
+  if (0 != memcmp(&file_digest, &calculated_digest, sizeof(file_digest))) {
+    RecordFormatEvent(FORMAT_EVENT_VALIDITY_CHECKSUM_FAILURE);
+    return OnCorruptDatabase();
+  }
+
+  return true;
+}
+
 void SafeBrowsingStoreFile::Init(
     const FilePath& filename,
     const base::Closure& corruption_callback
 ) {
   filename_ = filename;
   corruption_callback_ = corruption_callback;
 }
 
 bool SafeBrowsingStoreFile::BeginChunk() {
   return ClearChunkBuffers();
@@ skipping 240 matching lines @@
 
     // Calculate the digest to this point.
     base::MD5Digest calculated_digest;
     base::MD5Final(&calculated_digest, &context);
 
     // Read the stored checksum and verify it.
     base::MD5Digest file_digest;
     if (!ReadItem(&file_digest, file_.get(), NULL))
       return OnCorruptDatabase();
 
-    if (0 != memcmp(&file_digest, &calculated_digest, sizeof(file_digest)))
+    if (0 != memcmp(&file_digest, &calculated_digest, sizeof(file_digest))) {
+      RecordFormatEvent(FORMAT_EVENT_UPDATE_CHECKSUM_FAILURE);
       return OnCorruptDatabase();
+    }
 
     // Close the file so we can later rename over it.
     file_.reset();
   }
   DCHECK(!file_.get());
 
   // Rewind the temporary storage.
   if (!FileRewind(new_file_.get()))
     return false;
 
@@ skipping 176 matching lines @@
   out->insert(out->end(), sub_chunks_cache_.begin(), sub_chunks_cache_.end());
 }
 
 void SafeBrowsingStoreFile::DeleteAddChunk(int32 chunk_id) {
   add_del_cache_.insert(chunk_id);
 }
 
 void SafeBrowsingStoreFile::DeleteSubChunk(int32 chunk_id) {
   sub_del_cache_.insert(chunk_id);
 }