New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 0 users

Issue metadata

Status: Fixed
Closed: Dec 2012
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

  • Only users with EditIssue permission may comment.

Sign in to add a comment

Issue 137852: Heap-use-after-free in WebKit::WebElement::document

Reported by, Jul 18 2012 Project Member

Issue description

Detailed report:

Fuzzer: Fermin_swf_bitflip

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x7fc3246d58a0
Crash State:
  - crash stack -
  - free stack -

Minimized Testcase (238.71 Kb):

Comment 1 by, Jul 18 2012

Status: Assigned
This does not look flash code, but our ppapi handling code.

Comment 2 by, Jul 18 2012

Owner: ----

Comment 3 by, Jul 22 2012

Labels: Feature-Flash Feature-Plugins-Pepper SecImpacts-Beta

Comment 4 by, Jul 23 2012


Comment 5 by, Jul 26 2012

Looks like the m_pluginWidget member of FrameLoaderClientImpl is keeping the widget alive past the destruction of the document.  This keeps the instance registered so that a delayed GetWindowObject is still processed, rather than just erroring upon enter.  The plugininstance has a raw ptr to a webplugincontentsimpl which has a raw ptr to an HTMLPluginElement from the document which no longer exists.

Now, the m_pluginWidget is just about to be cleared by FrameLoaderClientImpl::redirectDataToPlugin (called from PluginDocumentParser::appendBytes), but before that can happen, layout causes the plugin to be created, which processes a sync message, which can catch the delayed GetWindowObject in its nested message loop.

Comment 6 by, Jul 26 2012

More background:  A Debug build will likely trip over an assert showing that the m_pluginWidget outlived it page when it gets cleared (frame 28):

(gdb) where
#0  0x00007fbb68fc34a5 in WebCore::disposeUnderlyingV8Object (
    at third_party/WebKit/Source/WebCore/bindings/v8/NPV8Object.cpp:178
#1  0x00007fbb68fc2d6d in WebCore::freeV8NPObject (npObject=0x7fbb57192f60)
    at third_party/WebKit/Source/WebCore/bindings/v8/NPV8Object.cpp:78
#2  0x00007fbb69037aa3 in _NPN_DeallocateObject (npObject=0x7fbb57192f60)
    at third_party/WebKit/Source/WebCore/bindings/v8/npruntime.cpp:310
#3  0x00007fbb69037b86 in _NPN_ReleaseObject (npObject=0x7fbb57192f60)
    at third_party/WebKit/Source/WebCore/bindings/v8/npruntime.cpp:323
#4  0x00007fbb68810616 in WebKit::WebBindings::releaseObject (
    at third_party/WebKit/Source/WebKit/chromium/src/WebBindings.cpp:148
#5  0x00007fbb69e1c42b in ppapi::NPObjectVar::~NPObjectVar (
    this=0x7fbb5713d6e0, __in_chrg=<optimized out>)
    at webkit/plugins/ppapi/
#6  0x00007fbb676f2414 in base::RefCounted<ppapi::Var>::Release (
    this=0x7fbb5713d6e8) at ./base/memory/ref_counted.h:95
#7  0x00007fbb687f6589 in scoped_refptr<ppapi::Var>::~scoped_refptr (
    this=0x7fbb57192fa0, __in_chrg=<optimized out>)
    at ./base/memory/ref_counted.h:252
#8  0x00007fbb687f81b8 in ppapi::VarTracker::VarInfo::~VarInfo (
    this=0x7fbb57192fa0, __in_chrg=<optimized out>)
    at ./ppapi/shared_impl/var_tracker.h:92
#9  0x00007fbb687f8236 in std::pair<int const, ppapi::VarTracker::VarInfo>::~pair (this=0x7fbb57192f98, __in_chrg=<optimized out>)
    at /usr/include/c++/4.4/bits/stl_pair.h:68
#10 0x00007fbb687fa0b2 in __gnu_cxx::new_allocator<std::pair<int const, ppapi::VarTracker::VarInfo> >::destroy (this=0x7fffc027eccf, __p=0x7fbb57192f98)
    at /usr/include/c++/4.4/ext/new_allocator.h:115
