Bug 73582 - InspectorController destruction order leads to use-after-free
Summary: InspectorController destruction order leads to use-after-free
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-12-01 12:31 PST by Adam Klein
Modified: 2011-12-12 15:19 PST (History)
4 users (show)

See Also:


Attachments
[Patch] with the speculative fix. (5.58 KB, patch)
2011-12-01 13:00 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
[Patch] with the speculative fix for ews (2). (5.59 KB, patch)
2011-12-01 13:07 PST, Pavel Feldman
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>