Null-deref when first access to an Attr node is after its Element is destroyed
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 25 Sep 2009 21:08:17 +0000 (21:08 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 25 Sep 2009 21:08:17 +0000 (21:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=29748

Patch by Darin Adler <darin@apple.com> on 2009-09-25
Reviewed by Geoffrey Garen.

WebCore:

Test: fast/dom/Attr/access-after-element-destruction.html

* bindings/js/JSAttrCustom.cpp:
(WebCore::JSAttr::markChildren): Added. Keeps the ownerElement alive as
long as the Attr is alive.

* bindings/js/JSNamedNodeMapCustom.cpp:
(WebCore::JSNamedNodeMap::markChildren): Added. Keeps the Element alive as
long as the NamedNodeMap is alive.

* dom/Attr.idl: Added CustomMarkFunction attribute.

* dom/NamedAttrMap.cpp:
(WebCore::NamedNodeMap::getAttributeItem): Tweaked formatting.
(WebCore::NamedNodeMap::detachFromElement): Call clearAttributes so we don't
have attributes hanging around that might need an Attr node created; that way
we won't crash with a null-dereference trying to deal with one of them. This
can't happen when working with JavaScript since the Element will be kept
alive due to the change above.
(WebCore::NamedNodeMap::addAttribute): Fix function name in comment.
(WebCore::NamedNodeMap::removeAttribute): Removed unneeded "+ 1" and added
missing braces.

* dom/NamedAttrMap.h: Made the element function public so it can be used by
the JavaScript binding to keep the Element alive.

* dom/NamedNodeMap.idl: Added CustomMarkFunction attribute.

LayoutTests:

* fast/dom/Attr/access-after-element-destruction-expected.txt: Added.
* fast/dom/Attr/access-after-element-destruction.html: Added.
* fast/dom/Attr/script-tests/TEMPLATE.html: Copied from LayoutTests/fast/dom/Node/script-tests/TEMPLATE.html.
* fast/dom/Attr/script-tests/access-after-element-destruction.js: Added.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@48769 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/dom/Attr/access-after-element-destruction-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/Attr/access-after-element-destruction.html [new file with mode: 0644]
LayoutTests/fast/dom/Attr/script-tests/TEMPLATE.html [new file with mode: 0644]
LayoutTests/fast/dom/Attr/script-tests/access-after-element-destruction.js [new file with mode: 0644]
WebCore/ChangeLog
WebCore/bindings/js/JSAttrCustom.cpp
WebCore/bindings/js/JSNamedNodeMapCustom.cpp
WebCore/dom/Attr.idl
WebCore/dom/NamedAttrMap.cpp
WebCore/dom/NamedAttrMap.h
WebCore/dom/NamedNodeMap.idl

index c7b6448..d259e9a 100644 (file)
@@ -1,3 +1,15 @@
+2009-09-25  Darin Adler  <darin@apple.com>
+
+        Reviewed by Geoffrey Garen.
+
+        Null-deref when first access to an Attr node is after its Element is destroyed
+        https://bugs.webkit.org/show_bug.cgi?id=29748
+
+        * fast/dom/Attr/access-after-element-destruction-expected.txt: Added.
+        * fast/dom/Attr/access-after-element-destruction.html: Added.
+        * fast/dom/Attr/script-tests/TEMPLATE.html: Copied from LayoutTests/fast/dom/Node/script-tests/TEMPLATE.html.
+        * fast/dom/Attr/script-tests/access-after-element-destruction.js: Added.
+
 2009-09-24  Alexey Proskuryakov  <ap@apple.com>
 
         Reviewed by Darin Adler and Sam Weinig.
diff --git a/LayoutTests/fast/dom/Attr/access-after-element-destruction-expected.txt b/LayoutTests/fast/dom/Attr/access-after-element-destruction-expected.txt
new file mode 100644 (file)
index 0000000..8c09132
--- /dev/null
@@ -0,0 +1,25 @@
+Tests that accessing Attr after its Element has been destroyed works without crashing.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS attributes.length is 1
+PASS attributes[0] is attributes.item(0)
+PASS attributes.getNamedItem('a') is attributes.item(0)
+PASS attributes.item(0).name is 'a'
+PASS attributes.item(0).specified is true
+PASS attributes.item(0).value is 'b'
+PASS attributes.item(0).ownerElement.tagName is 'P'
+PASS attributes.item(0).style is null
+PASS attributes.item(0).value is 'c'
+PASS attributes.length is 0
+PASS attr.name is 'a'
+PASS attr.specified is true
+PASS attr.value is 'b'
+PASS attr.ownerElement.tagName is 'P'
+PASS attr.style is null
+PASS attr.value is 'c'
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/Attr/access-after-element-destruction.html b/LayoutTests/fast/dom/Attr/access-after-element-destruction.html
new file mode 100644 (file)
index 0000000..39e014d
--- /dev/null
@@ -0,0 +1,13 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<link rel="stylesheet" href="../../js/resources/js-test-style.css">
+<script src="../../js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+<script src="script-tests/access-after-element-destruction.js"></script>
+<script src="../../js/resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/fast/dom/Attr/script-tests/TEMPLATE.html b/LayoutTests/fast/dom/Attr/script-tests/TEMPLATE.html
new file mode 100644 (file)
index 0000000..1951c43
--- /dev/null
@@ -0,0 +1,13 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<link rel="stylesheet" href="../../js/resources/js-test-style.css">
+<script src="../../js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+<script src="YOUR_JS_FILE_HERE"></script>
+<script src="../../js/resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/fast/dom/Attr/script-tests/access-after-element-destruction.js b/LayoutTests/fast/dom/Attr/script-tests/access-after-element-destruction.js
new file mode 100644 (file)
index 0000000..91588a3
--- /dev/null
@@ -0,0 +1,55 @@
+description("Tests that accessing Attr after its Element has been destroyed works without crashing.");
+
+function gc()
+{
+    if (window.GCController)
+        return GCController.collect();
+
+    // Trigger garbage collection indirectly.
+    for (var i = 0; i < 100000; i++)
+        new String(i);
+}
+
+var element = document.createElement("p");
+element.setAttribute("a", "b");
+var attributes = element.attributes;
+element = null;
+
+gc();
+
+shouldBe("attributes.length", "1");
+shouldBe("attributes[0]", "attributes.item(0)");
+shouldBe("attributes.getNamedItem('a')", "attributes.item(0)");
+
+shouldBe("attributes.item(0).name", "'a'");
+shouldBe("attributes.item(0).specified", "true");
+shouldBe("attributes.item(0).value", "'b'");
+shouldBe("attributes.item(0).ownerElement.tagName", "'P'");
+shouldBe("attributes.item(0).style", "null");
+
+attributes.item(0).value = 'c';
+
+shouldBe("attributes.item(0).value", "'c'");
+
+attributes.removeNamedItem('a');
+
+shouldBe("attributes.length", "0");
+
+element = document.createElement("p");
+element.setAttribute("a", "b");
+var attr = element.attributes.item(0);
+element = null;
+
+gc();
+
+shouldBe("attr.name", "'a'");
+shouldBe("attr.specified", "true");
+shouldBe("attr.value", "'b'");
+shouldBe("attr.ownerElement.tagName", "'P'");
+shouldBe("attr.style", "null");
+
+attr.value = 'c';
+
+shouldBe("attr.value", "'c'");
+
+var successfullyParsed = true;
index e8b7432..7aff711 100644 (file)
@@ -1,3 +1,38 @@
+2009-09-25  Darin Adler  <darin@apple.com>
+
+        Reviewed by Geoffrey Garen.
+
+        Null-deref when first access to an Attr node is after its Element is destroyed
+        https://bugs.webkit.org/show_bug.cgi?id=29748
+
+        Test: fast/dom/Attr/access-after-element-destruction.html
+
+        * bindings/js/JSAttrCustom.cpp:
+        (WebCore::JSAttr::markChildren): Added. Keeps the ownerElement alive as
+        long as the Attr is alive.
+
+        * bindings/js/JSNamedNodeMapCustom.cpp:
+        (WebCore::JSNamedNodeMap::markChildren): Added. Keeps the Element alive as
+        long as the NamedNodeMap is alive.
+
+        * dom/Attr.idl: Added CustomMarkFunction attribute.
+
+        * dom/NamedAttrMap.cpp:
+        (WebCore::NamedNodeMap::getAttributeItem): Tweaked formatting.
+        (WebCore::NamedNodeMap::detachFromElement): Call clearAttributes so we don't
+        have attributes hanging around that might need an Attr node created; that way
+        we won't crash with a null-dereference trying to deal with one of them. This
+        can't happen when working with JavaScript since the Element will be kept
+        alive due to the change above.
+        (WebCore::NamedNodeMap::addAttribute): Fix function name in comment.
+        (WebCore::NamedNodeMap::removeAttribute): Removed unneeded "+ 1" and added
+        missing braces.
+
+        * dom/NamedAttrMap.h: Made the element function public so it can be used by
+        the JavaScript binding to keep the Element alive.
+
+        * dom/NamedNodeMap.idl: Added CustomMarkFunction attribute.
+
 2009-09-24  Alexey Proskuryakov  <ap@apple.com>
 
         Reviewed by Darin Adler and Sam Weinig.
index e217023..14457c4 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2007, 2008 Apple Inc. All rights reserved.
+ * Copyright (C) 2007, 2008, 2009 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -59,4 +59,16 @@ void JSAttr::setValue(ExecState* exec, JSValue value)
     setDOMException(exec, ec);
 }
 
+void JSAttr::markChildren(MarkStack& markStack)
+{
+    Base::markChildren(markStack);
+
+    // Mark the element so that this will work to access the attribute even if the last
+    // other reference goes away.
+    if (Element* element = impl()->ownerElement()) {
+        if (JSNode* wrapper = getCachedDOMNodeWrapper(element->document(), element))
+            markStack.append(wrapper);
+    }
+}
+
 } // namespace WebCore
index 7bd95b4..93aedca 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2007, 2008 Apple Inc. All rights reserved.
+ * Copyright (C) 2007, 2008, 2009 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
 #include "JSNamedNodeMap.h"
 
 #include "JSNode.h"
-#include "NamedNodeMap.h"
-#include "Node.h"
-#include "PlatformString.h"
-#include "JSDOMBinding.h"
 
 using namespace JSC;
 
@@ -47,4 +43,16 @@ JSValue JSNamedNodeMap::nameGetter(ExecState* exec, const Identifier& propertyNa
     return toJS(exec, thisObj->impl()->getNamedItem(propertyName));
 }
 
+void JSNamedNodeMap::markChildren(MarkStack& markStack)
+{
+    Base::markChildren(markStack);
+
+    // Mark the element so that this will work to access the attribute even if the last
+    // other reference goes away.
+    if (Element* element = impl()->element()) {
+        if (JSNode* wrapper = getCachedDOMNodeWrapper(element->document(), element))
+            markStack.append(wrapper);
+    }
+}
+
 } // namespace WebCore
index 29f4be1..c01f34a 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2006, 2007, 2008 Apple Inc. All rights reserved.
+ * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Samuel Weinig <sam.weinig@gmail.com>
  *
  * This library is free software; you can redistribute it and/or
@@ -21,6 +21,7 @@
 module core {
 
     interface [
+        CustomMarkFunction,
         GenerateConstructor,
         GenerateNativeConverter,
         InterfaceUUID=EEE8E22B-22C3-4e50-95F4-5E0B8AAD8231,
index fe631c8..d4ec598 100644 (file)
@@ -178,10 +178,8 @@ Attribute* NamedNodeMap::getAttributeItem(const String& name, bool shouldIgnoreA
 {
     unsigned len = length();
     for (unsigned i = 0; i < len; ++i) {
-        if (!m_attributes[i]->name().hasPrefix() && 
-            m_attributes[i]->name().localName() == name)
-                return m_attributes[i].get();
-
+        if (!m_attributes[i]->name().hasPrefix() && m_attributes[i]->name().localName() == name)
+            return m_attributes[i].get();
         if (shouldIgnoreAttributeCase ? equalIgnoringCase(m_attributes[i]->name().toString(), name) : name == m_attributes[i]->name().toString())
             return m_attributes[i].get();
     }
@@ -206,10 +204,12 @@ void NamedNodeMap::clearAttributes()
 
 void NamedNodeMap::detachFromElement()
 {
-    // we allow a NamedNodeMap w/o an element in case someone still has a reference
-    // to if after the element gets deleted - but the map is now invalid
+    // This can't happen if the holder of the map is JavaScript, because we mark the
+    // element if the map is alive. So it has no impact on web page behavior. Because
+    // of that, we can simply clear all the attributes to avoid accessing stale
+    // pointers to do things like create Attr objects.
     m_element = 0;
-    detachAttributesFromElement();
+    clearAttributes();
 }
 
 void NamedNodeMap::setAttributes(const NamedNodeMap& other)
@@ -251,7 +251,7 @@ void NamedNodeMap::addAttribute(PassRefPtr<Attribute> prpAttribute)
         attr->m_element = m_element;
 
     // Notify the element that the attribute has been added, and dispatch appropriate mutation events
-    // Note that element may be null here if we are called from insertAttr() during parsing
+    // Note that element may be null here if we are called from insertAttribute() during parsing
     if (m_element) {
         m_element->attributeChanged(attribute.get());
         // Because of our updateStyleAttribute() style modification events are never sent at the right time, so don't bother sending them.
@@ -265,12 +265,13 @@ void NamedNodeMap::addAttribute(PassRefPtr<Attribute> prpAttribute)
 void NamedNodeMap::removeAttribute(const QualifiedName& name)
 {
     unsigned len = length();
-    unsigned index = len + 1;
-    for (unsigned i = 0; i < len; ++i)
+    unsigned index = len;
+    for (unsigned i = 0; i < len; ++i) {
         if (m_attributes[i]->name().matches(name)) {
             index = i;
             break;
         }
+    }
 
     if (index >= len)
         return;
index 4fb96de..759900b 100644 (file)
@@ -94,11 +94,11 @@ public:
     void addAttribute(PassRefPtr<Attribute>);
     void removeAttribute(const QualifiedName&);
 
+    Element* element() const { return m_element; }
+
 protected:
     virtual void clearAttributes();
 
-    Element* element() const { return m_element; }
-
 private:
     void detachAttributesFromElement();
     void detachFromElement();
index 3310ded..8166853 100644 (file)
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2006 Samuel Weinig <sam.weinig@gmail.com>
- * Copyright (C) 2007 Apple Inc. All rights reserved.
+ * Copyright (C) 2007, 2009 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -21,6 +21,7 @@
 module core {
 
     interface [
+        CustomMarkFunction,
         GenerateConstructor,
         HasIndexGetter,
         HasNameGetter,