M72: service worker: Make LevelDB database outlive its iterator to fi…

…x crash.

As explained in the bug by pwnall:
ServiceWorkerDatabase::GetRegistrationsForOrigin() obtains a LevelDB
iterator and calls ServiceWorkerDatabase::ReadResourceRecords() in a
loop. ReadResourceRecords() in turn calls
ServiceWorkerDatabase::HandleReadResult(), which may call
ServiceWorkerDatabase::Disable() if the status is a failure. Disable()
contains a "db_.reset()" which deletes the leveldb::DB instance. So,
ReadResourceRecords() may end up deleting the database before the
iterator used by GetRegistrationsForOrigin() is deleted. The contract
for leveldb::DB::NewIterator() [1] states that the iterator must be
deleted before the DB instance is deleted.

[1] https://cs.chromium.org/chromium/src/third_party/leveldatabase/src/include/leveldb/db.h?l=92&rcl=73d5834eceee8efa9a8ccfec77dc096a9e8ba18a

Bug: 909024
Change-Id: Ifee9aa0f7e1db9168d61b6407a11e249b2001986
Reviewed-on: https://chromium-review.googlesource.com/c/1354730
Commit-Queue: Matt Falkenhagen <[email protected]>
Reviewed-by: Hiroki Nakagawa <[email protected]>
Reviewed-by: Victor Costan <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#612599}(cherry picked from commit dd5d624)
Reviewed-on: https://chromium-review.googlesource.com/c/1362673
Reviewed-by: Matt Falkenhagen <[email protected]>
Cr-Commit-Position: refs/branch-heads/3626@{#65}
Cr-Branched-From: d897fb1-refs/heads/master@{#612437}

content/browser/service_worker/service_worker_database.cc

@@ -397,6 +397,8 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetRegistrationsForOrigin(
     return status;
 
   std::string prefix = CreateRegistrationKeyPrefix(origin);
+
+  // Read all registrations.
   {
     std::unique_ptr<leveldb::Iterator> itr(
         db_->NewIterator(leveldb::ReadOptions()));
@@ -421,21 +423,34 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetRegistrationsForOrigin(
         break;
       }
       registrations->push_back(registration);
+    }
+  }
 
-      if (opt_resources_list) {
-        std::vector<ResourceRecord> resources;
-        status = ReadResourceRecords(registration, &resources);
-        if (status != STATUS_OK) {
-          registrations->clear();
-          opt_resources_list->clear();
-          break;
-        }
-        opt_resources_list->push_back(resources);
+  // Count reading all registrations as one "read operation" for UMA
+  // purposes.
+  HandleReadResult(FROM_HERE, status);
+  if (status != STATUS_OK)
+    return status;
+
+  // Read the resources if requested. This must be done after the loop with
+  // leveldb::Iterator above, because it calls ReadResouceRecords() which
+  // deletes |db_| on failure, and iterators must be destroyed before the
+  // database.
+  if (opt_resources_list) {
+    for (const auto& registration : *registrations) {
+      std::vector<ResourceRecord> resources;
+      // NOTE: ReadResourceRecords already calls HandleReadResult() on its own,
+      // so to avoid double-counting the UMA, don't call it again after this.
+      status = ReadResourceRecords(registration, &resources);
+      if (status != STATUS_OK) {
+        registrations->clear();
+        opt_resources_list->clear();
+        break;
       }
+      opt_resources_list->push_back(resources);
     }
   }
 
-  HandleReadResult(FROM_HERE, status);
   return status;
 }
 

content/browser/service_worker/service_worker_database_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "content/browser/service_worker/service_worker_database.pb.h"
 #include "content/common/service_worker/service_worker_types.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -2042,4 +2043,64 @@ TEST(ServiceWorkerDatabaseTest, Corruption_NoMainResource) {
   EXPECT_TRUE(resources_out.empty());
 }
 
+// Tests that GetRegistrationsForOrigin() detects corruption without crashing.
+// It must delete the database after freeing the iterator it uses to read all
+// registrations. Regression test for https://crbug.com/909024.
+TEST(ServiceWorkerDatabaseTest, Corruption_GetRegistrationsForOrigin) {
+  std::unique_ptr<ServiceWorkerDatabase> database(CreateDatabaseInMemory());
+  ServiceWorkerDatabase::RegistrationData deleted_version;
+  std::vector<int64_t> newly_purgeable_resources;
+  std::vector<Resource> resources;
+  GURL origin("https://example.com");
+
+  // Write a normal registration.
+  RegistrationData data1;
+  data1.registration_id = 1;
+  data1.scope = URL(origin, "/foo");
+  data1.script = URL(origin, "/resource1");
+  data1.version_id = 1;
+  data1.resources_total_size_bytes = 2016;
+  resources = {CreateResource(1, URL(origin, "/resource1"), 2016)};
+  ASSERT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->WriteRegistration(data1, resources, &deleted_version,
+                                        &newly_purgeable_resources));
+
+  // Write a corrupt registration.
+  RegistrationData data2;
+  data2.registration_id = 2;
+  data2.scope = URL(origin, "/foo");
+  data2.script = URL(origin, "/resource2");
+  data2.version_id = 2;
+  data2.resources_total_size_bytes = 2016;
+  // Simulate that "/resource2" wasn't correctly written in the database by
+  // not adding it.
+  resources = {CreateResource(3, URL(origin, "/resource3"), 2016)};
+  ASSERT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->WriteRegistration(data2, resources, &deleted_version,
+                                        &newly_purgeable_resources));
+
+  // Call GetRegistrationsForOrigin(). It should detect corruption, and not
+  // crash.
+  base::HistogramTester histogram_tester;
+  std::vector<RegistrationData> registrations;
+  std::vector<std::vector<ServiceWorkerDatabase::ResourceRecord>>
+      resources_list;
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED,
+            database->GetRegistrationsForOrigin(origin, &registrations,
+                                                &resources_list));
+  EXPECT_TRUE(registrations.empty());
+  EXPECT_TRUE(resources_list.empty());
+
+  // There should be three "read" operations logged:
+  // 1. Reading all registration data.
+  // 2. Reading the resources of the first registration.
+  // 3. Reading the resources of the second registration. This one fails.
+  histogram_tester.ExpectTotalCount("ServiceWorker.Database.ReadResult", 3);
+  histogram_tester.ExpectBucketCount("ServiceWorker.Database.ReadResult",
+                                     ServiceWorkerDatabase::STATUS_OK, 2);
+  histogram_tester.ExpectBucketCount(
+      "ServiceWorker.Database.ReadResult",
+      ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED, 1);
+}
+
 }  // namespace content