REGRESSION(r192536): Null pointer dereference in JSPropertyNameEnumerator::visitChild...
authorakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 21 Nov 2015 06:07:40 +0000 (06:07 +0000)
committerakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 21 Nov 2015 06:07:40 +0000 (06:07 +0000)
<https://webkit.org/b/151495>

Reviewed by Mark Lam.

Source/JavaScriptCore:

The copied space allocation in JSPropertyNameEnumerator::finishCreation()
may end up triggering a GC, and so JSPropertyNameEnumerator::visitChildren()
would get called while m_propertyNames was still null.

This patch fixes that by having visitChildren() check for pointer nullity
instead of the number of names, since that is non-zero even before the
allocation is made.

Added a test that induces GC during JSPropertyNameEnumerator construction
to cover this bug.

Test: property-name-enumerator-gc-151495.js

* runtime/JSPropertyNameEnumerator.cpp:
(JSC::JSPropertyNameEnumerator::visitChildren):

LayoutTests:

* js/property-name-enumerator-gc-151495.html: Added.
* js/property-name-enumerator-gc-151495-expected.txt: Added.
* js/script-tests/property-name-enumerator-gc-151495.js: Added.

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

LayoutTests/ChangeLog
LayoutTests/js/property-name-enumerator-gc-151495-expected.txt [new file with mode: 0644]
LayoutTests/js/property-name-enumerator-gc-151495.html [new file with mode: 0644]
LayoutTests/js/script-tests/property-name-enumerator-gc-151495.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.cpp

index 946cf47..503b26f 100644 (file)
@@ -1,3 +1,14 @@
+2015-11-20  Andreas Kling  <akling@apple.com>
+
+        REGRESSION(r192536): Null pointer dereference in JSPropertyNameEnumerator::visitChildren().
+        <https://webkit.org/b/151495>
+
+        Reviewed by Mark Lam.
+
+        * js/property-name-enumerator-gc-151495.html: Added.
+        * js/property-name-enumerator-gc-151495-expected.txt: Added.
+        * js/script-tests/property-name-enumerator-gc-151495.js: Added.
+
 2015-11-20  Brady Eidson  <beidson@apple.com>
 
         Modern IDB: After versionchange transactions complete, fire onsuccess on the original IDBOpenDBRequest
diff --git a/LayoutTests/js/property-name-enumerator-gc-151495-expected.txt b/LayoutTests/js/property-name-enumerator-gc-151495-expected.txt
new file mode 100644 (file)
index 0000000..6e12bcb
--- /dev/null
@@ -0,0 +1,9 @@
+Regression test for https://webkit.org/b/151495. - This test should not crash.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/js/property-name-enumerator-gc-151495.html b/LayoutTests/js/property-name-enumerator-gc-151495.html
new file mode 100644 (file)
index 0000000..53bf884
--- /dev/null
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script src="script-tests/property-name-enumerator-gc-151495.js"></script>
+<script src="../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/js/script-tests/property-name-enumerator-gc-151495.js b/LayoutTests/js/script-tests/property-name-enumerator-gc-151495.js
new file mode 100644 (file)
index 0000000..0caffb9
--- /dev/null
@@ -0,0 +1,9 @@
+description("Regression test for https://webkit.org/b/151495. - This test should not crash.");
+
+var x = { a: 1, b: 2, c: 3, d: 4, e: 5, f: 6 };
+for (i = 0; i < 2000; ++i) {
+    // Keep adding new properties...
+    x["foo" + i] = 1;
+    // ...to force creation of new JSPropertyNameEnumerator objects.
+    for (j in x) { }
+}
index b8ad35d..4b165dd 100644 (file)
@@ -1,5 +1,28 @@
 2015-11-20  Andreas Kling  <akling@apple.com>
 
+        REGRESSION(r192536): Null pointer dereference in JSPropertyNameEnumerator::visitChildren().
+        <https://webkit.org/b/151495>
+
+        Reviewed by Mark Lam.
+
+        The copied space allocation in JSPropertyNameEnumerator::finishCreation()
+        may end up triggering a GC, and so JSPropertyNameEnumerator::visitChildren()
+        would get called while m_propertyNames was still null.
+
+        This patch fixes that by having visitChildren() check for pointer nullity
+        instead of the number of names, since that is non-zero even before the
+        allocation is made.
+
+        Added a test that induces GC during JSPropertyNameEnumerator construction
+        to cover this bug.
+
+        Test: property-name-enumerator-gc-151495.js
+
+        * runtime/JSPropertyNameEnumerator.cpp:
+        (JSC::JSPropertyNameEnumerator::visitChildren):
+
+2015-11-20  Andreas Kling  <akling@apple.com>
+
         GC timers should carry on gracefully when Heap claims it grew from GC.
         <https://webkit.org/b/151521>
 
index 8b5f74e..d6004fb 100644 (file)
@@ -89,12 +89,12 @@ void JSPropertyNameEnumerator::visitChildren(JSCell* cell, SlotVisitor& visitor)
     JSPropertyNameEnumerator* thisObject = jsCast<JSPropertyNameEnumerator*>(cell);
     visitor.append(&thisObject->m_prototypeChain);
 
-    if (thisObject->cachedPropertyNameCount()) {
+    if (auto* propertyNames = thisObject->m_propertyNames.getWithoutBarrier()) {
         for (unsigned i = 0; i < thisObject->cachedPropertyNameCount(); ++i)
-            visitor.append(&thisObject->m_propertyNames.getWithoutBarrier()[i]);
+            visitor.append(&propertyNames[i]);
         visitor.copyLater(
             thisObject, JSPropertyNameEnumeratorCopyToken,
-            thisObject->m_propertyNames.getWithoutBarrier(), thisObject->propertyNameCacheSize());
+            propertyNames, thisObject->propertyNameCacheSize());
     }
 }