New issue
Advanced search Search tips
Starred by 1 user
Status: Fixed
Owner:
Closed: Jul 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Security



Sign in to add a comment
SEGV in LocalWriteClosure::writeBlobToFileOnIOThread
Project Member Reported by cmumford@chromium.org, Jul 21 2014 Back to list
I believe we have a <100% reproducible memory access bug in LocalWriteClosure::writeBlobToFileOnIOThread. Normally run-webkit-tests spawns two content shells and all IDB tests pass 100% of the time. However, when fully parallel (with 32 cores) a crash in LocalWriteClosure::writeBlobToFileOnIOThread is easily reproducible (~80% of the time).

Most often the crashing test is: storage/indexeddb/blob-valid-before-commit.html, but have seen the same stack with storage/indexeddb/database-name-undefined.html and storage/indexeddb/database-odd-names.html.

Here is how I am invoking the layout tests:

python testing/xvfb.py out/Debug third_party/WebKit/Tools/Scripts/run-webkit-tests --no-show-results --debug --nocheck-sys-deps --clobber-old-results --no-retry-failures --no-new-test-results --fully-parallel storage/indexeddb
 
writeBlobToFileOnIOThread_stack.txt
70.1 KB View Download
Status: Started
There are crashes on this at crash.corp.google.com. They are M37 & M38, and one M36.
Project Member Comment 3 by bugdroid1@chromium.org, Jul 25 2014
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a1d5e0354ef792097296f63877e7adae3d1e1787

commit a1d5e0354ef792097296f63877e7adae3d1e1787
Author: cmumford@chromium.org <cmumford@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Fri Jul 25 00:17:06 2014

indexeddb: Removed use of dangling ptr in writeBlobToFileOnIOThread.

LocalWriteClosure only had a raw pointer to a ChainedBlobWriter which
was held by a scoped_refptr in IndexedDBBackingStore::Transaction. If
the transaction was deleted before writeBlobToFileOnIOThread was called
then a crash would ensue. Converted raw ptr to scoped_refptr prevents this.

BUG= 395650 

Review URL: https://codereview.chromium.org/417573004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@285428 0039d316-1c4b-4281-b951-d872f2087c98


Project Member Comment 4 by bugdroid1@chromium.org, Jul 25 2014
------------------------------------------------------------------
r285428 | cmumford@chromium.org | 2014-07-25T00:17:06.363849Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/indexed_db/indexed_db_active_blob_registry_unittest.cc?r1=285428&r2=285427&pathrev=285428
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/indexed_db/indexed_db_fake_backing_store.cc?r1=285428&r2=285427&pathrev=285428
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/indexed_db/indexed_db_cleanup_on_io_error_unittest.cc?r1=285428&r2=285427&pathrev=285428
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/indexed_db/indexed_db_fake_backing_store.h?r1=285428&r2=285427&pathrev=285428
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/indexed_db/indexed_db_context_impl.cc?r1=285428&r2=285427&pathrev=285428
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/indexed_db/indexed_db_context_impl.h?r1=285428&r2=285427&pathrev=285428
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/indexed_db/indexed_db_backing_store_unittest.cc?r1=285428&r2=285427&pathrev=285428
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/indexed_db/indexed_db_backing_store.cc?r1=285428&r2=285427&pathrev=285428
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/indexed_db/indexed_db_backing_store.h?r1=285428&r2=285427&pathrev=285428
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/browser/indexed_db_context.h?r1=285428&r2=285427&pathrev=285428

indexeddb: Removed use of dangling ptr in writeBlobToFileOnIOThread.

LocalWriteClosure only had a raw pointer to a ChainedBlobWriter which
was held by a scoped_refptr in IndexedDBBackingStore::Transaction. If
the transaction was deleted before writeBlobToFileOnIOThread was called
then a crash would ensue. Converted raw ptr to scoped_refptr prevents this.

BUG= 395650 

Review URL: https://codereview.chromium.org/417573004
-----------------------------------------------------------------
Status: Fixed
I will keep an eye on this CL and if it lands well will request merge into M37.
Labels: Merge-Requested
Requesting merge to M37. Has been in Canary for about 11 days with out reported problems.
Comment 7 by amin...@google.com, Aug 6 2014
Labels: merge-questions-applied

Please note that all merge requests must have been on or rolled into trunk
for at least 24 hours to be considered for merging (to ensure full bot
coverage and give an opportunity for any necessary reverts to occur).

To help facilitate this request, if you could please answer the following:
--------------------------------------------------------------------------
1) Has this change been on trunk for at least 24 hours?

2) Has this change shipped to at least one canary release (where applicable)?

3) Has anyone verified that these changes resolve the issue and cause no new
   crashes (via chromecrash/) or regressions?

4) Why is this necessary for this milestone?

Thanks!

(this message is auto-generated each time the merge-request label is
applied; if you have previously answered these questions kindly disregard)

1) Has this change been on trunk for at least 24 hours?

Yes, 12 days.

2) Has this change shipped to at least one canary release (where applicable)?

Yes it is in four canaries. it first appeared in 38.0.2111.0 (current is 38.0.2115.0)

3) Has anyone verified that these changes resolve the issue and cause no new
   crashes (via chromecrash/) or regressions?

chromecrash verifies that the first release with this crash was 36.0.1985.125, and the last one is 38.0.2104.0.

4) Why is this necessary for this milestone?

This is a potential security issue as it involves the use of unallocated memory.
Labels: M-37
Adding milestone based on c#6.
Cc: infe...@chromium.org
cc'ing inferno@ to evaluate from a security perspective
Labels: -Type-Bug -Pri-2 Type-Bug-Security Pri-1 Security_Severity-High M-38 Security_Impact-Stable Restrict-View-SecurityTeam
Yes this is use-after-free and needs to be merged.
Comment 12 by aarya@google.com, Sep 3 2014
Labels: -M-37
gettting conflicts with m37 merge, punting to m38.
Project Member Comment 13 by ClusterFuzz, Sep 3 2014
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -Merge-Requested
Which cl is this request for? 285428 should have made the branch cut since we branched @ 290040.  Feel free to re-request if more work is needed.
Labels: Release-0-M38
Project Member Comment 16 by ClusterFuzz, Oct 31 2014
Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member Comment 17 by sheriffbot@chromium.org, Oct 1 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 18 by sheriffbot@chromium.org, Oct 2 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Sign in to add a comment