[Backport] CVE-2019-1369356-based

IndexedDB: Unregister cursors from transactions more consistently.

IndexedDBCursor now calls IndexedDBTransaction::UnregisterOpenCursor()
in Close(), which is called by the destructor.

The previous setup missed an edge case where calling
IndexedDBCursor::Close() directly would not unregister the cursor. This
behavior was relied upon in IndexedDBTransaction::CloseOpenCursors(),
but was not intended at other callsites.

(cherry picked from commit 23303e6f178ca2993bfde7114e6bdf48dd0eff9d)

Bug: 1005753
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#699937}
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/branch-heads/3865@{#872}
Cr-Branched-From: 0cdcc6158160790658d1f033d3db873603250124-refs/heads/master@{#681094}
Reviewed-by: Michal Klocek <michal.klocek@qt.io>
Change-Id: I9a16b513715e05b3c85e80f9103305f771590a45

2 files changed, 40 insertions, 3 deletions

diff --git a/chromium/content/browser/indexed_db/indexed_db_cursor.cc b/chromium/content/browser/indexed_db/indexed_db_cursor.cc
index a029b08242f..915c4276543 100644
--- a/chromium/content/browser/indexed_db/indexed_db_cursor.cc
+++ b/chromium/content/browser/indexed_db/indexed_db_cursor.cc
@@ -15,8 +15,21 @@
 #include "content/browser/indexed_db/indexed_db_tracing.h"
 #include "content/browser/indexed_db/indexed_db_transaction.h"
 #include "content/browser/indexed_db/indexed_db_value.h"
+#include "third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabaseException.h"
 
 namespace content {
+namespace {
+// This should never be script visible: the cursor should either be closed when
+// it hits the end of the range (and script throws an error before the call
+// could be made), if the transaction has finished (ditto), or if there's an
+// incoming request from the front end but the transaction has aborted on the
+// back end; in that case the tx will already have sent an abort to the request
+// so this would be ignored.
+IndexedDBDatabaseError CreateCursorClosedError() {
+  return IndexedDBDatabaseError(blink::WebIDBDatabaseExceptionUnknownError,
+                                "The cursor has been closed.");
+}
+} // namespace
 
 IndexedDBCursor::IndexedDBCursor(
     std::unique_ptr<IndexedDBBackingStore::Cursor> cursor,
@@ -32,7 +45,8 @@ IndexedDBCursor::IndexedDBCursor(
 }
 
 IndexedDBCursor::~IndexedDBCursor() {
-  transaction_->UnregisterOpenCursor(this);
+   // Call to make sure we complete our lifetime trace.
+   Close();
 }
 
 void IndexedDBCursor::Continue(std::unique_ptr<IndexedDBKey> key,
@@ -40,6 +54,11 @@ void IndexedDBCursor::Continue(std::unique_ptr<IndexedDBKey> key,
                                scoped_refptr<IndexedDBCallbacks> callbacks) {
   IDB_TRACE("IndexedDBCursor::Continue");
 
+  if (closed_) {
+    callbacks->OnError(CreateCursorClosedError());
+    return;
+  }
+
   transaction_->ScheduleTask(
       task_type_,
       base::Bind(&IndexedDBCursor::CursorIterationOperation,
@@ -53,6 +72,11 @@ void IndexedDBCursor::Advance(uint32_t count,
                               scoped_refptr<IndexedDBCallbacks> callbacks) {
   IDB_TRACE("IndexedDBCursor::Advance");
 
+  if (closed_) {
+    callbacks->OnError(CreateCursorClosedError());
+    return;
+  }
+
   transaction_->ScheduleTask(
       task_type_,
       base::Bind(
@@ -104,6 +128,11 @@ void IndexedDBCursor::PrefetchContinue(
     scoped_refptr<IndexedDBCallbacks> callbacks) {
   IDB_TRACE("IndexedDBCursor::PrefetchContinue");
 
+  if (closed_) {
+    callbacks->OnError(CreateCursorClosedError());
+    return;
+  }
+
   transaction_->ScheduleTask(
       task_type_,
       base::Bind(&IndexedDBCursor::CursorPrefetchIterationOperation,
@@ -198,10 +227,14 @@ leveldb::Status IndexedDBCursor::PrefetchReset(int used_prefetches,
 }
 
 void IndexedDBCursor::Close() {
+  if (closed_)
+    return;
   IDB_TRACE("IndexedDBCursor::Close");
   closed_ = true;
   cursor_.reset();
   saved_cursor_.reset();
+  if (transaction_)
+    transaction_->UnregisterOpenCursor(this);
 }
 
 }  // namespace content
diff --git a/chromium/content/browser/indexed_db/indexed_db_transaction.cc b/chromium/content/browser/indexed_db/indexed_db_transaction.cc
index 5040dddf49f..b3867a21b49 100644
--- a/chromium/content/browser/indexed_db/indexed_db_transaction.cc
+++ b/chromium/content/browser/indexed_db/indexed_db_transaction.cc
@@ -460,9 +460,13 @@ void IndexedDBTransaction::Timeout() {
 
 void IndexedDBTransaction::CloseOpenCursors() {
   IDB_TRACE1("IndexedDBTransaction::CloseOpenCursors", "txn.id", id());
-  for (auto* cursor : open_cursors_)
-    cursor->Close();
+
+  // IndexedDBCursor::Close() indirectly mutates |open_cursors_|, when it calls
+  // IndexedDBTransaction::UnregisterOpenCursor().
+  std::set<IndexedDBCursor*> open_cursors = std::move(open_cursors_);
   open_cursors_.clear();
+  for (auto* cursor : open_cursors)
+    cursor->Close();
 }
 
 void IndexedDBTransaction::AddPendingObserver(