|Summary:||Modern IDB: UniqueIDBDatabase's m_databaseInfo is unsafely used from multiple threads|
|Product:||WebKit||Reporter:||Brady Eidson <beidson>|
|Component:||WebCore Misc.||Assignee:||Brady Eidson <beidson>|
|Severity:||Normal||CC:||achristensen, alecflett, ap, commit-queue, jsbell|
|Bug Depends on:|
Description Brady Eidson 2016-02-04 21:51:13 PST
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.
Comment 1 Brady Eidson 2016-02-04 21:58:07 PST
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.
Comment 2 Brady Eidson 2016-02-04 22:04:14 PST
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.
Comment 3 Brady Eidson 2016-02-05 09:31:00 PST
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.
Comment 4 Brady Eidson 2016-02-05 09:34:33 PST
(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.
Comment 5 Brady Eidson 2016-02-05 09:35:48 PST
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.
Comment 6 Brady Eidson 2016-02-05 10:55:43 PST
(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
Comment 7 Brady Eidson 2016-02-05 10:56:20 PST
(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.
Comment 9 WebKit Commit Bot 2016-02-05 14:30:09 PST
Comment on attachment 270760 [details] Patch v1 Clearing flags on attachment: 270760 Committed r196191: <http://trac.webkit.org/changeset/196191>
Comment 10 WebKit Commit Bot 2016-02-05 14:30:12 PST
All reviewed patches have been landed. Closing bug.