#11 0x00007fbb687f9715 in __gnu_cxx::hashtable<std::pair<int const, ppapi::VarTracker::VarInfo>, int, __gnu_cxx::hash<int>, std::_Select1st<std::pair<int const, ppapi::VarTracker::VarInfo> >, std::equal_to<int>, std::allocator<ppapi::VarTracker::VarInfo> >::_M_delete_node (this=0x7fbb65648600, __n=0x7fbb57192f90)
    at /usr/include/c++/4.4/backward/hashtable.h:616
#12 0x00007fbb687f8e1f in __gnu_cxx::hashtable<std::pair<int const, ppapi::VarTracker::VarInfo>, int, __gnu_cxx::hash<int>, std::_Select1st<std::pair<int const, ppapi::VarTracker::VarInfo> >, std::equal_to<int>, std::allocator<ppapi::VarTracker::VarInfo> >::erase (this=0x7fbb65648600, __it=...)
    at /usr/include/c++/4.4/backward/hashtable.h:913
#13 0x00007fbb687f84cb in __gnu_cxx::hash_map<int, ppapi::VarTracker::VarInfo, __gnu_cxx::hash<int>, std::equal_to<int>, std::allocator<ppapi::VarTracker::VarInfo> >::erase (this=0x7fbb65648600, __it=...)
    at /usr/include/c++/4.4/ext/hash_map:237
#14 0x00007fbb687f8169 in ppapi::VarTracker::DeleteObjectInfoIfNecessary (
    this=0x7fbb656485c0, iter=...) at ppapi/shared_impl/
#15 0x00007fbb69e1966b in webkit::ppapi::HostVarTracker::ForceReleaseNPObject (
    this=0x7fbb656485c0, object=...)
    at webkit/plugins/ppapi/
#16 0x00007fbb69e19328 in webkit::ppapi::HostVarTracker::DidDeleteInstance (
    this=0x7fbb656485c0, instance=1673976585)
    at webkit/plugins/ppapi/
#17 0x00007fbb69e14d09 in webkit::ppapi::HostGlobals::InstanceDeleted (
    this=0x7fbb65648540, instance=1673976585)
    at webkit/plugins/ppapi/
#18 0x00007fbb69e24fa6 in webkit::ppapi::PluginInstance::~PluginInstance (
    this=0x7fbb656db900, __in_chrg=<optimized out>)
    at webkit/plugins/ppapi/
#19 0x00007fbb69e2f7e6 in base::RefCounted<webkit::ppapi::PluginInstance>::Release (this=0x7fbb656db908) at ./base/memory/ref_counted.h:95
#20 0x00007fbb6b1bacd0 in scoped_refptr<webkit::ppapi::PluginInstance>::operator= (this=0x7fbb5717c258, p=0x0) at ./base/memory/ref_counted.h:280
#21 0x00007fbb6b1b9ee0 in webkit::ppapi::WebPluginImpl::destroy (
    this=0x7fbb5717c240) at webkit/plugins/ppapi/
#22 0x00007fbb6883f411 in WebKit::WebPluginContainerImpl::~WebPluginContainerImpl (this=0x7fbb57163680, __in_chrg=<optimized out>)
    at third_party/WebKit/Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:629
#23 0x00007fbb68841cc6 in WTF::RefCounted<WebCore::Widget>::deref (
    this=0x7fbb57163688) at third_party/WebKit/Source/WTF/wtf/RefCounted.h:190
#24 0x00007fbb68841563 in WTF::derefIfNotNull<WebKit::WebPluginContainerImpl> (
    ptr=0x7fbb57163680) at third_party/WebKit/Source/WTF/wtf/PassRefPtr.h:52
#25 0x00007fbb688960bc in WTF::RefPtr<WebKit::WebPluginContainerImpl>::operator= (this=0x7fbb576f1118, optr=0x7fbb57163820)
    at third_party/WebKit/Source/WTF/wtf/RefPtr.h:126
