Bug 54877 - chrome.dll!WebCore::HTMLAreaElement::setFocus ReadAV@NULL (2f8e4496f8ea69da13971f5fddd09753)
Summary: chrome.dll!WebCore::HTMLAreaElement::setFocus ReadAV@NULL (2f8e4496f8ea69da13...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P1 Normal
Assignee: Berend-Jan Wever
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-21 06:06 PST by Berend-Jan Wever
Modified: 2011-03-10 17:33 PST (History)
4 users (show)

See Also:


Attachments
Repro (95 bytes, text/html)
2011-02-21 06:06 PST, Berend-Jan Wever
no flags Details
Layout test (850 bytes, text/html)
2011-02-21 10:36 PST, Berend-Jan Wever
no flags Details
Patch (4.42 KB, patch)
2011-03-01 06:18 PST, Berend-Jan Wever
eric: review-
Details | Formatted Diff | Diff
Patch attempt #2 (3.39 KB, patch)
2011-03-02 00:33 PST, Berend-Jan Wever
darin: review-
Details | Formatted Diff | Diff
Patch (3.69 KB, patch)
2011-03-04 09:52 PST, Berend-Jan Wever
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 2011-02-21 06:06:22 PST
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>
Comment 1 Eric Seidel (no email) 2011-02-21 09:47:32 PST
Sounds like a trivial fix.
Comment 2 Berend-Jan Wever 2011-02-21 10:36:15 PST
Created attachment 83182 [details]
Layout test
Comment 3 Berend-Jan Wever 2011-02-21 10:38:45 PST
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();
+    // skylined@chromium.org, 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
Comment 4 Berend-Jan Wever 2011-03-01 05:20:19 PST
I'm working on a patch
Comment 5 Berend-Jan Wever 2011-03-01 06:18:38 PST
Created attachment 84223 [details]
Patch
Comment 6 Eric Seidel (no email) 2011-03-01 09:53:58 PST
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
> +2011-03-01  BJ Wever  <skylined@chromium.org>
> +

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. :)
Comment 7 Eric Seidel (no email) 2011-03-01 09:57:11 PST
I guess in my "whole test" example, I forgot the dumpAsText(),  but i think you get the picture. :)
Comment 8 Berend-Jan Wever 2011-03-02 00:33:21 PST
Created attachment 84380 [details]
Patch attempt #2
Comment 9 Darin Adler 2011-03-02 10:23:43 PST
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.
Comment 10 Berend-Jan Wever 2011-03-04 09:02:33 PST
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))
Comment 11 Berend-Jan Wever 2011-03-04 09:52:52 PST
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.
Comment 12 WebKit Commit Bot 2011-03-10 15:49:31 PST
Comment on attachment 84765 [details]
Patch

Clearing flags on attachment: 84765

Committed r80779: <http://trac.webkit.org/changeset/80779>
Comment 13 WebKit Commit Bot 2011-03-10 15:49:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 WebKit Review Bot 2011-03-10 17:33:27 PST
http://trac.webkit.org/changeset/80779 might have broken GTK Linux 32-bit Release