Commit b3314e50 authored by darin@apple.com's avatar darin@apple.com

Null-deref when first access to an Attr node is after its Element is destroyed

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
parent 418bfed4
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.
......
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
<!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>
<!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>
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;
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.
/*
* 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
/*
* 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
......@@ -27,10 +27,6 @@
#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
/*
* 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,
......
......@@ -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;
......
......@@ -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();
......
/*
* 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,
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment