Short-circuit safe-browsing updates once corruption is detected.

The corruption handler posts a task to the current message loop to
delete the database after the current task has completed.  The older
code re-created the database and allowed the transaction to continue,
the newer code did not, causing a crash.  [The database was deleted
first, so on next launch it would not crash again.]

This changes the new code to short-circuit storage of further chunks
once corruption is detected, until the next update (which will
re-create the database as needed).

Adding the tests exposed some code paths which weren't quite right.
Pulled the lock out of ClearUpdateCaches() because ResetDatabase()
calls it with a lock.  The corruption handler can delete open
databases.  Added sanity-check code to SafeBrowsingStoreFile so that
it doesn't allocate insane vectors.

BUG=55606
TEST=crash in 55606 stops happening, tests green.

Review URL: http://codereview.chromium.org/3420015

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@60128 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 487 insertions, 31 deletions

diff --git a/chrome/browser/safe_browsing/safe_browsing_database.cc b/chrome/browser/safe_browsing/safe_browsing_database.cc
index e3bae18..c62aecd 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database.cc
@@ -192,7 +192,8 @@ FilePath SafeBrowsingDatabase::BloomFilterForFilename(
 SafeBrowsingDatabaseNew::SafeBrowsingDatabaseNew(SafeBrowsingStore* store)
     : creation_loop_(MessageLoop::current()),
       store_(store),
-      ALLOW_THIS_IN_INITIALIZER_LIST(reset_factory_(this)) {
+      ALLOW_THIS_IN_INITIALIZER_LIST(reset_factory_(this)),
+      corruption_detected_(false) {
   DCHECK(store_.get());
 }
 
@@ -431,7 +432,7 @@ void SafeBrowsingDatabaseNew::InsertChunks(const std::string& list_name,
                                            const SBChunkList& chunks) {
   DCHECK_EQ(creation_loop_, MessageLoop::current());
 
-  if (chunks.empty())
+  if (corruption_detected_ || chunks.empty())
     return;
 
   const base::Time insert_start = base::Time::Now();
@@ -452,7 +453,7 @@ void SafeBrowsingDatabaseNew::DeleteChunks(
     const std::vector<SBChunkDelete>& chunk_deletes) {
   DCHECK_EQ(creation_loop_, MessageLoop::current());
 
-  if (chunk_deletes.empty())
+  if (corruption_detected_ || chunk_deletes.empty())
     return;
 
   const std::string& list_name = chunk_deletes.front().list_name;
@@ -526,12 +527,17 @@ bool SafeBrowsingDatabaseNew::UpdateStarted(
   lists->push_back(malware);
   lists->push_back(phishing);
 
+  corruption_detected_ = false;
+
   return true;
 }
 
 void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
   DCHECK_EQ(creation_loop_, MessageLoop::current());
 
+  if (corruption_detected_)
+    return;
+
   // Unroll any partially-received transaction.
   if (!update_succeeded) {
     store_->CancelUpdate();
@@ -644,6 +650,7 @@ void SafeBrowsingDatabaseNew::HandleCorruptDatabase() {
 
 void SafeBrowsingDatabaseNew::OnHandleCorruptDatabase() {
   UMA_HISTOGRAM_COUNTS("SB2.HandleCorrupt", 1);
+  corruption_detected_ = true;  // Stop updating the database.
   ResetDatabase();
   DCHECK(false) << "SafeBrowsing database was corrupt and reset";
 }
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.h b/chrome/browser/safe_browsing/safe_browsing_database.h
index f2c63a7..4120afd 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.h
+++ b/chrome/browser/safe_browsing/safe_browsing_database.h
@@ -135,9 +135,12 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   void WriteBloomFilter();
 
   // Helpers for handling database corruption.
-  // OnHandleCorruptDatabase() runs ResetDatabase(),
-  // HandleCorruptDatabase() posts OnHandleCorruptDatabase() to the
-  // current thread.
+  // |OnHandleCorruptDatabase()| runs |ResetDatabase()| and sets
+  // |corruption_detected_|, |HandleCorruptDatabase()| posts
+  // |OnHandleCorruptDatabase()| to the current thread, to be run
+  // after the current task completes.
+  // TODO(shess): Wire things up to entirely abort the update
+  // transaction when this happens.
   void HandleCorruptDatabase();
   void OnHandleCorruptDatabase();
 
@@ -178,6 +181,11 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
 
   // Used to schedule resetting the database because of corruption.
   ScopedRunnableMethodFactory<SafeBrowsingDatabaseNew> reset_factory_;
+
+  // Set if corruption is detected during the course of an update.
+  // Causes the update functions to fail with no side effects, until
+  // the next call to |UpdateStarted()|.
+  bool corruption_detected_;
 };
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_DATABASE_H_
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc b/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
index ba96c84..0c636ef 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
@@ -290,7 +290,10 @@ void SafeBrowsingDatabaseBloom::UpdateFinished(bool update_succeeded) {
   // We won't need the chunk caches until the next update (which will read them
   // from the database), so free their memory as they may contain thousands of
   // entries.
-  ClearUpdateCaches();
+  {
+    AutoLock lock(lookup_lock_);
+    ClearUpdateCaches();
+  }
 }
 
 bool SafeBrowsingDatabaseBloom::Open() {
@@ -1403,7 +1406,7 @@ bool SafeBrowsingDatabaseBloom::WriteChunkNumbers() {
 }
 
 void SafeBrowsingDatabaseBloom::ClearUpdateCaches() {
-  AutoLock lock(lookup_lock_);
+  lookup_lock_.AssertAcquired();
   add_del_cache_.clear();
   sub_del_cache_.clear();
   add_chunk_cache_.clear();
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_bloom.h b/chrome/browser/safe_browsing/safe_browsing_database_bloom.h
index 0f287fa..df3e1a6 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_bloom.h
+++ b/chrome/browser/safe_browsing/safe_browsing_database_bloom.h
@@ -177,7 +177,8 @@ class SafeBrowsingDatabaseBloom : public SafeBrowsingDatabase {
   void ReadChunkNumbers();
   bool WriteChunkNumbers();
 
-  // Flush in memory temporary caches.
+  // Flush in-memory temporary caches.  |lookup_lock_| must be locked
+  // by caller.
   void ClearUpdateCaches();
 
   // Encode the list id in the lower bit of the chunk.
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_bloom_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_database_bloom_unittest.cc
index a7a86f0..4359cc5 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_bloom_unittest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database_bloom_unittest.cc
@@ -4,9 +4,13 @@
 //
 // Unit tests for the SafeBrowsing storage system.
 
+#include "app/sql/connection.h"
+#include "app/sql/statement.h"
+#include "base/debug_util.h"
 #include "base/file_util.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
+#include "base/message_loop.h"
 #include "base/path_service.h"
 #include "base/process_util.h"
 #include "base/scoped_temp_dir.h"
@@ -20,6 +24,7 @@
 #include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
+#include "third_party/sqlite/sqlite3.h"
 
 using base::Time;
 
@@ -37,6 +42,103 @@ SBFullHash Sha256Hash(const std::string& str) {
   return hash;
 }
 
+// Prevent DCHECK from killing tests.
+// TODO(shess): Pawel disputes the use of this, so the test which uses
+// it is DISABLED.  http://crbug.com/56448
+class ScopedLogMessageIgnorer {
+ public:
+  ScopedLogMessageIgnorer() {
+    logging::SetLogMessageHandler(&LogMessageIgnorer);
+  }
+  ~ScopedLogMessageIgnorer() {
+    // TODO(shess): Would be better to verify whether anyone else
+    // changed it, and then restore it to the previous value.
+    logging::SetLogMessageHandler(NULL);
+  }
+
+ private:
+  static bool LogMessageIgnorer(int severity, const std::string& str) {
+    // Intercept FATAL, strip the stack backtrace, and log it without
+    // the crash part.
+    if (severity == logging::LOG_FATAL) {
+      size_t newline = str.find('\n');
+      if (newline != std::string::npos) {
+        const std::string msg = str.substr(0, newline + 1);
+        fprintf(stderr, "%s", msg.c_str());
+        fflush(stderr);
+      }
+      return true;
+    }
+
+    return false;
+  }
+};
+
+// Helper function which corrupts the root page of the indicated
+// table.  After this the table can be opened successfully, and
+// queries to other tables work, and possibly queries to this table
+// which only hit an index may work, but queries which hit the table
+// itself should not.  Returns |true| on success.
+bool CorruptSqliteTable(const FilePath& filename,
+                        const std::string& table_name) {
+  size_t root_page;  // Root page of the table.
+  size_t page_size;  // Page size of the database.
+
+  sql::Connection db;
+  if (!db.Open(filename))
+    return false;
+
+  sql::Statement stmt(db.GetUniqueStatement("PRAGMA page_size"));
+  if (!stmt.Step())
+    return false;
+  page_size = stmt.ColumnInt(0);
+
+  stmt.Assign(db.GetUniqueStatement(
+                  "SELECT rootpage FROM sqlite_master WHERE name = ?"));
+  stmt.BindString(0, "sub_prefix");
+  if (!stmt.Step())
+    return false;
+  root_page = stmt.ColumnInt(0);
+
+  // The page numbers are 1-based.
+  const size_t root_page_offset = (root_page - 1) * page_size;
+
+  // Corrupt the file by overwriting the table's root page.
+  FILE* fp = file_util::OpenFile(filename, "r+");
+  if (!fp)
+    return false;
+
+  file_util::ScopedFILE file_closer(fp);
+  if (fseek(fp, root_page_offset, SEEK_SET) == -1)
+    return false;
+
+  for (size_t i = 0; i < page_size; ++i) {
+    fputc('!', fp);  // Character experimentally verified.
+  }
+
+  if (!file_util::CloseFile(fp))
+    return false;
+
+  file_closer.reset();
+  return true;
+}
+
+// Run a select against the named table to test for corruption.
+bool TestCorruptSqliteTable(const FilePath& filename,
+                            const std::string& table_name) {
+  sql::Connection db;
+  if (!db.Open(filename))
+    return false;
+
+  const std::string sql =
+      StringPrintf("SELECT COUNT(*) FROM \"%s\"", table_name.c_str());
+  sql::Statement stmt(db.GetUniqueStatement(sql.c_str()));
+  if (stmt.Step())
+    return false;
+
+  return db.GetErrorCode() == SQLITE_CORRUPT;
+}
+
 }  // namespace
 
 class SafeBrowsingDatabaseBloomTest : public PlatformTest {
@@ -921,6 +1023,73 @@ TEST_F(SafeBrowsingDatabaseBloomTest, HashCaching) {
                                       Time::Now()));
 }
 
+// Test that corrupt databases are appropriately handled, even if the
+// corruption is detected in the midst of the update.
+// TODO(shess): Disabled until ScopedLogMessageIgnorer resolved.
+// http://crbug.com/56448
+TEST_F(SafeBrowsingDatabaseBloomTest, DISABLED_SqliteCorruptionHandling) {
+  // Re-create the database in a captive message loop so that we can
+  // influence task-posting.
+  database_.reset();
+  MessageLoop loop(MessageLoop::TYPE_DEFAULT);
+  database_.reset(new SafeBrowsingDatabaseBloom);
+  database_->Init(database_filename_);
+
+  // This will cause an empty database to be created.
+  std::vector<SBListChunkRanges> lists;
+  EXPECT_TRUE(database_->UpdateStarted(&lists));
+  database_->UpdateFinished(true);
+
+  // Create a sub chunk to insert.
+  SBChunkList chunks;
+  SBChunk chunk;
+  SBChunkHost host;
+  host.host = Sha256Prefix("www.subbed.com/");
+  host.entry = SBEntry::Create(SBEntry::SUB_PREFIX, 1);
+  host.entry->set_chunk_id(7);
+  host.entry->SetChunkIdAtPrefix(0, 19);
+  host.entry->SetPrefixAt(0, Sha256Prefix("www.subbed.com/notevil1.html"));
+  chunk.chunk_number = 7;
+  chunk.is_add = false;
+  chunk.hosts.clear();
+  chunk.hosts.push_back(host);
+  chunks.clear();
+  chunks.push_back(chunk);
+
+  // Corrupt the |sub_prefix| table.
+  ASSERT_TRUE(CorruptSqliteTable(database_filename_, "sub_prefix"));
+
+  {
+    // Verify the corruption.  This will DCHECK, so suppress the crash.
+    ScopedLogMessageIgnorer ignorer;
+    LOG(INFO) << "Expect failed check on: database disk image is malformed";
+    ASSERT_TRUE(TestCorruptSqliteTable(database_filename_, "sub_prefix"));
+
+    // Start an update.  The insert will fail due to corruption.
+    EXPECT_TRUE(database_->UpdateStarted(&lists));
+    LOG(INFO) << "Expect failed check on: sqlite error 11";
+    database_->InsertChunks(safe_browsing_util::kMalwareList, chunks);
+
+    // TODO(shess): Would prefer to test that the database is still
+    // corrupt at this point, but the database is locked.
+
+    // Flush through the corruption-handler task.
+    LOG(INFO) << "Expect failed check on: SafeBrowsing database reset";
+    MessageLoop::current()->RunAllPending();
+    database_->UpdateFinished(true);
+  }
+
+  // Database was re-created, and is now not corrupt.
+  ASSERT_FALSE(TestCorruptSqliteTable(database_filename_, "sub_prefix"));
+
+  // This update succeeds.
+  EXPECT_TRUE(database_->UpdateStarted(&lists));
+  database_->InsertChunks(safe_browsing_util::kMalwareList, chunks);
+  database_->UpdateFinished(true);
+
+  database_.reset();
+}
+
 namespace {
 
 void PrintStat(const char* name) {
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
index a3ec50f..d381d55 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
@@ -4,9 +4,12 @@
 //
 // Unit tests for the SafeBrowsing storage system.
 
+#include "app/sql/connection.h"
+#include "app/sql/statement.h"
 #include "base/file_util.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
+#include "base/message_loop.h"
 #include "base/path_service.h"
 #include "base/process_util.h"
 #include "base/scoped_temp_dir.h"
@@ -17,6 +20,7 @@
 #include "chrome/browser/safe_browsing/protocol_parser.h"
 #include "chrome/browser/safe_browsing/safe_browsing_database.h"
 #include "chrome/browser/safe_browsing/safe_browsing_store_file.h"
+#include "chrome/browser/safe_browsing/safe_browsing_store_sqlite.h"
 #include "chrome/browser/safe_browsing/safe_browsing_store_unittest_helper.h"
 #include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -38,6 +42,89 @@ SBFullHash Sha256Hash(const std::string& str) {
   return hash;
 }
 
+// Prevent DCHECK from killing tests.
+// TODO(shess): Pawel disputes the use of this, so the test which uses
+// it is DISABLED.  http://crbug.com/56448
+class ScopedLogMessageIgnorer {
+ public:
+  ScopedLogMessageIgnorer() {
+    logging::SetLogMessageHandler(&LogMessageIgnorer);
+  }
+  ~ScopedLogMessageIgnorer() {
+    // TODO(shess): Would be better to verify whether anyone else
+    // changed it, and then restore it to the previous value.
+    logging::SetLogMessageHandler(NULL);
+  }
+
+ private:
+  static bool LogMessageIgnorer(int severity, const std::string& str) {
+    // Intercept FATAL, strip the stack backtrace, and log it without
+    // the crash part.
+    if (severity == logging::LOG_FATAL) {
+      size_t newline = str.find('\n');
+      if (newline != std::string::npos) {
+        const std::string msg = str.substr(0, newline + 1);
+        fprintf(stderr, "%s", msg.c_str());
+        fflush(stderr);
+      }
+      return true;
+    }
+
+    return false;
+  }
+};
+
+// Helper function which corrupts the root page of the indicated
+// table.  After this the table can be opened successfully, and
+// queries to other tables work, and possibly queries to this table
+// which only hit an index may work, but queries which hit the table
+// itself should not.  Returns |true| on success.
+bool CorruptSqliteTable(const FilePath& filename,
+                        const std::string& table_name) {
+  size_t root_page;  // Root page of the table.
+  size_t page_size;  // Page size of the database.
+
+  sql::Connection db;
+  if (!db.Open(filename))
+    return false;
+
+  sql::Statement stmt(db.GetUniqueStatement("PRAGMA page_size"));
+  if (!stmt.Step())
+    return false;
+  page_size = stmt.ColumnInt(0);
+
+  stmt.Assign(db.GetUniqueStatement(
+                  "SELECT rootpage FROM sqlite_master WHERE name = ?"));
+  stmt.BindString(0, "sub_prefix");
+  if (!stmt.Step())
+    return false;
+  root_page = stmt.ColumnInt(0);
+
+  // The page numbers are 1-based.
+  const size_t root_page_offset = (root_page - 1) * page_size;
+
+  // Corrupt the file by overwriting the table's root page.
+  FILE* fp = file_util::OpenFile(filename, "r+");
+  if (!fp)
+    return false;
+
+  file_util::ScopedFILE file_closer(fp);
+  if (fseek(fp, root_page_offset, SEEK_SET) == -1)
+    return false;
+
+  for (size_t i = 0; i < page_size; ++i) {
+    fputc('!', fp);  // Character experimentally verified.
+  }
+
+  // Close the file manually because if there is an error in the
+  // close, it's likely because the data could not be flushed to the
+  // file.
+  if (!file_util::CloseFile(file_closer.release()))
+    return false;
+
+  return true;
+}
+
 }  // namespace
 
 class SafeBrowsingDatabaseTest : public PlatformTest {
@@ -926,6 +1013,158 @@ TEST_F(SafeBrowsingDatabaseTest, HashCaching) {
                                       Time::Now()));
 }
 
+// Test that corrupt databases are appropriately handled, even if the
+// corruption is detected in the midst of the update.
+// TODO(shess): Disabled until ScopedLogMessageIgnorer resolved.
+// http://crbug.com/56448
+TEST_F(SafeBrowsingDatabaseTest, DISABLED_SqliteCorruptionHandling) {
+  // Re-create the database in a captive message loop so that we can
+  // influence task-posting.  Database specifically needs to the
+  // SQLite-backed.
+  database_.reset();
+  MessageLoop loop(MessageLoop::TYPE_DEFAULT);
+  SafeBrowsingStoreSqlite* store = new SafeBrowsingStoreSqlite();
+  database_.reset(new SafeBrowsingDatabaseNew(store));
+  database_->Init(database_filename_);
+
+  // This will cause an empty database to be created.
+  std::vector<SBListChunkRanges> lists;
+  EXPECT_TRUE(database_->UpdateStarted(&lists));
+  database_->UpdateFinished(true);
+
+  // Create a sub chunk to insert.
+  SBChunkList chunks;
+  SBChunk chunk;
+  SBChunkHost host;
+  host.host = Sha256Prefix("www.subbed.com/");
+  host.entry = SBEntry::Create(SBEntry::SUB_PREFIX, 1);
+  host.entry->set_chunk_id(7);
+  host.entry->SetChunkIdAtPrefix(0, 19);
+  host.entry->SetPrefixAt(0, Sha256Prefix("www.subbed.com/notevil1.html"));
+  chunk.chunk_number = 7;
+  chunk.is_add = false;
+  chunk.hosts.clear();
+  chunk.hosts.push_back(host);
+  chunks.clear();
+  chunks.push_back(chunk);
+
+  // Corrupt the |sub_prefix| table.
+  ASSERT_TRUE(CorruptSqliteTable(database_filename_, "sub_prefix"));
+
+  {
+    // The following code will cause DCHECKs, so suppress the crashes.
+    ScopedLogMessageIgnorer ignorer;
+
+    // Start an update.  The insert will fail due to corruption.
+    EXPECT_TRUE(database_->UpdateStarted(&lists));
+    LOG(INFO) << "Expect failed check on: sqlite error 11";
+    database_->InsertChunks(safe_browsing_util::kMalwareList, chunks);
+
+    // Database file still exists until the corruption handler has run.
+    EXPECT_TRUE(file_util::PathExists(database_filename_));
+
+    // Flush through the corruption-handler task.
+    LOG(INFO) << "Expect failed check on: SafeBrowsing database reset";
+    MessageLoop::current()->RunAllPending();
+  }
+
+  // Database file should not exist.
+  EXPECT_FALSE(file_util::PathExists(database_filename_));
+
+  // Finish the transaction.  This should short-circuit, so no
+  // DCHECKs.
+  database_->InsertChunks(safe_browsing_util::kMalwareList, chunks);
+  database_->UpdateFinished(true);
+
+  // Flush through any posted tasks.
+  MessageLoop::current()->RunAllPending();
+
+  // Database file should still not exist.
+  EXPECT_FALSE(file_util::PathExists(database_filename_));
+
+  // Run the update again successfully.
+  EXPECT_TRUE(database_->UpdateStarted(&lists));
+  database_->InsertChunks(safe_browsing_util::kMalwareList, chunks);
+  database_->UpdateFinished(true);
+  EXPECT_TRUE(file_util::PathExists(database_filename_));
+
+  database_.reset();
+}
+
+// Test that corrupt databases are appropriately handled, even if the
+// corruption is detected in the midst of the update.
+// TODO(shess): Disabled until ScopedLogMessageIgnorer resolved.
+// http://crbug.com/56448
+TEST_F(SafeBrowsingDatabaseTest, DISABLED_FileCorruptionHandling) {
+  // Re-create the database in a captive message loop so that we can
+  // influence task-posting.  Database specifically needs to the
+  // file-backed.
+  database_.reset();
+  MessageLoop loop(MessageLoop::TYPE_DEFAULT);
+  SafeBrowsingStoreFile* store = new SafeBrowsingStoreFile();
+  database_.reset(new SafeBrowsingDatabaseNew(store));
+  database_->Init(database_filename_);
+
+  // This will cause an empty database to be created.
+  std::vector<SBListChunkRanges> lists;
+  EXPECT_TRUE(database_->UpdateStarted(&lists));
+  database_->UpdateFinished(true);
+
+  // Create a sub chunk to insert.
+  SBChunkList chunks;
+  SBChunk chunk;
+  SBChunkHost host;
+  host.host = Sha256Prefix("www.subbed.com/");
+  host.entry = SBEntry::Create(SBEntry::SUB_PREFIX, 1);
+  host.entry->set_chunk_id(7);
+  host.entry->SetChunkIdAtPrefix(0, 19);
+  host.entry->SetPrefixAt(0, Sha256Prefix("www.subbed.com/notevil1.html"));
+  chunk.chunk_number = 7;
+  chunk.is_add = false;
+  chunk.hosts.clear();
+  chunk.hosts.push_back(host);
+  chunks.clear();
+  chunks.push_back(chunk);
+
+  // Corrupt the file by corrupting the checksum, which is not checked
+  // until the entire table is read in |UpdateFinished()|.
+  FILE* fp = file_util::OpenFile(database_filename_, "r+");
+  ASSERT_TRUE(fp);
+  ASSERT_NE(-1, fseek(fp, -8, SEEK_END));
+  for (size_t i = 0; i < 8; ++i) {
+    fputc('!', fp);
+  }
+  fclose(fp);
+
+  {
+    // The following code will cause DCHECKs, so suppress the crashes.
+    ScopedLogMessageIgnorer ignorer;
+
+    // Start an update.  The insert will fail due to corruption.
+    EXPECT_TRUE(database_->UpdateStarted(&lists));
+    database_->InsertChunks(safe_browsing_util::kMalwareList, chunks);
+    database_->UpdateFinished(true);
+
+    // Database file still exists until the corruption handler has run.
+    EXPECT_TRUE(file_util::PathExists(database_filename_));
+
+    // Flush through the corruption-handler task.
+    LOG(INFO) << "Expect failed check on: SafeBrowsing database reset";
+    MessageLoop::current()->RunAllPending();
+  }
+
+  // Database file should not exist.
+  EXPECT_FALSE(file_util::PathExists(database_filename_));
+
+  // Run the update again successfully.
+  EXPECT_TRUE(database_->UpdateStarted(&lists));
+  database_->InsertChunks(safe_browsing_util::kMalwareList, chunks);
+  database_->UpdateFinished(true);
+  EXPECT_TRUE(file_util::PathExists(database_filename_));
+
+  database_.reset();
+}
+
 namespace {
 
 void PrintStat(const char* name) {
diff --git a/chrome/browser/safe_browsing/safe_browsing_store_file.cc b/chrome/browser/safe_browsing/safe_browsing_store_file.cc
index de3d7ee..032aa6e 100644
--- a/chrome/browser/safe_browsing/safe_browsing_store_file.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_store_file.cc
@@ -146,6 +146,29 @@ void DeleteChunksFromSet(const base::hash_set<int32>& deleted,
   }
 }
 
+// Sanity-check the header against the file's size to make sure our
+// vectors aren't gigantic.  This doubles as a cheap way to detect
+// corruption without having to checksum the entire file.
+bool FileHeaderSanityCheck(const FilePath& filename,
+                           const FileHeader& header) {
+  int64 size = 0;
+  if (!file_util::GetFileSize(filename, &size))
+    return false;
+
+  int64 expected_size = sizeof(FileHeader);
+  expected_size += header.add_chunk_count * sizeof(int32);
+  expected_size += header.sub_chunk_count * sizeof(int32);
+  expected_size += header.add_prefix_count * sizeof(SBAddPrefix);
+  expected_size += header.sub_prefix_count * sizeof(SBSubPrefix);
+  expected_size += header.add_hash_count * sizeof(SBAddFullHash);
+  expected_size += header.sub_hash_count * sizeof(SBSubFullHash);
+  expected_size += sizeof(MD5Digest);
+  if (size != expected_size)
+    return false;
+
+  return true;
+}
+
 }  // namespace
 
 SafeBrowsingStoreFile::SafeBrowsingStoreFile()
@@ -279,24 +302,9 @@ bool SafeBrowsingStoreFile::BeginUpdate() {
     return true;
   }
 
-  // Check that the file size makes sense given the header.  This is a
-  // cheap way to protect against header corruption while deferring
-  // the checksum calculation until the end of the update.
   // TODO(shess): Under POSIX it is possible that this could size a
   // file different from the file which was opened.
-  int64 size = 0;
-  if (!file_util::GetFileSize(filename_, &size))
-    return OnCorruptDatabase();
-
-  int64 expected_size = sizeof(FileHeader);
-  expected_size += header.add_chunk_count * sizeof(int32);
-  expected_size += header.sub_chunk_count * sizeof(int32);
-  expected_size += header.add_prefix_count * sizeof(SBAddPrefix);
-  expected_size += header.sub_prefix_count * sizeof(SBSubPrefix);
-  expected_size += header.add_hash_count * sizeof(SBAddFullHash);
-  expected_size += header.sub_hash_count * sizeof(SBSubFullHash);
-  expected_size += sizeof(MD5Digest);
-  if (size != expected_size)
+  if (!FileHeaderSanityCheck(filename_, header))
     return OnCorruptDatabase();
 
   // Pull in the chunks-seen data for purposes of implementing
@@ -390,6 +398,9 @@ bool SafeBrowsingStoreFile::DoUpdate(
     if (header.magic != kFileMagic || header.version != kFileVersion)
       return OnCorruptDatabase();
 
+    if (!FileHeaderSanityCheck(filename_, header))
+      return OnCorruptDatabase();
+
     // Re-read the chunks-seen data to get to the later data in the
     // file and calculate the checksum.  No new elements should be
     // added to the sets.
@@ -429,13 +440,32 @@ bool SafeBrowsingStoreFile::DoUpdate(
   if (!FileRewind(new_file_.get()))
     return false;
 
+  // Get chunk file's size for validating counts.
+  int64 size = 0;
+  if (!file_util::GetFileSize(TemporaryFileForFilename(filename_), &size))
+    return OnCorruptDatabase();
+
   // Append the accumulated chunks onto the vectors read from |file_|.
   for (int i = 0; i < chunks_written_; ++i) {
     ChunkHeader header;
 
+    int64 ofs = ftell(new_file_.get());
+    if (ofs == -1)
+      return false;
+
     if (!ReadArray(&header, 1, new_file_.get(), NULL))
       return false;
 
+    // As a safety measure, make sure that the header describes a sane
+    // chunk, given the remaining file size.
+    int64 expected_size = ofs + sizeof(ChunkHeader);
+    expected_size += header.add_prefix_count * sizeof(SBAddPrefix);
+    expected_size += header.sub_prefix_count * sizeof(SBSubPrefix);
+    expected_size += header.add_hash_count * sizeof(SBAddFullHash);
+    expected_size += header.sub_hash_count * sizeof(SBSubFullHash);
+    if (expected_size > size)
+      return false;
+
     // TODO(shess): If the vectors were kept sorted, then this code
     // could use std::inplace_merge() to merge everything together in
     // sorted order.  That might still be slower than just sorting at
diff --git a/chrome/browser/safe_browsing/safe_browsing_store_sqlite.cc b/chrome/browser/safe_browsing/safe_browsing_store_sqlite.cc
index 12b281b..486d6f2 100644
--- a/chrome/browser/safe_browsing/safe_browsing_store_sqlite.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_store_sqlite.cc
@@ -53,11 +53,6 @@ SafeBrowsingStoreSqlite::~SafeBrowsingStoreSqlite() {
 }
 
 bool SafeBrowsingStoreSqlite::Delete() {
-  // The database should not be open at this point.  TODO(shess): It
-  // can be open if corruption was detected while opening the
-  // database.  Ick.
-  DCHECK(!db_);
-
   // The file must be closed, both so that the journal file is deleted
   // by SQLite, and because open files cannot be deleted on Windows.
   if (!Close()) {
@@ -96,8 +91,12 @@ bool SafeBrowsingStoreSqlite::OnCorruptDatabase() {
 }
 
 bool SafeBrowsingStoreSqlite::Open() {
-  if (db_)
+  // This case should never happen, but if it does we shouldn't leak
+  // handles.
+  if (db_) {
+    NOTREACHED() << " Database was already open in Open().";
     return true;
+  }
 
   if (sqlite_utils::OpenSqliteDb(filename_, &db_) != SQLITE_OK) {
     sqlite3_close(db_);