Use-after-free in WebCore::Range::compareBoundaryPoints

Fetch parentNode after toNormalizedRange() call has happened in containsNode()

toNormalizedRange() call can trigger layout, there removing the last
reference to parentNode. We don't need to RefPtr parentNode since later
callers don't modify it.

BUG=242114
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150685

[ojan, 2013-05-20 02:30:48]

https://codereview.chromium.org/15390003/diff/1/Source/core/page/DOMSelection...
File Source/core/page/DOMSelection.cpp (right):

https://codereview.chromium.org/15390003/diff/1/Source/core/page/DOMSelection...
Source/core/page/DOMSelection.cpp:453: // reference to node |n|,
The render tree doesn't hold refptrs to Nodes AFAIK, how does a layout remove
the last reference?

Looking at the test case in this patch, it seems like the problem is that
javascript isn't keeping the Node alive. If it's really layout that is removing
the last reference to the node, you should be able to get a use-after-free by
doing something like the following after the document.close() call:
document.documentElement.offsetHeight;
element.parentNode;

[inferno, 2013-05-20 02:37:55]

On 2013/05/20 02:30:48, ojan wrote:
>
https://codereview.chromium.org/15390003/diff/1/Source/core/page/DOMSelection...
> File Source/core/page/DOMSelection.cpp (right):
> 
>
https://codereview.chromium.org/15390003/diff/1/Source/core/page/DOMSelection...
> Source/core/page/DOMSelection.cpp:453: // reference to node |n|,
> The render tree doesn't hold refptrs to Nodes AFAIK, how does a layout remove
> the last reference?
> 
> Looking at the test case in this patch, it seems like the problem is that
> javascript isn't keeping the Node alive. If it's really layout that is
removing
> the last reference to the node, you should be able to get a use-after-free by
> doing something like the following after the document.close() call:
> document.documentElement.offsetHeight;
> element.parentNode;

freed by thread T0 (chrome) here:
    #0 0x7f941650ea02 in operator delete(void*)
    #1 0x7f93faee316e in WebCore::HTMLHtmlElement::~HTMLHtmlElement()
src/out/Debug/../../third_party/WebKit/Source/core/html/HTMLHtmlElement.h:31
    #2 0x7f93fd5dc82b in WebCore::Node::removedLastRef()
src/out/Debug/../../third_party/WebKit/Source/core/dom/Node.cpp:2629
    #3 0x7f93efa9cc57 in WebCore::TreeShared<WebCore::Node>::deref()
src/out/Debug/../../third_party/WebKit/Source/core/platform/TreeShared.h:79
    #4 0x7f93efaa0547 in void WTF::derefIfNotNull<WebCore::Node>(WebCore::Node*)
src/out/Debug/../../third_party/WebKit/Source/wtf/PassRefPtr.h:44
    #5 0x7f93efd438b7 in WTF::RefPtr<WebCore::Node>::operator=(WebCore::Node*)
src/out/Debug/../../third_party/WebKit/Source/wtf/RefPtr.h:126
    #6 0x7f94000a4e2b in WebCore::FrameSelection::recomputeCaretRect()
src/out/Debug/../../third_party/WebKit/Source/core/editing/FrameSelection.cpp:1391
    #7 0x7f940008b2ec in WebCore::FrameSelection::updateAppearance()
src/out/Debug/../../third_party/WebKit/Source/core/editing/FrameSelection.cpp:1759
    #8 0x7f94012d3138 in WebCore::FrameView::performPostLayoutTasks()
src/out/Debug/../../third_party/WebKit/Source/core/page/FrameView.cpp:2236
    #9 0x7f94012d2125 in WebCore::FrameView::layout(bool)
src/out/Debug/../../third_party/WebKit/Source/core/page/FrameView.cpp:1102
    #10 0x7f93fd048337 in WebCore::Document::updateLayout()
src/out/Debug/../../third_party/WebKit/Source/core/dom/Document.cpp:1712
    #11 0x7f94001e4870 in WebCore::VisibleSelection::toNormalizedRange() const
src/out/Debug/../../third_party/WebKit/Source/core/editing/VisibleSelection.cpp:145

m_previousCaretNode keeps the last reference, setting it during layout blows it.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[inferno, 2013-05-20 02:43:26]

In addition to last comment, here is the reason why these lines won't trigger
the use-after-free.

document.documentElement.offsetHeight;
element.parentNode;

offsetHeight would trigger layout, so element is null after the reference is
gone. an alert on element and element.parentNode is null. In this case, the bug
was toNormalizedRange() triggered layout, so node is freed, but the function
containsNode continues to use weak reference on Node and parentNode.

[inferno, 2013-05-20 16:41:03]

I debugged it wrong before. Node |n| was const, and it is not the one getting
blown away. It is the weak reference to parentNode that it fetches before the
toNormalizedRange() call. toNormalizedRange() triggers layout, thereby resetting
m_previousCaretNode. resetting m_previousCaretNode blows the last reference to
parentNode and hence a use-after-free occurs.

[adamk, 2013-05-20 17:41:02]

lgtm

[commit-bot: I haz the power, 2013-05-20 17:41:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/inferno@chromium.org/15390003/9001

[commit-bot: I haz the power, 2013-05-20 18:43:24]

Change committed as 150685