Skip to content
This repository was archived by the owner on Jun 24, 2022. It is now read-only.

Commit 0fbe2ac

Browse files
author
[email protected]
committed
REGRESSION(r192536): Null pointer dereference in JSPropertyNameEnumerator::visitChildren().
<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: http://svn.webkit.org/repository/webkit/trunk@192722 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent c3b0b2b commit 0fbe2ac

File tree

6 files changed

+65
-3
lines changed

6 files changed

+65
-3
lines changed

‎LayoutTests/ChangeLog

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,14 @@
1+
2015-11-20 Andreas Kling <[email protected]>
2+
3+
REGRESSION(r192536): Null pointer dereference in JSPropertyNameEnumerator::visitChildren().
4+
<https://webkit.org/b/151495>
5+
6+
Reviewed by Mark Lam.
7+
8+
* js/property-name-enumerator-gc-151495.html: Added.
9+
* js/property-name-enumerator-gc-151495-expected.txt: Added.
10+
* js/script-tests/property-name-enumerator-gc-151495.js: Added.
11+
112
2015-11-20 Brady Eidson <[email protected]>
213

314
Modern IDB: After versionchange transactions complete, fire onsuccess on the original IDBOpenDBRequest

‎LayoutTests/js/property-name-enumerator-gc-151495-expected.txt

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
Regression test for https://webkit.org/b/151495. - This test should not crash.
2+
3+
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
4+
5+
6+
PASS successfullyParsed is true
7+
8+
TEST COMPLETE
9+

‎LayoutTests/js/property-name-enumerator-gc-151495.html

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
2+
<html>
3+
<head>
4+
<script src="../resources/js-test-pre.js"></script>
5+
</head>
6+
<body>
7+
<script src="script-tests/property-name-enumerator-gc-151495.js"></script>
8+
<script src="../resources/js-test-post.js"></script>
9+
</body>
10+
</html>

‎LayoutTests/js/script-tests/property-name-enumerator-gc-151495.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
description("Regression test for https://webkit.org/b/151495. - This test should not crash.");
2+
3+
var x = { a: 1, b: 2, c: 3, d: 4, e: 5, f: 6 };
4+
for (i = 0; i < 2000; ++i) {
5+
// Keep adding new properties...
6+
x["foo" + i] = 1;
7+
// ...to force creation of new JSPropertyNameEnumerator objects.
8+
for (j in x) { }
9+
}

‎Source/JavaScriptCore/ChangeLog

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,26 @@
1+
2015-11-20 Andreas Kling <[email protected]>
2+
3+
REGRESSION(r192536): Null pointer dereference in JSPropertyNameEnumerator::visitChildren().
4+
<https://webkit.org/b/151495>
5+
6+
Reviewed by Mark Lam.
7+
8+
The copied space allocation in JSPropertyNameEnumerator::finishCreation()
9+
may end up triggering a GC, and so JSPropertyNameEnumerator::visitChildren()
10+
would get called while m_propertyNames was still null.
11+
12+
This patch fixes that by having visitChildren() check for pointer nullity
13+
instead of the number of names, since that is non-zero even before the
14+
allocation is made.
15+
16+
Added a test that induces GC during JSPropertyNameEnumerator construction
17+
to cover this bug.
18+
19+
Test: property-name-enumerator-gc-151495.js
20+
21+
* runtime/JSPropertyNameEnumerator.cpp:
22+
(JSC::JSPropertyNameEnumerator::visitChildren):
23+
124
2015-11-20 Andreas Kling <[email protected]>
225

326
GC timers should carry on gracefully when Heap claims it grew from GC.

‎Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,12 +89,12 @@ void JSPropertyNameEnumerator::visitChildren(JSCell* cell, SlotVisitor& visitor)
8989
JSPropertyNameEnumerator* thisObject = jsCast<JSPropertyNameEnumerator*>(cell);
9090
visitor.append(&thisObject->m_prototypeChain);
9191

92-
if (thisObject->cachedPropertyNameCount()) {
92+
if (auto* propertyNames = thisObject->m_propertyNames.getWithoutBarrier()) {
9393
for (unsigned i = 0; i < thisObject->cachedPropertyNameCount(); ++i)
94-
visitor.append(&thisObject->m_propertyNames.getWithoutBarrier()[i]);
94+
visitor.append(&propertyNames[i]);
9595
visitor.copyLater(
9696
thisObject, JSPropertyNameEnumeratorCopyToken,
97-
thisObject->m_propertyNames.getWithoutBarrier(), thisObject->propertyNameCacheSize());
97+
propertyNames, thisObject->propertyNameCacheSize());
9898
}
9999
}
100100

0 commit comments

Comments
 (0)