From ebb70fea28ea6827759547662495ff05f3c58d4d Mon Sep 17 00:00:00 2001 From: Matt Falkenhagen Date: Wed, 5 Dec 2018 04:02:25 +0000 Subject: [PATCH] M72: service worker: Make LevelDB database outlive its iterator to fix 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 Reviewed-by: Hiroki Nakagawa Reviewed-by: Victor Costan Cr-Original-Commit-Position: refs/heads/master@{#612599}(cherry picked from commit dd5d624d6060fffbd79899ca9967715976b55ed0) Reviewed-on: https://chromium-review.googlesource.com/c/1362673 Reviewed-by: Matt Falkenhagen Cr-Commit-Position: refs/branch-heads/3626@{#65} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} --- .../service_worker/service_worker_database.cc | 35 ++++++++--- .../service_worker_database_unittest.cc | 61 +++++++++++++++++++ 2 files changed, 86 insertions(+), 10 deletions(-) diff --git a/content/browser/service_worker/service_worker_database.cc b/content/browser/service_worker/service_worker_database.cc index d652c99137f8..6f313b7e20e7 100644 --- a/content/browser/service_worker/service_worker_database.cc +++ b/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 itr( db_->NewIterator(leveldb::ReadOptions())); @@ -421,21 +423,34 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetRegistrationsForOrigin( break; } registrations->push_back(registration); + } + } - if (opt_resources_list) { - std::vector 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 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; } diff --git a/content/browser/service_worker/service_worker_database_unittest.cc b/content/browser/service_worker/service_worker_database_unittest.cc index 93cd04fda470..90a2b42fd045 100644 --- a/content/browser/service_worker/service_worker_database_unittest.cc +++ b/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 database(CreateDatabaseInMemory()); + ServiceWorkerDatabase::RegistrationData deleted_version; + std::vector newly_purgeable_resources; + std::vector 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 registrations; + std::vector> + resources_list; + EXPECT_EQ(ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED, + database->GetRegistrationsForOrigin(origin, ®istrations, + &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