PVer4: Test checksum on startup outside the hotpath of DB load

components/safe_browsing_db/v4_local_database_manager.cc

Index: components/safe_browsing_db/v4_local_database_manager.cc
diff --git a/components/safe_browsing_db/v4_local_database_manager.cc b/components/safe_browsing_db/v4_local_database_manager.cc
index 28320252fec77167878326768d915c7ef53f9ae8..f52b399f24c376e9a7463039c99f57548b62b60b 100644
--- a/components/safe_browsing_db/v4_local_database_manager.cc
+++ b/components/safe_browsing_db/v4_local_database_manager.cc
@@ -272,7 +272,7 @@ void V4LocalDatabaseManager::StopOnIOThread(bool shutdown) {
 // End: SafeBrowsingDatabaseManager implementation
 //
 
-void V4LocalDatabaseManager::DatabaseReady(
+void V4LocalDatabaseManager::DatabaseReadyForChecks(
     std::unique_ptr<V4Database> v4_database) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
@@ -281,9 +281,14 @@ void V4LocalDatabaseManager::DatabaseReady(
   if (enabled_) {
     v4_database_ = std::move(v4_database);
 
-    // The database is in place. Start fetching updates now.
-    v4_update_protocol_manager_->ScheduleNextUpdate(
-        v4_database_->GetStoreStateMap());
+    // The consistency of the stores read from the disk needs to verified. Post
+    // that task on the task runner. It calls |db_ready_for_updates_callback|
+    // callback with the stores to reset, if any, and then we can schedule the
+    // database updates.
+    DatabaseReadyForUpdatesCallback db_ready_for_updates_callback =
+        base::Bind(&V4LocalDatabaseManager::DatabaseReadyForUpdates,
+                   base::Unretained(this));
+    v4_database_->VerifyChecksum(db_ready_for_updates_callback);

[Scott Hess - ex-Googler, 2016/10/06 23:04:10]

OK, thinking out loud ... since this is enabled_, then StopOnIOThread() hasn't
been called, so there isn't an outstanding DeleteSoon().  But StopOnIOThread()
might be sent immediately after this.  In that case, the outstanding verify will
send the callback, which will short-circuit because of enabled_, and then delete
the database.

I think there's still a flaw in the threading.  For messages which go from the
manager to the db, enabled_ is sufficient, because the manager scopes enabled_
changes around db acquisition.  But StopOnIOThread() could be called while
VerifyChecksumOnTaskRunner() is in flight.  There's nothing which prevents the
client from calling the destructor immediately after StopOnIOThread() is done,
in which case the callback will use-after-free.

I think a similar sequence can be constructed for StopOnIOThread() versus the
callback from ApplyUpdate().  The only way I can think of solving it offhand is
a weak_ptr for both callbacks, so when the callback posts it is dropped on the
floor.  I think that's reasonable for a followup CL, though.

[vakh (use Gerrit instead), 2016/10/07 00:39:45]

Would that be Case 2 here:
http://go/design-doc-v4store-verifychecksum/edit#heading=h.9q94nrcrdfra

I think that's covered. PLMK if you are thinking of a different case.
http://crbug.com/647237 :)

[Scott Hess - ex-Googler, 2016/10/07 13:31:09]

I'm thinking:

tio: NewDatabaseReady
tio: StopOnIOThread
tio: ~V4LocalDatabaseManager
tr: VerifyChecksum
tio: UAF in DatabaseReadyForUpdates

Hmm ... is ~V4LocalDatabaseManager guaranteed never to run?  Like even at
shutdown?
Yes...

 
     ProcessQueuedChecks();
   } else {
@@ -292,6 +297,17 @@ void V4LocalDatabaseManager::DatabaseReady(
   }
 }
 
+void V4LocalDatabaseManager::DatabaseReadyForUpdates(
+    const std::unordered_set<ListIdentifier>& stores_to_reset) {
+  if (enabled_) {
+    v4_database_->ResetStores(stores_to_reset);
+
+    // The database is ready to process updates. Start fetching them now.
+    v4_update_protocol_manager_->ScheduleNextUpdate(
+        v4_database_->GetStoreStateMap());
+  }
+}
+
 void V4LocalDatabaseManager::DatabaseUpdated() {
   if (enabled_) {
     v4_update_protocol_manager_->ScheduleNextUpdate(
@@ -456,7 +472,7 @@ void V4LocalDatabaseManager::SetupDatabase() {
   // operation. Instead, do that on the task_runner and when the new database
   // has been created, swap it out on the IO thread.
   NewDatabaseReadyCallback db_ready_callback = base::Bind(
-      &V4LocalDatabaseManager::DatabaseReady, base::Unretained(this));
+      &V4LocalDatabaseManager::DatabaseReadyForChecks, base::Unretained(this));
   V4Database::Create(task_runner_, base_path_, list_infos_, db_ready_callback);
 }