Bug 73582 – InspectorController destruction order leads to use-after-free

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Adam Klein]

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.

[Pavel Feldman]

> 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.

[Timothy Hatcher]

<rdar://problem/10508230>

[Pavel Feldman]

Created attachment 117466 [details]
[Patch] with the speculative fix.

[Early Warning System Bot]

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

[Pavel Feldman]

Created attachment 117469 [details]
[Patch] with the speculative fix for ews (2).

[Pavel Feldman]

Committed r101754: <http://trac.webkit.org/changeset/101754>