1565091 - Accesses beyond bounds in ConnectionPool

Status: RESOLVED DUPLICATE of bug 1481164

(Core :: Storage: IndexedDB, defect)

Description

[mozillabugs]

Attachment: bug_1483_str_2.htm

Various functions in class ConnectionPool (dom/indexedDB/ActorsParent.cpp) can write and/or read beyond bounds due to a thread race caused by missing mutexing.

The bug is that multiple threads can access ConnectionPool::mIdleDatabases (an nsTArray) concurrently without proper synchronization. I have STR using the debugger, but the problem occurs in the wild. I found it when running Mochitest dom/base/test/browser_promiseDocumentFlush_.js, subtest test_resolved_in_window_close via a release-assert crash in nsTArray::InsertElementAt() on an attempt to insert an element at index 2 in ConnectionPool::mIdleDatabases, which had 0 elements, but capacity 3. [1]

In my crash, InsertElementAt() was called by InsertElementSorted(), which made me curious, since there doesn't appear to be any way for InsertElementSorted() to insert an element beyond the array's end. This suggested that a thread race had emptied the array between the time InsertElementSorted() found the correct insert index (2), and the time it called InsertElementAt() with that index.

This is very similar to what I found after investigation, which is that an persistent NS_ERROR_STORAGE_BUSY error (in my case caused by paging on a memory-limited system) causes multiple threads to access ConnectionPool::mIdleDatabases without synchronization.

To reproduce the problem, put the STR file on a webserver, start an unoptimized build of FF 66.0.3 [2], and attach a debugger to the chrome process.

Set a BP on OpenDatabaseAndHandleBusy(), and load the STR file. Proceed BPs until you get one with the stack:

   OpenDatabaseAndHandleBusy() called by
   GetStorageConnection() called by
   GetStorageConnection() called by
   ConnectionPool::GetOrCreateConnection() called by
   Database::EnsureConnection()

where aDatabase->mMetadata->mRawPtr->mDatabaseId inside ConnectionPool::GetOrCreateConnection() is something like "2*http://127.0.0.1*0*theDB". "theDB" is the name of the database that the STR file uses.

Now use the debugger to simulate a persistent database-busy event by setting control to the statement

   if (NS_WARN_IF(NS_FAILED(rv))) {
      return rv;
   }

and setting rv to 0x80630001 (NS_ERROR_STORAGE_BUSY). DON'T PROCEED YET.

Set a BP on ConnectionPool::IdleConnectionRunnable::Run() on the statement

   connectionPool->NoteIdleDatabase(mDatabaseInfo);

and proceed. You might get another BP in OpenDatabaseAndHandleBusy() with the above stack; do the same thing as above.

When you get the BP in ConnectionPool::IdleConnectionRunnable::Run(), note the thread name and ID. The name will be something like "IndexedDB #<n>". Now use the debugger to freeze all the other threads in the process.

Step the current thread into ConnectionPool::NoteIdleDatabase() up to (but not including) the statement

   mIdleDatabases.InsertElementSorted(aDatabaseInfo);

Now freeze the "IndexedDB #<n>" thread and set a BP on every function that uses mIdleDatabases, preferably on the first line on which it uses that object. When you've done that, thaw all the threads BUT the "IndexedDB #<n>" thread and proceed.

You will get a BP on some other function that uses mIdleDatabases in the context of a thread other than the "IndexedDB #<n>" thread. [3] Use the debugger to verify that the object is at the same address in both threads [4]. If you want, you can actually step the collision by strategically thawing and freezing threads. If you happen to get the collision documented in note 3, you strategically can step the colliding

mIdleDatabases.InsertElementSorted(aDatabaseInfo);

statements to cause a write beyond bounds to element 2 of the array.

[1] This is likely very similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1209666 and possibly some other closed-as-irreproducible IndexedDB crashes.

[2] This code looks unchanged since then.

[3] In one test, I got a BP in ConnectionPool::NoteIdleDatabase() on the very same

mIdleDatabases.InsertElementSorted(aDatabaseInfo);

statement as in the frozen thread, attempting to insert aDatabaseInfo for the database "0chrome0*ActivityStream". Stategically proceeding here causes a write beyond bounds as one thread moves element 1 to the address of element 2, space for which neither thread allocated.

[4] There is only one ConnectionPool object, pointed to by gConnectionPool.

Comment 1

[:Gijs (he/him)]

Jan, are you the right person to look at this?

Comment 2

[Jan Varga [:janv]]

I don't have time to investigate this deeply, but I know that all methods that access mIdleDatabases call AssertIsOnOwningThread(), so it was designed to be accessed only on one thread.
Maybe Tom can take a look.

Comment 3

[Tom Tung [:tt, :ttung]]

I'll do some initial investigation for this soon.

Comment 4

[Tom Tung [:tt, :ttung]]

(In reply to mozillabugs from comment #0)

Hi,

First of all thanks for opening this issue and providing a document to reproduce.

I couldn't reproduce the issue on current Nightly 70. Maybe However, I wonder whether the issue has been solved in FF68 in this bug 1481164 [1]. While FF is idling, IDB would dispatch task into wrong threads that's why there were some unexpected threading issue on ConntionPool (such as crashes). And, the issue has been fixed since FF 68 (and was uplifted to 67).

Could you help me to try to do your STR locally on FF68 or the currently Nightly (70)? Thanks!

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1481164

Comment 5

[mozillabugs]

(In reply to Tom Tung [:tt, :ttung] from comment #4)

(In reply to mozillabugs from comment #0)

Hi,

First of all thanks for opening this issue and providing a document to reproduce.

I couldn't reproduce the issue on current Nightly 70. Maybe However, I wonder whether the issue has been solved in FF68 in this bug 1481164 [1]. While FF is idling, IDB would dispatch task into wrong threads that's why there were some unexpected threading issue on ConntionPool (such as crashes). And, the issue has been fixed since FF 68 (and was uplifted to 67).

Could you help me to try to do your STR locally on FF68 or the currently Nightly (70)? Thanks!

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1481164

I will test this on FF68. Is there an official debug/nonoptimized build?

Comment 6

[Daniel Veditz [:dveditz]]

There aren't "official" ones, but you can get close from treeherder builds. Our debug builds are still optimized, though.
https://treeherder.mozilla.org/#/jobs?repo=mozilla-release

Comment 7

[mozillabugs]

The unconditional Dispatch() added by https://bugzilla.mozilla.org/show_bug.cgi?id=1481164 appears to have fixed this bug in FF 68. I cannot reproduce it anymore.

Comment 8

[Tom Tung [:tt, :ttung]]

(In reply to mozillabugs from comment #7)

The unconditional Dispatch() added by https://bugzilla.mozilla.org/show_bug.cgi?id=1481164 appears to have fixed this bug in FF 68. I cannot reproduce it anymore.

Thanks! Let's close this ticket because it seems to be fixed on bug 1481164. Please feel free to reopen it if we find out the issue is still there.