Bug 54877 – chrome.dll!WebCore::HTMLAreaElement::setFocus ReadAV@NULL (2f8e4496f8ea69da13971f5fddd09753)

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Berend-Jan Wever]

Created attachment 83154 [details]
Repro

Chromium: http://code.google.com/p/chromium/issues/detail?id=73650

"area" elements can be focussed if they are a link. They can be made a link by setting a "href" attribute to a non-null value. If an area element is focussed, the code checks if its parent element is a "map" element, without verifying if it has a parent element. By creating an area element, setting the href attribute and calling its focus method, we can cause the code to deref a NULL pointer, crashing the renderer.

http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/HTMLAreaElement.h&q=HTMLAreaElement&exact_package=chromium&sa=N&cd=1&ct=rc&l=36
class HTMLAreaElement : public HTMLAnchorElement {
<snip>

http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/HTMLAnchorElement.cpp&q=HTMLAnchorElement::supportsFocus&exact_package=chromium&sa=N&cd=1&ct=rc&l=73
bool HTMLAnchorElement::supportsFocus() const
{
<snip>
    return isLink() || HTMLElement::supportsFocus();

http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/HTMLAreaElement.cpp&q=HTMLAreaElement::setFocus&exact_package=chromium&sa=N&cd=1&ct=rc&l=199
void HTMLAreaElement::setFocus(bool shouldBeFocused)
{
<snip>
    HTMLImageElement* imageElement = this->imageElement();

http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/HTMLAreaElement.cpp&q=HTMLAreaElement::imageElement&exact_package=chromium&sa=N&cd=1&ct=rc&l=175

HTMLImageElement* HTMLAreaElement::imageElement() const
{
    Node* mapElement = parentNode();                          /// Might return NULL
    if (!mapElement->hasTagName(mapTag))                      /// Might deref NULL => KaB00m
<snip>

id:             chrome.dll!WebCore::HTMLAreaElement::setFocus ReadAV@NULL (2f8e4496f8ea69da13971f5fddd09753)
description:    Attempt to read from unallocated NULL pointer+0x24 in chrome.dll!WebCore::HTMLAreaElement::setFocus
application:    Chromium 11.0.679.0
stack:          chrome.dll!WebCore::HTMLAreaElement::setFocus
                chrome.dll!WebCore::Document::setFocusedNode
                chrome.dll!WebCore::FocusController::setFocusedNode
                chrome.dll!WebCore::Element::focus
                chrome.dll!WebCore::ElementInternal::focusCallback
                chrome.dll!v8::internal::HandleApiCallHelper<...>
                chrome.dll!v8::internal::Builtin_HandleApiCall
                chrome.dll!v8::internal::Invoke
                chrome.dll!v8::internal::Execution::Call
                ...

Repro:
<script>
  oArea = document.createElement('area');
  oArea.href = 0;
  oArea.focus();
</script>

[Eric Seidel (no email)]

Sounds like a trivial fix.

[Berend-Jan Wever]

Created attachment 83182 [details]
Layout test

[Berend-Jan Wever]

I'm still trying to figure out hwo to submit a patch properly, but here's my suggestion:

Index: HTMLAreaElement.cpp
===================================================================
--- HTMLAreaElement.cpp (revision 79111)
+++ HTMLAreaElement.cpp (working copy)
@@ -231,7 +231,9 @@
     // If the AREA element was a link, it should support focus.
     // The inherited method is not used because it assumes that a render object must exist
     // for the element to support focus. AREA elements do not have render objects.
-    return isLink();
+    // [e-mail], Chromium issue 73650, WebKit issue 54877:
+    // An element that is not in the DOM tree cannot be focussed.
+    return isLink() && parentNode();
 }

 String HTMLAreaElement::target() const

[Berend-Jan Wever]

I'm working on a patch

[Berend-Jan Wever]

Created attachment 84223 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 84223 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84223&action=review

Otherwise seems fine to me.

> LayoutTests/ChangeLog:16
> [phone]  BJ Wever  <[e-mail]>
> +

Double ChangeLog.

> LayoutTests/fast/dom/HTMLAreaElement/area-islink-focus-null-ptr-crash.html:31
> +  <body onload="go()">

This doens't need to be in on onload, and doesn't need to be in the head.  You don't even need the rest of the document.  I woul djust have a <script> tag with your 3 lines of JS.   Something like this:

<div id="log">FAIL</div>
<script>
var area = document.createElement("area");
area.href = 0;
area.focus();
log.innerHTML = "PASS, did not crash";
</script>

And that should be your whole test. :)

[Eric Seidel (no email)]

I guess in my "whole test" example, I forgot the dumpAsText(),  but i think you get the picture. :)

[Berend-Jan Wever]

Created attachment 84380 [details]
Patch attempt #2

[Darin Adler]

Comment on attachment 84380 [details]
Patch attempt #2

View in context: https://bugs.webkit.org/attachment.cgi?id=84380&action=review

Thanks for tackling this.

> Source/WebCore/html/HTMLAreaElement.cpp:234
> -    // If the AREA element was a link, it should support focus.
> +    // If the AREA element was a link and has a parent, it should support focus.
>      // The inherited method is not used because it assumes that a render object must exist 
>      // for the element to support focus. AREA elements do not have render objects.
> -    return isLink();
> +    return isLink() && parentNode();

This check seems incorrect, too specific, and in the wrong place. For example, an area element might have a parent, but its parent might in turn not be in the document’s DOM tree.

I think the correct check would be not in the supportsFocus function, but rather at the call sites. And the check would probably be inDocument, or possibly a walk up the parent node chain until we reach the document that's trying to do the focus or don’t.

I’m pretty sure that the function that needs the fix is Element::focus. It doesn’t have the inDocument check that Node::isFocusable has, and the fix could be as simple as adding that.

[Berend-Jan Wever]

Thanks, that makes sense Darin.

In addition, there seems to be another problem: HTMLAreaElement::imageElement assumes the area element has a parent, which is not true in this case, and may not be true in others. I think we should address this as a defense in depth as well:

HTMLImageElement* HTMLAreaElement::imageElement() const
{
    Node* mapElement = parentNode();
    if (!mapElement->hasTagName(mapTag))
        return 0;
    
    return static_cast<HTMLMapElement*>(mapElement)->imageElement();
}
Patch:
    if (!mapElement || !mapElement->hasTagName(mapTag))

[Berend-Jan Wever]

Created attachment 84765 [details]
Patch

The changes to HTMLAreaElement::imageElement are not strictly needed, so let me know if it makes sense to add them as a defense in depth or not.

[WebKit Commit Bot]

Comment on attachment 84765 [details]
Patch

Clearing flags on attachment: 84765

Committed r80779: <http://trac.webkit.org/changeset/80779>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/80779 might have broken GTK Linux 32-bit Release