- Revision
- 115096
- Author
- [email protected]
- Date
- 2012-04-24 12:43:01 -0700 (Tue, 24 Apr 2012)
Log Message
Failure to allocate ArrayStorage in emit_op_new_array leads to poisonous JSArray
https://bugs.webkit.org/show_bug.cgi?id=84648
Reviewed by Geoffrey Garen.
Source/_javascript_Core:
When emit_op_new_array successfully allocates a new JSArray but fails to allocate
the corresponding ArrayStorage for it, it falls back to the out-of-line stub call
to constructArray, which constructs and entirely new JSArray/ArrayStorage pair.
This leaves us with a JSArray hanging around on the stack or in a register that
did not go through its own constructor, thus giving it uninitialized memory in the
two fields that are checked in JSArray::visitChildren.
* jit/JITInlineMethods.h:
(JSC::JIT::emitAllocateJSArray): We try to allocate the ArrayStorage first, so that
if we fail we haven't generated the poisonous JSArray that can cause a GC crash.
* jit/JITOpcodes.cpp:
(JSC::JIT::emitSlow_op_new_array):
LayoutTests:
Added a test that randomly allocates new arrays, modifies the object graph by
pointing arrays to other arrays, and removes arrays from the global list. This test
is sufficient to repro this bug when the DFG is disabled, and it should serve as
a good regression test as we implement more optimizations for array allocation in
both JITs.
* fast/js/random-array-gc-stress-expected.txt: Added.
* fast/js/random-array-gc-stress.html: Added.
* fast/js/script-tests/random-array-gc-stress.js: Added.
(gc):
(getRandomIndex):
(test):
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (115095 => 115096)
--- trunk/LayoutTests/ChangeLog 2012-04-24 19:41:08 UTC (rev 115095)
+++ trunk/LayoutTests/ChangeLog 2012-04-24 19:43:01 UTC (rev 115096)
@@ -1,3 +1,23 @@
+2012-04-24 Mark Hahnenberg <[email protected]>
+
+ Failure to allocate ArrayStorage in emit_op_new_array leads to poisonous JSArray
+ https://bugs.webkit.org/show_bug.cgi?id=84648
+
+ Reviewed by Geoffrey Garen.
+
+ Added a test that randomly allocates new arrays, modifies the object graph by
+ pointing arrays to other arrays, and removes arrays from the global list. This test
+ is sufficient to repro this bug when the DFG is disabled, and it should serve as
+ a good regression test as we implement more optimizations for array allocation in
+ both JITs.
+
+ * fast/js/random-array-gc-stress-expected.txt: Added.
+ * fast/js/random-array-gc-stress.html: Added.
+ * fast/js/script-tests/random-array-gc-stress.js: Added.
+ (gc):
+ (getRandomIndex):
+ (test):
+
2012-04-21 Robert Hogan <[email protected]>
CSS 2.1 failure: table-columns-example-001 fails
Added: trunk/LayoutTests/fast/js/random-array-gc-stress-expected.txt (0 => 115096)
--- trunk/LayoutTests/fast/js/random-array-gc-stress-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/js/random-array-gc-stress-expected.txt 2012-04-24 19:43:01 UTC (rev 115096)
@@ -0,0 +1,9 @@
+Tests to randomly modify an object graph of arrays to make sure the GC handles them properly now that it allocates both the JSArray and the ArrayStorage. To pass we need to not crash.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/js/random-array-gc-stress.html (0 => 115096)
--- trunk/LayoutTests/fast/js/random-array-gc-stress.html (rev 0)
+++ trunk/LayoutTests/fast/js/random-array-gc-stress.html 2012-04-24 19:43:01 UTC (rev 115096)
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script src=""
+<script src=""
+</body>
+</html>
Added: trunk/LayoutTests/fast/js/script-tests/random-array-gc-stress.js (0 => 115096)
--- trunk/LayoutTests/fast/js/script-tests/random-array-gc-stress.js (rev 0)
+++ trunk/LayoutTests/fast/js/script-tests/random-array-gc-stress.js 2012-04-24 19:43:01 UTC (rev 115096)
@@ -0,0 +1,55 @@
+description(
+'Tests to randomly modify an object graph of arrays to make sure the GC handles them properly now that it allocates both the JSArray and the ArrayStorage. To pass we need to not crash.'
+);
+
+function gc()
+{
+ if (this.GCController)
+ GCController.collect();
+ else
+ for (var i = 0; i < 10000; ++i) // Allocate a sufficient number of objects to force a GC.
+ ({});
+}
+
+var global = [];
+
+var getRandomIndex = function(length) {
+ return Math.floor(Math.random() * length);
+};
+
+var test = function() {
+ var numActions = 4;
+ var iters = 1000000;
+ var percent = 0;
+
+ for (var i = 0; i < iters; i++) {
+ var r = Math.floor(Math.random() * numActions);
+ var actionNum = r % numActions;
+ if (actionNum === 0 || actionNum === 3) {
+ global.push([]);
+ } else if (actionNum === 1) {
+ global.pop(getRandomIndex(global.length));
+ } else if (actionNum === 2) {
+ if (global.length > 1) {
+ var receivingIndex = getRandomIndex(global.length);
+ var sendingIndex = getRandomIndex(global.length);
+ while (receivingIndex === sendingIndex)
+ sendingIndex = getRandomIndex(global.length);
+ global[receivingIndex].push(global[sendingIndex]);
+ } else
+ continue;
+ }
+
+ if (Math.floor((i / iters) * 100) !== percent) {
+ percent = Math.floor((i / iters) * 100);
+ }
+ }
+};
+
+var runs = 0;
+while (runs < 40) {
+ test();
+ runs += 1;
+ global = [];
+ gc();
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (115095 => 115096)
--- trunk/Source/_javascript_Core/ChangeLog 2012-04-24 19:41:08 UTC (rev 115095)
+++ trunk/Source/_javascript_Core/ChangeLog 2012-04-24 19:43:01 UTC (rev 115096)
@@ -1,3 +1,23 @@
+2012-04-24 Mark Hahnenberg <[email protected]>
+
+ Failure to allocate ArrayStorage in emit_op_new_array leads to poisonous JSArray
+ https://bugs.webkit.org/show_bug.cgi?id=84648
+
+ Reviewed by Geoffrey Garen.
+
+ When emit_op_new_array successfully allocates a new JSArray but fails to allocate
+ the corresponding ArrayStorage for it, it falls back to the out-of-line stub call
+ to constructArray, which constructs and entirely new JSArray/ArrayStorage pair.
+ This leaves us with a JSArray hanging around on the stack or in a register that
+ did not go through its own constructor, thus giving it uninitialized memory in the
+ two fields that are checked in JSArray::visitChildren.
+
+ * jit/JITInlineMethods.h:
+ (JSC::JIT::emitAllocateJSArray): We try to allocate the ArrayStorage first, so that
+ if we fail we haven't generated the poisonous JSArray that can cause a GC crash.
+ * jit/JITOpcodes.cpp:
+ (JSC::JIT::emitSlow_op_new_array):
+
2012-04-23 Filip Pizlo <[email protected]>
DFG on ARMv7 should not OSR exit on every integer division
Modified: trunk/Source/_javascript_Core/jit/JITInlineMethods.h (115095 => 115096)
--- trunk/Source/_javascript_Core/jit/JITInlineMethods.h 2012-04-24 19:41:08 UTC (rev 115095)
+++ trunk/Source/_javascript_Core/jit/JITInlineMethods.h 2012-04-24 19:43:01 UTC (rev 115096)
@@ -485,12 +485,13 @@
unsigned initialLength = std::max(length, 4U);
size_t initialStorage = JSArray::storageSize(initialLength);
+ // We allocate the backing store first to ensure that garbage collection
+ // doesn't happen during JSArray initialization.
+ emitAllocateBasicStorage(initialStorage, storageResult, storagePtr);
+
// Allocate the cell for the array.
emitAllocateBasicJSObject<JSArray, false>(TrustedImmPtr(m_codeBlock->globalObject()->arrayStructure()), cellResult, storagePtr);
- // Allocate the backing store for the array.
- emitAllocateBasicStorage(initialStorage, storageResult, storagePtr);
-
// Store all the necessary info in the ArrayStorage.
storePtr(storageResult, Address(storageResult, ArrayStorage::allocBaseOffset()));
store32(Imm32(length), Address(storageResult, ArrayStorage::lengthOffset()));
Modified: trunk/Source/_javascript_Core/jit/JITOpcodes.cpp (115095 => 115096)
--- trunk/Source/_javascript_Core/jit/JITOpcodes.cpp 2012-04-24 19:41:08 UTC (rev 115095)
+++ trunk/Source/_javascript_Core/jit/JITOpcodes.cpp 2012-04-24 19:43:01 UTC (rev 115096)
@@ -1710,11 +1710,14 @@
void JIT::emitSlow_op_new_array(Instruction* currentInstruction, Vector<SlowCaseEntry>::iterator& iter)
{
+ // If the allocation would be oversize, we will already make the proper stub call above in
+ // emit_op_new_array.
int length = currentInstruction[3].u.operand;
if (CopiedSpace::isOversize(JSArray::storageSize(length)))
return;
+ linkSlowCase(iter); // Not enough space in CopiedSpace for storage.
linkSlowCase(iter); // Not enough space in MarkedSpace for cell.
- linkSlowCase(iter); // Not enough space in CopiedSpace for storage.
+
JITStubCall stubCall(this, cti_op_new_array);
stubCall.addArgument(TrustedImm32(currentInstruction[2].u.operand));
stubCall.addArgument(TrustedImm32(currentInstruction[3].u.operand));
