diff options
4 files changed, 245 insertions, 9 deletions
diff --git a/chrome/browser/safe_browsing/safe_browsing_store_file.cc b/chrome/browser/safe_browsing/safe_browsing_store_file.cc index 7186388..207a607 100644 --- a/chrome/browser/safe_browsing/safe_browsing_store_file.cc +++ b/chrome/browser/safe_browsing/safe_browsing_store_file.cc @@ -7,6 +7,9 @@ #include "base/callback.h" #include "base/md5.h" +// TODO(shess): Remove after migration. +#include "chrome/browser/safe_browsing/safe_browsing_store_sqlite.h" + namespace { // NOTE(shess): kFileMagic should not be a byte-wise palindrome, so @@ -206,6 +209,17 @@ bool SafeBrowsingStoreFile::Delete() { return false; } + // Also make sure any SQLite data is deleted. This should only be + // needed if a journal file is left from a crash and the database is + // reset before SQLite gets a chance to straighten things out. + // TODO(shess): Remove after migration. + SafeBrowsingStoreSqlite old_store; + old_store.Init( + filename_, + NewCallback(this, &SafeBrowsingStoreFile::HandleCorruptDatabase)); + if (!old_store.Delete()) + return false; + return true; } @@ -229,11 +243,12 @@ bool SafeBrowsingStoreFile::Close() { // Make sure the files are closed. file_.reset(); new_file_.reset(); + old_store_.reset(); return true; } bool SafeBrowsingStoreFile::BeginUpdate() { - DCHECK(!file_.get() && !new_file_.get()); + DCHECK(!file_.get() && !new_file_.get() && !old_store_.get()); // Structures should all be clear unless something bad happened. DCHECK(add_chunks_cache_.empty()); @@ -267,8 +282,34 @@ bool SafeBrowsingStoreFile::BeginUpdate() { if (!ReadArray(&header, 1, file.get(), NULL)) return OnCorruptDatabase(); - if (header.magic != kFileMagic || header.version != kFileVersion) - return OnCorruptDatabase(); + if (header.magic != kFileMagic || header.version != kFileVersion) { + // Something about having the file open causes a problem with + // SQLite opening it. Perhaps PRAGMA locking_mode = EXCLUSIVE? + file.reset(); + + // Magic numbers didn't match, maybe it's a SQLite database. + scoped_ptr<SafeBrowsingStoreSqlite> + sqlite_store(new SafeBrowsingStoreSqlite()); + sqlite_store->Init( + filename_, + NewCallback(this, &SafeBrowsingStoreFile::HandleCorruptDatabase)); + if (!sqlite_store->BeginUpdate()) + return OnCorruptDatabase(); + + // Pull chunks-seen data into local structures, rather than + // optionally wiring various calls through to the SQLite store. + std::vector<int32> chunks; + sqlite_store->GetAddChunks(&chunks); + add_chunks_cache_.insert(chunks.begin(), chunks.end()); + + sqlite_store->GetSubChunks(&chunks); + sub_chunks_cache_.insert(chunks.begin(), chunks.end()); + + new_file_.swap(new_file); + old_store_.swap(sqlite_store); + + return true; + } // Check that the file size makes sense given the header. This is a // cheap way to protect against header corruption while deferring @@ -333,7 +374,7 @@ bool SafeBrowsingStoreFile::DoUpdate( const std::vector<SBAddFullHash>& pending_adds, std::vector<SBAddPrefix>* add_prefixes_result, std::vector<SBAddFullHash>* add_full_hashes_result) { - DCHECK(file_.get() || empty_); + DCHECK(old_store_.get() || file_.get() || empty_); DCHECK(new_file_.get()); std::vector<SBAddPrefix> add_prefixes; @@ -341,8 +382,30 @@ bool SafeBrowsingStoreFile::DoUpdate( std::vector<SBAddFullHash> add_full_hashes; std::vector<SBSubFullHash> sub_full_hashes; - // Read |file_| into the vectors. - if (!empty_) { + // Read |old_store_| into the vectors. + if (old_store_.get()) { + // Push deletions to |old_store_| so they can be applied to the + // data being read. + for (base::hash_set<int32>::const_iterator iter = add_del_cache_.begin(); + iter != add_del_cache_.end(); ++iter) { + old_store_->DeleteAddChunk(*iter); + } + for (base::hash_set<int32>::const_iterator iter = sub_del_cache_.begin(); + iter != sub_del_cache_.end(); ++iter) { + old_store_->DeleteSubChunk(*iter); + } + + if (!old_store_->ReadAddPrefixes(&add_prefixes) || + !old_store_->ReadSubPrefixes(&sub_prefixes) || + !old_store_->ReadAddHashes(&add_full_hashes) || + !old_store_->ReadSubHashes(&sub_full_hashes)) + return OnCorruptDatabase(); + + // Do not actually update the old store. + if (!old_store_->CancelUpdate()) + return OnCorruptDatabase(); + } else if (!empty_) { + // Read |file_| into the vectors. DCHECK(file_.get()); if (!FileRewind(file_.get())) @@ -479,9 +542,16 @@ bool SafeBrowsingStoreFile::DoUpdate( // Close the file handle and swizzle the file into place. new_file_.reset(); - if (!file_util::Delete(filename_, false) && - file_util::PathExists(filename_)) - return false; + if (old_store_.get()) { + const bool deleted = old_store_->Delete(); + old_store_.reset(); + if (!deleted) + return false; + } else { + if (!file_util::Delete(filename_, false) && + file_util::PathExists(filename_)) + return false; + } const FilePath new_filename = TemporaryFileForFilename(filename_); if (!file_util::Move(new_filename, filename_)) @@ -508,10 +578,12 @@ bool SafeBrowsingStoreFile::FinishUpdate( DCHECK(!new_file_.get()); DCHECK(!file_.get()); + DCHECK(!old_store_.get()); return Close(); } bool SafeBrowsingStoreFile::CancelUpdate() { + old_store_.reset(); return Close(); } diff --git a/chrome/browser/safe_browsing/safe_browsing_store_file.h b/chrome/browser/safe_browsing/safe_browsing_store_file.h index 4e0cda9..3996442 100644 --- a/chrome/browser/safe_browsing/safe_browsing_store_file.h +++ b/chrome/browser/safe_browsing/safe_browsing_store_file.h @@ -13,6 +13,12 @@ #include "base/callback.h" #include "base/file_util.h" +// TODO(shess): Data is migrated from SafeBrowsingStoreSqlite as part +// of the next update. That way everyone doesn't pull a new database +// when the code rolls out (and the migration code isn't gnarly). +// After substantially everyone has migrated, the migration code can +// be removed. Make sure that it deletes any journal file. + // Implement SafeBrowsingStore in terms of a flat file. The file // format is pretty literal: // @@ -103,6 +109,9 @@ // often, consider retaining the last known-good file for recovery // purposes, rather than deleting it. +// TODO(shess): Remove after migration. +class SafeBrowsingStoreSqlite; + class SafeBrowsingStoreFile : public SafeBrowsingStore { public: SafeBrowsingStoreFile(); @@ -189,6 +198,12 @@ class SafeBrowsingStoreFile : public SafeBrowsingStore { // a convenience to the caller. bool OnCorruptDatabase(); + // Helper for creating a corruption callback for |old_store_|. + // TODO(shess): Remove after migration. + void HandleCorruptDatabase() { + OnCorruptDatabase(); + } + // Clear temporary buffers used to accumulate chunk data. bool ClearChunkBuffers() { // NOTE: .clear() doesn't release memory. @@ -231,6 +246,11 @@ class SafeBrowsingStoreFile : public SafeBrowsingStore { file_util::ScopedFILE new_file_; bool empty_; + // If the main file existed, but was an SQLite store, this is a + // handle to it. + // TODO(shess): Remove this (and all references) after migration. + scoped_ptr<SafeBrowsingStoreSqlite> old_store_; + // Cache of chunks which have been seen. Loaded from the database // on BeginUpdate() so that it can be queried during the // transaction. diff --git a/chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc index 8e6ee61..4a2d750 100644 --- a/chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc +++ b/chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "chrome/browser/safe_browsing/safe_browsing_store_file.h" +#include "chrome/browser/safe_browsing/safe_browsing_store_sqlite.h" #include "base/callback.h" #include "chrome/browser/safe_browsing/safe_browsing_store_unittest_helper.h" @@ -144,4 +145,143 @@ TEST_F(SafeBrowsingStoreFileTest, DetectsCorruption) { EXPECT_TRUE(corruption_detected_); } +// Info to build a trivial store for migration testing. +const int kAddChunk1 = 1; +const int kAddChunk2 = 3; +const int kAddChunk3 = 5; +const int kSubChunk1 = 2; +const int kSubChunk2 = 4; +const SBFullHash kHash1 = SBFullHashFromString("one"); +const SBFullHash kHash2 = SBFullHashFromString("two"); +const SBPrefix kPrefix1 = kHash1.prefix; +const SBPrefix kPrefix2 = kHash2.prefix; +const SBPrefix kPrefix3 = SBFullHashFromString("three").prefix; +const SBPrefix kPrefix4 = SBFullHashFromString("four").prefix; +const SBPrefix kPrefix5 = SBFullHashFromString("five").prefix; + +// Load the store with some data. +void LoadStore(SafeBrowsingStore* store) { + EXPECT_TRUE(store->BeginUpdate()); + EXPECT_TRUE(store->BeginChunk()); + + store->SetAddChunk(kAddChunk1); + store->SetSubChunk(kSubChunk1); + store->SetAddChunk(kAddChunk2); + + EXPECT_TRUE(store->WriteAddPrefix(kAddChunk1, kPrefix1)); + EXPECT_TRUE(store->WriteAddPrefix(kAddChunk1, kPrefix2)); + EXPECT_TRUE(store->WriteAddPrefix(kAddChunk2, kPrefix3)); + EXPECT_TRUE(store->WriteSubPrefix(kSubChunk1, kAddChunk3, kPrefix4)); + EXPECT_TRUE(store->WriteAddHash(kAddChunk1, base::Time::Now(), kHash1)); + EXPECT_TRUE(store->WriteSubHash(kSubChunk1, kAddChunk1, kHash2)); + + EXPECT_TRUE(store->FinishChunk()); + + std::vector<SBAddFullHash> pending_adds; + std::vector<SBAddPrefix> add_prefixes; + std::vector<SBAddFullHash> add_hashes; + EXPECT_TRUE(store->FinishUpdate(pending_adds, &add_prefixes, &add_hashes)); + EXPECT_EQ(3U, add_prefixes.size()); +} + +// Verify that the store looks like what results from LoadStore(), and +// update it. +void UpdateStore(SafeBrowsingStore* store) { + EXPECT_TRUE(store->BeginUpdate()); + EXPECT_TRUE(store->BeginChunk()); + + // The chunks in the database should be the same. + std::vector<int> add_chunks; + store->GetAddChunks(&add_chunks); + ASSERT_EQ(2U, add_chunks.size()); + EXPECT_EQ(kAddChunk1, add_chunks[0]); + EXPECT_EQ(kAddChunk2, add_chunks[1]); + + std::vector<int> sub_chunks; + store->GetSubChunks(&sub_chunks); + ASSERT_EQ(1U, sub_chunks.size()); + EXPECT_EQ(kSubChunk1, sub_chunks[0]); + + EXPECT_TRUE(store->CheckAddChunk(kAddChunk1)); + EXPECT_TRUE(store->CheckSubChunk(kSubChunk1)); + EXPECT_TRUE(store->CheckAddChunk(kAddChunk2)); + + EXPECT_FALSE(store->CheckAddChunk(kAddChunk3)); + store->SetAddChunk(kAddChunk3); + // This one already has a sub. + EXPECT_TRUE(store->WriteAddPrefix(kAddChunk3, kPrefix4)); + EXPECT_TRUE(store->WriteAddPrefix(kAddChunk3, kPrefix5)); + + EXPECT_FALSE(store->CheckSubChunk(kSubChunk2)); + store->SetSubChunk(kSubChunk2); + EXPECT_TRUE(store->WriteSubPrefix(kSubChunk2, kAddChunk1, kPrefix1)); + + EXPECT_TRUE(store->FinishChunk()); + + store->DeleteAddChunk(kAddChunk2); + + std::vector<SBAddFullHash> pending_adds; + std::vector<SBAddPrefix> add_prefixes; + std::vector<SBAddFullHash> add_hashes; + EXPECT_TRUE(store->FinishUpdate(pending_adds, &add_prefixes, &add_hashes)); + EXPECT_EQ(2U, add_prefixes.size()); +} + +// Verify that the expected UpdateStore() data is present. +void CheckStore(SafeBrowsingStore* store) { + EXPECT_TRUE(store->BeginUpdate()); + + // The chunks in the database should be the same. + std::vector<int> add_chunks; + store->GetAddChunks(&add_chunks); + ASSERT_EQ(2U, add_chunks.size()); + EXPECT_EQ(kAddChunk1, add_chunks[0]); + EXPECT_EQ(kAddChunk3, add_chunks[1]); + + std::vector<int> sub_chunks; + store->GetSubChunks(&sub_chunks); + ASSERT_EQ(2U, sub_chunks.size()); + EXPECT_EQ(kSubChunk1, sub_chunks[0]); + EXPECT_EQ(kSubChunk2, sub_chunks[1]); + + EXPECT_TRUE(store->CancelUpdate()); +} + +// Verify that the migration sequence works as expected in the +// non-migration cases. +TEST_F(SafeBrowsingStoreFileTest, MigrateBaselineFile) { + LoadStore(store_.get()); + UpdateStore(store_.get()); + CheckStore(store_.get()); +} +TEST_F(SafeBrowsingStoreFileTest, MigrateBaselineSqlite) { + SafeBrowsingStoreSqlite sqlite_store; + sqlite_store.Init(filename_, NULL); + + LoadStore(&sqlite_store); + UpdateStore(&sqlite_store); + CheckStore(&sqlite_store); +} + +// The sequence should work exactly the same when we migrate from +// SQLite to file. +TEST_F(SafeBrowsingStoreFileTest, Migrate) { + // No existing store. + EXPECT_FALSE(file_util::PathExists(filename_)); + + { + SafeBrowsingStoreSqlite sqlite_store; + sqlite_store.Init(filename_, NULL); + + LoadStore(&sqlite_store); + } + + // At this point |filename_| references a SQLite store. + EXPECT_TRUE(file_util::PathExists(filename_)); + + // Update and check using a file store. + UpdateStore(store_.get()); + CheckStore(store_.get()); +} + } // namespace diff --git a/chrome/browser/safe_browsing/safe_browsing_store_sqlite.h b/chrome/browser/safe_browsing/safe_browsing_store_sqlite.h index 5e03d50..e72073b 100644 --- a/chrome/browser/safe_browsing/safe_browsing_store_sqlite.h +++ b/chrome/browser/safe_browsing/safe_browsing_store_sqlite.h @@ -102,6 +102,10 @@ class SafeBrowsingStoreSqlite : public SafeBrowsingStore { } private: + // For on-the-fly migration. + // TODO(shess): Remove (entire class) after migration. + friend class SafeBrowsingStoreFile; + // The following routines return true on success, or false on // failure. Failure is presumed to be persistent, so the caller // should stop trying and unwind the transaction. |