diff options
| -rw-r--r-- | base/base.gyp | 1 | ||||
| -rw-r--r-- | base/scoped_bool.h | 24 | ||||
| -rw-r--r-- | chrome/browser/safe_browsing/safe_browsing_database_bloom.cc | 40 | ||||
| -rw-r--r-- | chrome/browser/safe_browsing/safe_browsing_database_bloom.h | 10 | ||||
| -rw-r--r-- | chrome/browser/safe_browsing/safe_browsing_service.cc | 34 | ||||
| -rw-r--r-- | chrome/browser/safe_browsing/safe_browsing_service.h | 3 |
6 files changed, 73 insertions, 39 deletions
diff --git a/base/base.gyp b/base/base.gyp index b954e3b..51aa6c8 100644 --- a/base/base.gyp +++ b/base/base.gyp @@ -251,6 +251,7 @@ 'resource_util.h', 'safe_strerror_posix.cc', 'safe_strerror_posix.h', + 'scoped_bool.h', 'scoped_bstr_win.cc', 'scoped_bstr_win.h', 'scoped_cftyperef.h', diff --git a/base/scoped_bool.h b/base/scoped_bool.h new file mode 100644 index 0000000..0622f967 --- /dev/null +++ b/base/scoped_bool.h @@ -0,0 +1,24 @@ +// Copyright (c) 2009 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. + +#ifndef BASE_SCOPED_BOOL_H_ +#define BASE_SCOPED_BOOL_H_ + +// ScopedBool is useful for setting a flag only during a particular scope. If +// you have code that has to add "var = false;" at all the exit points of a +// function, for example, you would benefit from using this instead. + +class ScopedBool { + public: + explicit ScopedBool(bool* scoped_bool) : scoped_bool_(scoped_bool) { + *scoped_bool_ = true; + } + ~ScopedBool() { *scoped_bool_ = false; } + + private: + bool* scoped_bool_; + DISALLOW_COPY_AND_ASSIGN(ScopedBool); +}; + +#endif // BASE_SCOPED_BOOL_H_ diff --git a/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc b/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc index 1f4d943..650f556 100644 --- a/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc +++ b/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc @@ -10,6 +10,7 @@ #include "base/message_loop.h" #include "base/platform_thread.h" #include "base/process_util.h" +#include "base/scoped_bool.h" #include "base/sha2.h" #include "base/stats_counters.h" #include "base/string_util.h" @@ -38,9 +39,9 @@ static const FilePath::CharType kBloomFilterFileSuffix[] = SafeBrowsingDatabaseBloom::SafeBrowsingDatabaseBloom() : db_(NULL), - init_(false), ALLOW_THIS_IN_INITIALIZER_LIST(reset_factory_(this)), - add_count_(0) { + add_count_(0), + performing_reset_(false) { } SafeBrowsingDatabaseBloom::~SafeBrowsingDatabaseBloom() { @@ -49,16 +50,17 @@ SafeBrowsingDatabaseBloom::~SafeBrowsingDatabaseBloom() { void SafeBrowsingDatabaseBloom::Init(const FilePath& filename, Callback0::Type* chunk_inserted_callback) { - DCHECK(!init_ && filename_.empty()); + DCHECK(filename_.empty()); // Ensure we haven't been run before. filename_ = FilePath(filename.value() + kBloomFilterFileSuffix); bloom_filter_filename_ = BloomFilterFilename(filename_); + // NOTE: There is no need to grab the lock in this function, since until it + // returns, there are no pointers to this class on other threads. hash_cache_.reset(new HashCache); LoadBloomFilter(); - init_ = true; chunk_inserted_callback_.reset(chunk_inserted_callback); } @@ -186,26 +188,33 @@ bool SafeBrowsingDatabaseBloom::CreateTables() { return true; } -// The SafeBrowsing service assumes this operation is run synchronously on the -// database thread. Any URLs that the service needs to check when this is -// running are queued up and run once the reset is done. bool SafeBrowsingDatabaseBloom::ResetDatabase() { - hash_cache_->clear(); - ClearUpdateCaches(); + // Open() can call us when trying to handle potential database corruption. + // Because we call Open() at the bottom of the function, we need to guard + // against recursion here. + if (performing_reset_) + return false; // Don't recurse. + + ScopedBool preforming_reset_scope_(&performing_reset_); + // Delete files on disk. bool rv = Close(); DCHECK(rv); - if (!file_util::Delete(filename_, false)) { NOTREACHED(); return false; } - DeleteBloomFilter(); - bloom_filter_ = new BloomFilter(BloomFilter::kBloomFilterMinSize * - BloomFilter::kBloomFilterSizeRatio); - // TODO(paulg): Fix potential infinite recursion between Open and Reset. + // Reset objects in memory. + { + AutoLock lock(lookup_lock_); + hash_cache_->clear(); + ClearUpdateCaches(); + bloom_filter_ = new BloomFilter(BloomFilter::kBloomFilterMinSize * + BloomFilter::kBloomFilterSizeRatio); + } + return Open(); } @@ -225,6 +234,7 @@ bool SafeBrowsingDatabaseBloom::CheckCompatibleVersion() { } void SafeBrowsingDatabaseBloom::ClearUpdateCaches() { + AutoLock lock(lookup_lock_); add_del_cache_.clear(); sub_del_cache_.clear(); add_chunk_cache_.clear(); @@ -1214,6 +1224,7 @@ void SafeBrowsingDatabaseBloom::BuildBloomFilter() { add_count_ = GetAddPrefixCount(); if (add_count_ == 0) { + AutoLock lock(lookup_lock_); bloom_filter_ = NULL; return; } @@ -1318,6 +1329,7 @@ void SafeBrowsingDatabaseBloom::GetCachedFullHashes( std::vector<SBFullHashResult>* full_hits, Time last_update) { DCHECK(prefix_hits && full_hits); + lookup_lock_.AssertAcquired(); Time max_age = Time::Now() - TimeDelta::FromMinutes(kMaxStalenessMinutes); diff --git a/chrome/browser/safe_browsing/safe_browsing_database_bloom.h b/chrome/browser/safe_browsing/safe_browsing_database_bloom.h index c6a65b3..296e4e649 100644 --- a/chrome/browser/safe_browsing/safe_browsing_database_bloom.h +++ b/chrome/browser/safe_browsing/safe_browsing_database_bloom.h @@ -178,9 +178,6 @@ class SafeBrowsingDatabaseBloom : public SafeBrowsingDatabase { // Cache of compiled statements for our database. scoped_ptr<SqliteStatementCache> statement_cache_; - // True iff the database has been opened successfully. - bool init_; - // Called after an add/sub chunk is processed. scoped_ptr<Callback0::Type> chunk_inserted_callback_; @@ -202,9 +199,14 @@ class SafeBrowsingDatabaseBloom : public SafeBrowsingDatabase { // Transaction for protecting database integrity during updates. scoped_ptr<SQLTransaction> insert_transaction_; - // Lock for protecting access to the bloom filter and hash cache. + // Lock for protecting access to variables that may be used on the IO thread. + // This includes |bloom_filter_|, |hash_cache_| and |prefix_miss_cache_|. Lock lookup_lock_; + // True if we're in the middle of a reset. This is used to prevent possible + // infinite recursion. + bool performing_reset_; + // A store for GetHash results that have not yet been written to the database. HashList pending_full_hashes_; diff --git a/chrome/browser/safe_browsing/safe_browsing_service.cc b/chrome/browser/safe_browsing/safe_browsing_service.cc index e59183b..cc01493 100644 --- a/chrome/browser/safe_browsing/safe_browsing_service.cc +++ b/chrome/browser/safe_browsing/safe_browsing_service.cc @@ -39,7 +39,6 @@ SafeBrowsingService::SafeBrowsingService() : database_(NULL), protocol_manager_(NULL), enabled_(false), - resetting_(false), update_in_progress_(false) { } @@ -66,7 +65,7 @@ bool SafeBrowsingService::CheckUrl(const GURL& url, Client* client) { if (!enabled_) return true; - if (!database_ || resetting_) { + if (!database_) { QueuedCheck check; check.client = client; check.url = url; @@ -114,7 +113,7 @@ void SafeBrowsingService::CancelCheck(Client* client) { } // Scan the queued clients store. Clients may be here if they requested a URL - // check before the database has finished loading or resetting. + // check before the database has finished loading. for (std::deque<QueuedCheck>::iterator it(queued_checks_.begin()); it != queued_checks_.end(); ++it) { if (it->client == client) @@ -261,7 +260,6 @@ void SafeBrowsingService::RegisterPrefs(PrefService* prefs) { void SafeBrowsingService::ResetDatabase() { DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO)); - resetting_ = true; safe_browsing_thread_->message_loop()->PostTask(FROM_HERE, NewRunnableMethod( this, &SafeBrowsingService::OnResetDatabase)); } @@ -317,7 +315,6 @@ void SafeBrowsingService::OnIOShutdown() { return; enabled_ = false; - resetting_ = false; // This cancels all in-flight GetHash requests. delete protocol_manager_; @@ -467,8 +464,7 @@ void SafeBrowsingService::DatabaseLoadComplete() { return; // If we have any queued requests, we can now check them. - if (!resetting_) - RunQueuedClients(); + RunQueuedClients(); } void SafeBrowsingService::HandleChunkForDatabase( @@ -545,7 +541,6 @@ void SafeBrowsingService::Start() { void SafeBrowsingService::OnResetDatabase() { DCHECK(MessageLoop::current() == safe_browsing_thread_->message_loop()); GetDatabase()->ResetDatabase(); - resetting_ = false; ChromeThread::PostTask( ChromeThread::IO, FROM_HERE, NewRunnableMethod(this, &SafeBrowsingService::DatabaseLoadComplete)); @@ -651,19 +646,22 @@ void SafeBrowsingService::ReportMalware(const GURL& malware_url, const GURL& referrer_url) { DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO)); - if (!enabled_ || !database_) + if (!enabled_) return; - // Check if 'page_url' is already blacklisted (exists in our cache). Only - // report if it's not there. - std::string list; - std::vector<SBPrefix> prefix_hits; - std::vector<SBFullHashResult> full_hits; - database_->ContainsUrl(page_url, &list, &prefix_hits, &full_hits, - protocol_manager_->last_update()); + if (database_) { + // Check if 'page_url' is already blacklisted (exists in our cache). Only + // report if it's not there. + std::string list; + std::vector<SBPrefix> prefix_hits; + std::vector<SBFullHashResult> full_hits; + database_->ContainsUrl(page_url, &list, &prefix_hits, &full_hits, + protocol_manager_->last_update()); + if (!full_hits.empty()) + return; + } - if (full_hits.empty()) - protocol_manager_->ReportMalware(malware_url, page_url, referrer_url); + protocol_manager_->ReportMalware(malware_url, page_url, referrer_url); } void SafeBrowsingService::RunQueuedClients() { diff --git a/chrome/browser/safe_browsing/safe_browsing_service.h b/chrome/browser/safe_browsing/safe_browsing_service.h index a2d5adb..9f138c8 100644 --- a/chrome/browser/safe_browsing/safe_browsing_service.h +++ b/chrome/browser/safe_browsing/safe_browsing_service.h @@ -274,9 +274,6 @@ class SafeBrowsingService // The SafeBrowsing thread that runs database operations. scoped_ptr<base::Thread> safe_browsing_thread_; - // Indicates if we are in the process of resetting the database. - bool resetting_; - // Indicates if we're currently in an update cycle. bool update_in_progress_; |