JSArrayBufferViews of length 0 allocate 0 CopiedSpace bytes, which is…

… invalid https://bugs.webkit.org/show_bug.cgi?id=123746 Reviewed by Geoffrey Garen. Source/JavaScriptCore: This patch disallows clients from allocating 0 bytes in CopiedSpace. We enforce this invariant with an ASSERT in C++ code and a breakpoint in JIT code. Clients who care about 0-byte allocations (like JSArrayBufferViews) must handle that case themselves, but we don't punish anybody else for the rare case that somebody decides to allocate a 0-length typed array. It also makes the allocation and copying cases consistent for CopiedSpace: no 0-byte allocations, no 0-byte copying. Also added a check so that JSArrayBufferViews don't try to copy their m_vector backing store when their length is 0. Also sprinkled several ASSERTs throughout the JSArrayBufferView code to make sure that when length is 0 m_vector is null. * dfg/DFGSpeculativeJIT.cpp: (JSC::DFG::SpeculativeJIT::compileNewTypedArray): * dfg/DFGSpeculativeJIT.h: (JSC::DFG::SpeculativeJIT::emitAllocateBasicStorage): * heap/CopiedSpaceInlines.h: (JSC::CopiedSpace::tryAllocate): * runtime/ArrayBuffer.h: (JSC::ArrayBuffer::create): * runtime/JSArrayBufferView.cpp: (JSC::JSArrayBufferView::ConstructionContext::ConstructionContext): * runtime/JSGenericTypedArrayViewInlines.h: (JSC::::visitChildren): (JSC::::copyBackingStore): (JSC::::slowDownAndWasteMemory): LayoutTests: Added a test to make sure that we don't crash when allocating a typed array with 0 length. * js/script-tests/typedarray-zero-size.js: Added. (foo): * js/typedarray-zero-size-expected.txt: Added. * js/typedarray-zero-size.html: Added. Canonical link: https://commits.webkit.org/141936@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@158583 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Nov 4, 2013 · 6b0ff88 · 6b0ff88
1 parent c204a1e
commit 6b0ff88
Show file tree

Hide file tree

Showing 11 changed files with 107 additions and 4 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2013-11-04  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        JSArrayBufferViews of length 0 allocate 0 CopiedSpace bytes, which is invalid
+        https://bugs.webkit.org/show_bug.cgi?id=123746
+
+        Reviewed by Geoffrey Garen.
+
+        Added a test to make sure that we don't crash when allocating a typed array with 0 length.
+
+        * js/script-tests/typedarray-zero-size.js: Added.
+        (foo):
+        * js/typedarray-zero-size-expected.txt: Added.
+        * js/typedarray-zero-size.html: Added.
+
 2013-11-04  Alexey Proskuryakov  <ap@apple.com>
 
         Implement generateKey for HMAC and AES-CBC

diff --git a/LayoutTests/js/script-tests/typedarray-zero-size.js b/LayoutTests/js/script-tests/typedarray-zero-size.js
@@ -0,0 +1,22 @@
+description(
+"Tests that creating TypedArrays of length 0 doesn't cause us to crash."
+);
+
+var array = new Uint8Array(0);
+
+function foo() {
+    return new Uint16Array(0);
+}
+
+var result = 0;
+
+for (var i = 1; i < 10001; i++) {
+    var newArray = foo();
+    var otherArray = new Array(i);
+    for (var j = 0; j < i; ++j)
+        otherArray[j] = j;
+    result += otherArray[i - 1];
+}
+
+if (result != (10000 * 9999) / 2)
+    throw "Bad result: " + result;
diff --git a/LayoutTests/js/typedarray-zero-size-expected.txt b/LayoutTests/js/typedarray-zero-size-expected.txt
@@ -0,0 +1,9 @@
+Tests that creating TypedArrays of length 0 doesn't cause us to 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/typedarray-zero-size.html b/LayoutTests/js/typedarray-zero-size.html
@@ -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/typedarray-zero-size.js"></script>
+<script src="../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,36 @@
+2013-11-04  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        JSArrayBufferViews of length 0 allocate 0 CopiedSpace bytes, which is invalid
+        https://bugs.webkit.org/show_bug.cgi?id=123746
+
+        Reviewed by Geoffrey Garen.
+
+        This patch disallows clients from allocating 0 bytes in CopiedSpace. We enforce this invariant 
+        with an ASSERT in C++ code and a breakpoint in JIT code. Clients who care about 0-byte 
+        allocations (like JSArrayBufferViews) must handle that case themselves, but we don't punish 
+        anybody else for the rare case that somebody decides to allocate a 0-length typed array. 
+        It also makes the allocation and copying cases consistent for CopiedSpace: no 0-byte allocations, 
+        no 0-byte copying.
+
+        Also added a check so that JSArrayBufferViews don't try to copy their m_vector backing store when 
+        their length is 0. Also sprinkled several ASSERTs throughout the JSArrayBufferView code to make sure that 
+        when length is 0 m_vector is null.
+
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileNewTypedArray):
+        * dfg/DFGSpeculativeJIT.h:
+        (JSC::DFG::SpeculativeJIT::emitAllocateBasicStorage):
+        * heap/CopiedSpaceInlines.h:
+        (JSC::CopiedSpace::tryAllocate):
+        * runtime/ArrayBuffer.h:
+        (JSC::ArrayBuffer::create):
+        * runtime/JSArrayBufferView.cpp:
+        (JSC::JSArrayBufferView::ConstructionContext::ConstructionContext):
+        * runtime/JSGenericTypedArrayViewInlines.h:
+        (JSC::::visitChildren):
+        (JSC::::copyBackingStore):
+        (JSC::::slowDownAndWasteMemory):
+
 2013-11-04  Julien Brianceau  <jbriance@cisco.com>
 
         [sh4] Refactor jumps in baseline JIT to return label after the jump.

diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
@@ -4709,6 +4709,7 @@ void SpeculativeJIT::compileNewTypedArray(Node* node)
 
     slowCases.append(m_jit.branch32(
         MacroAssembler::Above, sizeGPR, TrustedImm32(JSArrayBufferView::fastSizeLimit)));
+    slowCases.append(m_jit.branchTest32(MacroAssembler::Zero, sizeGPR));
 
     m_jit.move(sizeGPR, scratchGPR);
     m_jit.lshift32(TrustedImm32(logElementSize(type)), scratchGPR);

diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
@@ -2052,7 +2052,15 @@ class SpeculativeJIT {
     MacroAssembler::Jump emitAllocateBasicStorage(SizeType size, GPRReg resultGPR)
     {
         CopiedAllocator* copiedAllocator = &m_jit.vm()->heap.storageAllocator();
-
+
+        // It's invalid to allocate zero bytes in CopiedSpace. 
+#ifndef NDEBUG
+        m_jit.move(size, resultGPR);
+        MacroAssembler::Jump nonZeroSize = m_jit.branchTest32(MacroAssembler::NonZero, resultGPR);
+        m_jit.breakpoint();
+        nonZeroSize.link(&m_jit);
+#endif
+
         m_jit.loadPtr(&copiedAllocator->m_currentRemaining, resultGPR);
         MacroAssembler::Jump slowPath = m_jit.branchSubPtr(JITCompiler::Signed, size, resultGPR);
         m_jit.storePtr(resultGPR, &copiedAllocator->m_currentRemaining);

diff --git a/Source/JavaScriptCore/heap/CopiedSpaceInlines.h b/Source/JavaScriptCore/heap/CopiedSpaceInlines.h
@@ -150,6 +150,7 @@ inline void CopiedSpace::allocateBlock()
 inline CheckedBoolean CopiedSpace::tryAllocate(size_t bytes, void** outPtr)
 {
     ASSERT(!m_heap->vm()->isInitializingObject());
+    ASSERT(bytes);
 
     if (!m_allocator.tryAllocate(bytes, outPtr))
         return tryAllocateSlowCase(bytes, outPtr);

diff --git a/Source/JavaScriptCore/runtime/ArrayBuffer.h b/Source/JavaScriptCore/runtime/ArrayBuffer.h
@@ -160,6 +160,7 @@ PassRefPtr<ArrayBuffer> ArrayBuffer::create(const void* source, unsigned byteLen
     if (!contents.m_data)
         return 0;
     RefPtr<ArrayBuffer> buffer = adoptRef(new ArrayBuffer(contents));
+    ASSERT(!byteLength || source);
     memcpy(buffer->data(), source, byteLength);
     return buffer.release();
 }

diff --git a/Source/JavaScriptCore/runtime/JSArrayBufferView.cpp b/Source/JavaScriptCore/runtime/JSArrayBufferView.cpp
@@ -45,9 +45,10 @@ JSArrayBufferView::ConstructionContext::ConstructionContext(
 {
     if (length <= fastSizeLimit) {
         // Attempt GC allocation.
-        void* temp;
+        void* temp = 0;
         size_t size = sizeOf(length, elementSize);
-        if (!vm.heap.tryAllocateStorage(0, size, &temp))
+        // CopiedSpace only allows non-zero size allocations.
+        if (size && !vm.heap.tryAllocateStorage(0, size, &temp))
             return;
 
         m_structure = structure;

diff --git a/Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h b/Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h
@@ -441,7 +441,8 @@ void JSGenericTypedArrayView<Adaptor>::visitChildren(JSCell* cell, SlotVisitor&
 
     switch (thisObject->m_mode) {
     case FastTypedArray: {
-        visitor.copyLater(thisObject, TypedArrayVectorCopyToken, thisObject->m_vector, thisObject->byteSize());
+        if (thisObject->m_vector)
+            visitor.copyLater(thisObject, TypedArrayVectorCopyToken, thisObject->m_vector, thisObject->byteSize());
         break;
     }
 
@@ -469,6 +470,7 @@ void JSGenericTypedArrayView<Adaptor>::copyBackingStore(
 
     if (token == TypedArrayVectorCopyToken
         && visitor.checkIfShouldCopy(thisObject->m_vector)) {
+        ASSERT(thisObject->m_vector);
         void* oldVector = thisObject->m_vector;
         void* newVector = visitor.allocateNewSpace(thisObject->byteSize());
         memcpy(newVector, oldVector, thisObject->byteSize());
@@ -505,6 +507,7 @@ ArrayBuffer* JSGenericTypedArrayView<Adaptor>::slowDownAndWasteMemory(JSArrayBuf
 
     if (thisObject->m_mode == FastTypedArray
         && !thisObject->m_butterfly && size >= sizeof(IndexingHeader)) {
+        ASSERT(thisObject->m_vector);
         // Reuse already allocated memory if at all possible.
         thisObject->m_butterfly =
             static_cast<IndexingHeader*>(thisObject->m_vector)->butterfly();