[IndexedDB] Fix request reentry in IndexedDBDatabase

During ForceClose, a closing connection could cause the active request
to 'complete', triggering the rest of the requests to execute. Since
the connections are cleared after-the-fact in ForceClose(), this caused
a UAF.

Instead having specialized weakptr factories here, this change creates
a |force_closing_| variable which is set in ForceClose(), which is used
to ensure reentry doesn't occur.

R: pwnall@chromium.org
Bug: 966557
Change-Id: Iaaf678853431c35299dc9289b505fdf66c19a88e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1627707
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662926}

content/browser/indexed_db/indexed_db_database.cc

@@ -587,7 +587,7 @@ std::unique_ptr<IndexedDBConnection> IndexedDBDatabase::CreateConnection(
           base::BindRepeating(&IndexedDBDatabase::VersionChangeIgnored,
                               weak_factory_.GetWeakPtr()),
           base::BindOnce(&IndexedDBDatabase::ConnectionClosed,
-                         connection_close_weak_factory_.GetWeakPtr()),
+                         weak_factory_.GetWeakPtr()),
           error_callback_, database_callbacks);
   connections_.insert(connection.get());
   backing_store_->GrantChildProcessPermissions(child_process_id);
@@ -600,6 +600,8 @@ void IndexedDBDatabase::VersionChangeIgnored() {
 }
 
 void IndexedDBDatabase::ConnectionClosed(IndexedDBConnection* connection) {
+  if (force_closing_)
+    return;
   DCHECK(connections_.count(connection));
   DCHECK(connection->IsConnected());
   DCHECK(connection->transactions().empty());
@@ -2027,7 +2029,8 @@ void IndexedDBDatabase::ProcessRequestQueueAndMaybeRelease() {
 }
 
 void IndexedDBDatabase::MaybeReleaseDatabase() {
-  if (!active_request_ && pending_requests_.empty() && connections_.empty())
+  if (!active_request_ && pending_requests_.empty() && connections_.empty() &&
+      !force_closing_)
     std::move(destroy_me_).Run();
 }
 
@@ -2074,6 +2077,7 @@ void IndexedDBDatabase::ScheduleDeleteDatabase(
 }
 
 void IndexedDBDatabase::ForceClose() {
+  force_closing_ = true;
   // Remove all pending requests that don't want to execute during force close
   // (open requests).
   base::queue<std::unique_ptr<ConnectionRequest>> requests_to_still_run;
@@ -2088,12 +2092,14 @@ void IndexedDBDatabase::ForceClose() {
   if (!requests_to_still_run.empty())
     pending_requests_ = std::move(requests_to_still_run);
 
-  // Clear the weak pointers used to bind ConnectionClosed to prevent re-entry.
-  connection_close_weak_factory_.InvalidateWeakPtrs();
-  for (IndexedDBConnection* connection : connections_) {
+  // Since |force_closing_| is true, there are no re-entry modifications to
+  // this list by ConnectionClosed().
+  while (!connections_.empty()) {
+    IndexedDBConnection* connection = *connections_.begin();
     connection->CloseAndReportForceClose();
+    connections_.erase(connection);
   }
-  connections_.clear();
+  force_closing_ = false;
 
   // OnConnectionClosed usually synchronously calls RequestComplete.
   if (active_request_)

content/browser/indexed_db/indexed_db_database.h

@@ -401,6 +401,12 @@ class CONTENT_EXPORT IndexedDBDatabase {
 
   list_set<IndexedDBConnection*> connections_;
 
+  // During ForceClose(), the internal state can be inconsistent during cleanup,
+  // specifically for ConnectionClosed() and MaybeReleaseDatabase(). Keeping
+  // track of whether the code is currently in the ForceClose() method helps
+  // ensure that the state stays consistent.
+  bool force_closing_ = false;
+
   // This holds the first open or delete request that is currently being
   // processed. The request has already broadcast OnVersionChange if
   // necessary.
@@ -416,12 +422,7 @@ class CONTENT_EXPORT IndexedDBDatabase {
   // synchronously.
   bool processing_pending_requests_ = false;
 
-  // |connection_close_weak_factory_| is specifically used for binding the
-  // ConnectionClosed callback with connections. This lets us invalidate all
-  // connection callbacks during operations like ForceClose to prevent re-entry.
-  base::WeakPtrFactory<IndexedDBDatabase> connection_close_weak_factory_{this};
-  // |weak_factory_| is used for all non-ConnectionClosed callback uses. It is
-  // only invalidated when the object is destroyed.
+  // |weak_factory_| is used for all callback uses.
   base::WeakPtrFactory<IndexedDBDatabase> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(IndexedDBDatabase);

content/browser/indexed_db/indexed_db_database_unittest.cc

@@ -381,6 +381,55 @@ TEST_F(IndexedDBDatabaseTest, ForceCloseWhileOpenPending) {
   EXPECT_FALSE(db_);
 }
 
+TEST_F(IndexedDBDatabaseTest, ForceCloseWhileOpenAndDeletePending) {
+  // Verify that pending connection requests are handled correctly during a
+  // ForceClose.
+  auto request1 = base::MakeRefCounted<MockIndexedDBCallbacks>();
+  auto callbacks1 = base::MakeRefCounted<MockIndexedDBDatabaseCallbacks>();
+  const int64_t transaction_id1 = 1;
+  auto create_transaction_callback1 =
+      base::BindOnce(&CreateAndBindTransactionPlaceholder);
+  std::unique_ptr<IndexedDBPendingConnection> connection =
+      std::make_unique<IndexedDBPendingConnection>(
+          request1, callbacks1, kFakeChildProcessId, transaction_id1,
+          IndexedDBDatabaseMetadata::DEFAULT_VERSION,
+          std::move(create_transaction_callback1));
+  db_->ScheduleOpenConnection(IndexedDBOriginStateHandle(),
+                              std::move(connection));
+
+  EXPECT_EQ(db_->ConnectionCount(), 1UL);
+  EXPECT_EQ(db_->ActiveOpenDeleteCount(), 0UL);
+  EXPECT_EQ(db_->PendingOpenDeleteCount(), 0UL);
+
+  auto request2 = base::MakeRefCounted<MockIndexedDBCallbacks>(false);
+  auto callbacks2 = base::MakeRefCounted<MockIndexedDBDatabaseCallbacks>();
+  const int64_t transaction_id2 = 2;
+  auto create_transaction_callback2 =
+      base::BindOnce(&CreateAndBindTransactionPlaceholder);
+  std::unique_ptr<IndexedDBPendingConnection> connection2(
+      std::make_unique<IndexedDBPendingConnection>(
+          request1, callbacks1, kFakeChildProcessId, transaction_id2, 3,
+          std::move(create_transaction_callback2)));
+  db_->ScheduleOpenConnection(IndexedDBOriginStateHandle(),
+                              std::move(connection2));
+
+  bool deleted = false;
+  auto request3 = base::MakeRefCounted<MockCallbacks>();
+  db_->ScheduleDeleteDatabase(
+      IndexedDBOriginStateHandle(), request3,
+      base::BindLambdaForTesting([&]() { deleted = true; }));
+  EXPECT_FALSE(deleted);
+
+  EXPECT_EQ(db_->ConnectionCount(), 1UL);
+  EXPECT_EQ(db_->ActiveOpenDeleteCount(), 1UL);
+  EXPECT_EQ(db_->PendingOpenDeleteCount(), 1UL);
+
+  db_->ForceClose();
+
+  EXPECT_TRUE(deleted);
+  EXPECT_FALSE(db_);
+}
+
 leveldb::Status DummyOperation(IndexedDBTransaction* transaction) {
   return leveldb::Status::OK();
 }