Bug 153912 – Modern IDB: UniqueIDBDatabase's m_databaseInfo is unsafely used from multiple threads

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Modern IDB: UniqueIDBDatabase's m_databaseInfo is used from multiple threads

It's meant to be assigned and used on the main thread, but in UniqueIDBDatabase::performPutOrAdd, which is a database thread method:
    auto objectStoreInfo = m_databaseInfo->infoForExistingObjectStore(objectStoreIdentifier);

Yikes!

Noticed this in a random audit, don't know of any actual fallout, but it's obviously not safe.

[Brady Eidson]

Created attachment 270728 [details]
Patch for moving between machines

This is the patch I plan on fixing up and landing tomorrow from a different machine - attaching it here so I don't lose it.

[Brady Eidson]

In the meanwhile - while fixing this specific instance is easy - I'm trying to come up with a good design pattern to apply here to actually prevent this type of coding mistake.

One idea is that UniqueIDBDatabase inherits from some class - like UniqueIDBDatabaseBase - and that class is the keeper of thread-sensitive objects, such as m_databaseInfo.

Additionally, the data members themselves are private, and access to them is only allowed through protected methods.

The protector methods that allow access could have ASSERT_WITH_SECURITY_IMPLICATION checks making sure the access is on the expected thread (e.g., m_databaseInfo only ever on the main thread, m_backingStore only ever on the database server thread, etc etc)

It would still be *possible* for the caller to save off their own pointer to the object that could be incorrectly used on another thread, but that would appear much more suspicious in any patch review and/or audit.

CC'ing Alexey here for comment and ideas, since he's spent a lot of time dwelling on thread safety issues.

[Brady Eidson]

Sending the entire IDBObjectStoreInfo over for every put/add might be overkill - I'll look closely and see if sending some pieces over piecemeal might be lighter weight.

For example - We don't need a copy of each IDBIndexInfo that might exist in the object store.

[Brady Eidson]

(In reply to comment #3)
> Sending the entire IDBObjectStoreInfo over for every put/add might be
> overkill - I'll look closely and see if sending some pieces over piecemeal
> might be lighter weight.
> 
> For example - We don't need a copy of each IDBIndexInfo that might exist in
> the object store.

We can go much lighter weight for the Memory backing store, but the SQLite backing store requires the whole thing to walk the indexes.

The Memory backing store does NOT need this because it saves its own copy of IDBIndexInfos in each MemoryIndex when it is created.

That seems much more efficient than passing over a full copy for every put or add.

[Brady Eidson]

All UniqueIDBDatabase needs for its logic is:
-object store identifier
-autoincrement flag
-object store key path

If the SQLite backing store maintained its own copy of the database info, it could also get buy with just that.

[Brady Eidson]

(In reply to comment #5)
> If the SQLite backing store maintained its own copy of the database info, it
> could also get buy with just that.

There is, of course, complexity in doing this properly which is:
-Keeping it updated during version change transactions
-Reverting it on version change abort

[Brady Eidson]

(In reply to comment #6)
> (In reply to comment #5)
> > If the SQLite backing store maintained its own copy of the database info, it
> > could also get buy with just that.
> 
> There is, of course, complexity in doing this properly which is:
> -Keeping it updated during version change transactions
> -Reverting it on version change abort

Both are things we already do in the memory backing store, and will be covered by a myriad of tests, so it's definitely not insurmountable.

[Brady Eidson]

Created attachment 270760 [details]
Patch v1

[WebKit Commit Bot]

Comment on attachment 270760 [details]
Patch v1

Clearing flags on attachment: 270760

Committed r196191: <http://trac.webkit.org/changeset/196191>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.