Bug 73582

Summary: InspectorController destruction order leads to use-after-free
Product: WebKit Reporter: Adam Klein <adamk>
Component: Web Inspector (Deprecated)Assignee: Pavel Feldman <pfeldman>
Status: RESOLVED FIXED    
Severity: Normal CC: ayao, benjamin, timothy, yurys
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
[Patch] with the speculative fix.
none
[Patch] with the speculative fix for ews (2). yurys: review+

Description Adam Klein 2011-12-01 12:31:44 PST 
After http://trac.webkit.org/changeset/101345, I'm seeing consistent Valgrind errors on the bots:

http://build.chromium.org/p/chromium.webkit/builders/Linux%20Valgrind/builds/17144/steps/memory%20test%3A%20test_shell/logs/stdio

Here's the top of the stack trace:

InvalidWrite
Invalid write of size 4
  WebCore::InspectorDOMAgent::setDOMListener(WebCore::InspectorDOMAgent::DOMListener*) (third_party/WebKit/Source/WebCore/inspector/InspectorDOMAgent.cpp:254)
  WebCore::InspectorCSSAgent::~InspectorCSSAgent() (third_party/WebKit/Source/WebCore/inspector/InspectorCSSAgent.cpp:203)
  void WTF::deleteOwnedPtr<WebCore::InspectorBaseAgentInterface>(WebCore::InspectorBaseAgentInterface*) (third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtrCommon.h:53)
  WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>::~OwnPtr() (third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtr.h:54)
  WTF::VectorDestructor<true, WTF::OwnPtr<WebCore::InspectorBaseAgentInterface> >::destruct(WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>*, WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>*) (third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h:58)
  WTF::VectorTypeOperations<WTF::OwnPtr<WebCore::InspectorBaseAgentInterface> >::destruct(WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>*, WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>*) (third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h:217)
  WTF::Vector<WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>, 0u>::shrink(unsigned int) (third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h:840)
  WTF::Vector<WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>, 0u>::~Vector() (third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h:498)
  WebCore::InspectorController::~InspectorController() (third_party/WebKit/Source/WebCore/inspector/InspectorController.cpp:155)
...

Address 0xb112d20 is 32 bytes inside a block of size 172 free'd
  operator delete(void*) (m_replacemalloc/vg_replace_malloc.c:1083)
  WebCore::InspectorDOMAgent::~InspectorDOMAgent() (third_party/WebKit/Source/WebCore/inspector/InspectorDOMAgent.cpp:191)
  void WTF::deleteOwnedPtr<WebCore::InspectorBaseAgentInterface>(WebCore::InspectorBaseAgentInterface*) (third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtrCommon.h:53)

  WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>::~OwnPtr() (third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtr.h:54)
  WTF::VectorDestructor<true, WTF::OwnPtr<WebCore::InspectorBaseAgentInterface> >::destruct(WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>*, WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>*) (third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h:58)
  WTF::VectorTypeOperations<WTF::OwnPtr<WebCore::InspectorBaseAgentInterface> >::destruct(WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>*, WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>*) (third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h:217)
  WTF::Vector<WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>, 0u>::shrink(unsigned int) (third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h:840)
  WTF::Vector<WTF::OwnPtr<WebCore::InspectorBaseAgentInterface>, 0u>::~Vector() (third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h:498)
  WebCore::InspectorController::~InspectorController() (third_party/WebKit/Source/WebCore/inspector/InspectorController.cpp:155)
...

The issue is that the InspectorDOMAgent passed to the InspectorCSSAgent is at an earlier index in the Vector<OwnPtr> than the CSSAgent, so it's deleted by the time ~InspectorCSSAgent tries to call setDOMListener(0).

The same error exists for InspectorDebuggerAgent (it's also passed a handle to the InspectorDOMAgent, and tries to call setListener(0)).

Easiest fix would be to change the order of the vector, putting the InspectorDOMAgent at the end; seems a bit fragile, though.
Comment 1 Pavel Feldman 2011-12-01 12:33:49 PST 
> Easiest fix would be to change the order of the vector, putting the InspectorDOMAgent at the end; seems a bit fragile, though.

Thanks for the heads up, I'll handle it.
Comment 2 Timothy Hatcher 2011-12-01 12:51:38 PST 
<rdar://problem/10508230>
Comment 3 Pavel Feldman 2011-12-01 13:00:53 PST 
Created attachment 117466 [details]
[Patch] with the speculative fix.
Comment 4 Early Warning System Bot 2011-12-01 13:04:02 PST 
Comment on attachment 117466 [details]
[Patch] with the speculative fix.

Attachment 117466 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10703242
Comment 5 Pavel Feldman 2011-12-01 13:07:56 PST 
Created attachment 117469 [details]
[Patch] with the speculative fix for ews (2).
Comment 6 Pavel Feldman 2011-12-02 00:22:15 PST 
Committed r101754: <http://trac.webkit.org/changeset/101754>