#26 0x00007fbb68894ab9 in WebKit::FrameLoaderClientImpl::redirectDataToPlugin (
    this=0x7fbb576f1028, pluginWidget=0x7fbb57163820)
    at third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1495
#27 0x00007fbb68ca8a4a in WebCore::PluginDocumentParser::appendBytes (
    at third_party/WebKit/Source/WebCore/html/PluginDocument.cpp:129
---Type <return> to continue, or q <return> to quit---y
#28 0x00007fbb6928507c in WebCore::DocumentWriter::addData (
    this=0x7fbb577650c0, bytes=0x0, length=0)
    at third_party/WebKit/Source/WebCore/loader/DocumentWriter.cpp:218
#29 0x00007fbb69275009 in WebCore::DocumentLoader::commitData (
    this=0x7fbb57765000, bytes=0x0, length=0)
    at third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:355
#30 0x00007fbb69274c04 in WebCore::DocumentLoader::finishedLoading (
    at third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:296
#31 0x00007fbb692af1b5 in WebCore::MainResourceLoader::didFinishLoading (
    this=0x7fbb57117400, finishTime=3734932.239753)
    at third_party/WebKit/Source/WebCore/loader/MainResourceLoader.cpp:516
#32 0x00007fbb692c2ca5 in WebCore::ResourceLoader::didFinishLoading (
    this=0x7fbb57117400, finishTime=3734932.239753)
    at third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:436
#33 0x00007fbb6b0cd2fe in WebCore::ResourceHandleInternal::didFinishLoading (
    this=0x7fbb57171700, finishTime=3734932.239753)
    at third_party/WebKit/Source/WebCore/platform/network/chromium/ResourceHandle.cpp:156

Comment 7 by, Jul 27 2012

So here's a change that solves the problem. Seems similar to so adding author/reviewer of that patch to ask the question:  Is this an appropriate place to clear the m_pluginWidget?  Or is there some method that didn't happen in the PPAPI case that should have taken care of this?  Let me know if this is OK and I'll file a bug upstream to land it.

Index: third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp
--- third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp	(revision 123007)
+++ third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp	(working copy)
@@ -133,12 +133,16 @@
 void FrameLoaderClientImpl::dispatchDidClearWindowObjectInWorld(DOMWrapperWorld*)
+    // Don't let the plugin widget outlive the rest of the page.
+    m_pluginWidget = 0;
     if (m_webFrame->client())
 void FrameLoaderClientImpl::documentElementAvailable()
+    // Verify that the plugin widget didn't outlive the rest of the page.
+    ASSERT(!m_pluginWidget);
     if (m_webFrame->client())

Comment 8 by, Aug 2 2012

Labels: Security-CodeYellow
Please do read Mark's email titled "Code Yellow: Security Bug Backlog" on chrome-team mailing list.

Comment 9 by, Aug 2 2012

Adding abarth@, who knows things about the document-loader code.

We've had niggling issues with plugin widget lifetimes seemingly forever; various parts of WebKit assume that they won't outlive the page but at some point that guarantee has been broken - it's possible that this change could fix a swathe of such issues, if the FrameLoaderClientImpl's reference to the widget is the root cause of it living too long.

Comment 10 by, Aug 2 2012

I don't know anything about plugins.

Comment 11 by, Aug 3 2012

Argh.  By the time dispatchDidClearWindowObjectInWorld() happens, its already too late.

Comment 12 by, Aug 3 2012

Ah. This seems like it would do the trick.  FrameLoader::clear() is about to clear out the document in question, so this would be a good time to drop the ref.

Index: WebKit/chromium/src/FrameLoaderClientImpl.cpp
--- WebKit/chromium/src/FrameLoaderClientImpl.cpp	(revision 124582)
+++ WebKit/chromium/src/FrameLoaderClientImpl.cpp	(working copy)
@@ -1491,9 +1491,8 @@
 // (e.g., acrobat reader).
 void FrameLoaderClientImpl::redirectDataToPlugin(Widget* pluginWidget)
-    if (pluginWidget->isPluginContainer())
-        m_pluginWidget = static_cast<WebPluginContainerImpl*>(pluginWidget);
-    ASSERT(m_pluginWidget);
+    ASSERT(!pluginWidget || pluginWidget->isPluginContainer());
+    m_pluginWidget = static_cast<WebPluginContainerImpl*>(pluginWidget);
 PassRefPtr<Widget> FrameLoaderClientImpl::createJavaAppletWidget(
Index: WebCore/loader/FrameLoader.cpp
--- WebCore/loader/FrameLoader.cpp	(revision 124582)
+++ WebCore/loader/FrameLoader.cpp	(working copy)
@@ -510,7 +510,9 @@
     if (!m_needsClear)
     m_needsClear = false;
+    m_client->redirectDataToPlugin(0);
     if (!m_frame->document()->inPageCache()) {

Seems to run cleanly with this change.  Question about whether this should be a 
separate method in all the FLCs or if all the FLCs should change to expect a NULL argument.

Comment 13 by, Aug 3 2012

Calling this out of PluginDocument::detach() seems cleaner.

Comment 14 by, Aug 6 2012

upstremed as  We're going to have to handle this on the webkit side.

Comment 15 by, Aug 9 2012

Labels: Mstone-22

Comment 16 by, Aug 9 2012

Tom, does this affect Stable ? c#3 is confusing, is this a pepper bug ?

Comment 17 by, Aug 9 2012

I suspect this could affect other plugins, so a merge seems reasonable.

Comment 18 by, Aug 9 2012

Labels: -Mstone-22 Mstone-21 SecImpacts-Stable
Thanks Tom.

Comment 19 by, Aug 14 2012

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved
Status: FixUnreleased

Comment 20 by, Sep 5 2012

Labels: -Mstone-21 -Merge-Approved Mstone-22 Merge-Merged

Comment 21 by, Oct 14 2012

Project Member
Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.

Comment 22 by, Dec 20 2012

Status: Fixed

Comment 23 by, Jan 18 2013

Project Member
Labels: Restrict-View-EditIssue
Restrict-View-EditIssue is preferred since it allows anyone who can edit an issue (committers and contributors) to view the bug.

Comment 24 by, Mar 10 2013

Project Member
Labels: -Area-WebKit -Type-Security -SecSeverity-High -Stability-AddressSanitizer -Feature-Flash -Feature-Plugins-Pepper -SecImpacts-Beta -Mstone-22 -SecImpacts-Stable Cr-Content Security-Impact-Beta Type-Bug-Security Cr-Content-Plugins-Flash M-22 Security-Severity-High Security-Impact-Stable Cr-Content-Plugins-Pepper Performance-Memory-AddressSanitizer

Comment 25 by, Mar 14 2013

Project Member
Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue

Comment 26 by, Mar 21 2013

Labels: -Restrict-View-SecurityNotify -Restrict-View-EditIssue

Comment 27 by, Mar 21 2013

Project Member
Labels: -Security-Severity-High Security_Severity-High

Comment 28 by, Mar 21 2013

Project Member
Labels: -Security-Impact-Stable Security_Impact-Stable

Comment 29 by, Mar 21 2013

Project Member
Labels: -Security-Impact-Beta Security_Impact-Beta

Comment 30 by, Apr 1 2013

Project Member
Labels: -Performance-Memory-AddressSanitizer Stability-Memory-AddressSanitizer

Comment 31 by, Apr 6 2013

Project Member
Labels: -Cr-Content Cr-Blink

Comment 32 by, Apr 6 2013

Project Member
Labels: -Cr-Content-Plugins-Flash Cr-Internals-Plugins-Flash

Comment 33 by, Apr 6 2013

Project Member
Labels: Cr-Internals-Plugins

Comment 34 by, Apr 6 2013

Project Member
Labels: -Cr-Content-Plugins-Pepper Cr-Internals-Plugins-Pepper

Comment 35 by, Jun 14 2016

Project Member
Labels: -security_impact-beta

Comment 36 by, Oct 1 2016

Project Member
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot

Comment 37 by, Oct 2 2016

Project Member
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot

Comment 38 by, Oct 2 2016

Labels: allpublic

Sign in to add a comment