unshift should zero unused property storage
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 23 Jun 2018 05:27:44 +0000 (05:27 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 23 Jun 2018 05:27:44 +0000 (05:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=186960

Reviewed by Saam Barati.

JSTests:

* stress/array-unshift-zero-property-storage.js: Added.
(run):
(test):

Source/JavaScriptCore:

Also, this patch adds the zeroed unused property storage assertion
to one more place it was missing.

* runtime/JSArray.cpp:
(JSC::JSArray::unshiftCountSlowCase):
* runtime/JSObjectInlines.h:
(JSC::JSObject::putDirectInternal):

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

JSTests/ChangeLog
JSTests/stress/array-unshift-zero-property-storage.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/JSArray.cpp
Source/JavaScriptCore/runtime/JSObjectInlines.h

index e3f4da7..ff081ea 100644 (file)
@@ -1,3 +1,14 @@
+2018-06-22  Keith Miller  <keith_miller@apple.com>
+
+        unshift should zero unused property storage
+        https://bugs.webkit.org/show_bug.cgi?id=186960
+
+        Reviewed by Saam Barati.
+
+        * stress/array-unshift-zero-property-storage.js: Added.
+        (run):
+        (test):
+
 2018-06-22  Mark Lam  <mark.lam@apple.com>
 
         PropertyCondition::isValidValueForAttributes() should also consider deleted values.
diff --git a/JSTests/stress/array-unshift-zero-property-storage.js b/JSTests/stress/array-unshift-zero-property-storage.js
new file mode 100644 (file)
index 0000000..b4ed033
--- /dev/null
@@ -0,0 +1,34 @@
+let foo = "get some property storage";
+let first = "new first element";
+let bar = "ensure property storage is zeroed";
+
+function run(array) {
+    array.foo = foo;
+    array.unshift(first, ...new Array(100));
+    array.bar = bar;
+    return array;
+}
+noInline(run);
+
+function test() {
+    let array = run([]);
+    if (array.foo !== foo)
+        throw new Error();
+    if (array.bar !== bar)
+        throw new Error();
+    if (array[0] !== first)
+        throw new Error();
+
+    array = [];
+    array.unshift(1);
+    array = run(array);
+    if (array.foo !== foo)
+        throw new Error();
+    if (array.bar !== bar)
+        throw new Error();
+    if (array[0] !== first)
+        throw new Error();
+}
+
+for (let i = 0; i < 1; i++)
+    test();
index aad4308..9d9006a 100644 (file)
@@ -1,3 +1,18 @@
+2018-06-22  Keith Miller  <keith_miller@apple.com>
+
+        unshift should zero unused property storage
+        https://bugs.webkit.org/show_bug.cgi?id=186960
+
+        Reviewed by Saam Barati.
+
+        Also, this patch adds the zeroed unused property storage assertion
+        to one more place it was missing.
+
+        * runtime/JSArray.cpp:
+        (JSC::JSArray::unshiftCountSlowCase):
+        * runtime/JSObjectInlines.h:
+        (JSC::JSObject::putDirectInternal):
+
 2018-06-22  Mark Lam  <mark.lam@apple.com>
 
         PropertyCondition::isValidValueForAttributes() should also consider deleted values.
index 83b5145..731e45b 100644 (file)
@@ -347,7 +347,7 @@ bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool
     // Step 2:
     // We're either going to choose to allocate a new ArrayStorage, or we're going to reuse the existing one.
 
-    void* newAllocBase = 0;
+    void* newAllocBase = nullptr;
     unsigned newStorageCapacity;
     bool allocatedNewStorage;
     // If the current storage array is sufficiently large (but not too large!) then just keep using it.
@@ -356,11 +356,11 @@ bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool
         newStorageCapacity = currentCapacity;
         allocatedNewStorage = false;
     } else {
-        size_t newSize = Butterfly::totalSize(0, propertyCapacity, true, ArrayStorage::sizeFor(desiredCapacity));
-        newAllocBase = vm.jsValueGigacageAuxiliarySpace.allocateNonVirtual(
-            vm, newSize, nullptr, AllocationFailureMode::ReturnNull);
-        if (!newAllocBase)
+        const unsigned preCapacity = 0;
+        Butterfly* newButterfly = Butterfly::tryCreateUninitialized(vm, this, preCapacity, propertyCapacity, true, ArrayStorage::sizeFor(desiredCapacity));
+        if (!newButterfly)
             return false;
+        newAllocBase = newButterfly->base(preCapacity, propertyCapacity);
         newStorageCapacity = desiredCapacity;
         allocatedNewStorage = true;
     }
@@ -385,23 +385,25 @@ bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool
 
     unsigned newVectorLength = requiredVectorLength + postCapacity;
     RELEASE_ASSERT(newVectorLength <= MAX_STORAGE_VECTOR_LENGTH);
-    unsigned newIndexBias = newStorageCapacity - newVectorLength;
+    unsigned preCapacity = newStorageCapacity - newVectorLength;
 
-    Butterfly* newButterfly = Butterfly::fromBase(newAllocBase, newIndexBias, propertyCapacity);
+    Butterfly* newButterfly = Butterfly::fromBase(newAllocBase, preCapacity, propertyCapacity);
 
     if (addToFront) {
         ASSERT(count + usedVectorLength <= newVectorLength);
         memmove(newButterfly->arrayStorage()->m_vector + count, storage->m_vector, sizeof(JSValue) * usedVectorLength);
         memmove(newButterfly->propertyStorage() - propertySize, butterfly->propertyStorage() - propertySize, sizeof(JSValue) * propertySize + sizeof(IndexingHeader) + ArrayStorage::sizeFor(0));
-        
+
         if (allocatedNewStorage) {
             // We will set the vectorLength to newVectorLength. We populated requiredVectorLength
             // (usedVectorLength + count), which is less. Clear the difference.
             for (unsigned i = requiredVectorLength; i < newVectorLength; ++i)
                 newButterfly->arrayStorage()->m_vector[i].clear();
+            // We don't need to zero the pre-capacity because it is not available to use as property storage.
+            memset(newButterfly->base(0, propertyCapacity), 0, (propertyCapacity - propertySize) * sizeof(JSValue));
         }
-    } else if ((newAllocBase != butterfly->base(structure)) || (newIndexBias != storage->m_indexBias)) {
-        memmove(newButterfly->propertyStorage() - propertySize, butterfly->propertyStorage() - propertySize, sizeof(JSValue) * propertySize + sizeof(IndexingHeader) + ArrayStorage::sizeFor(0));
+    } else if ((newAllocBase != butterfly->base(structure)) || (preCapacity != storage->m_indexBias)) {
+        memmove(newButterfly->propertyStorage() - propertyCapacity, butterfly->propertyStorage() - propertyCapacity, sizeof(JSValue) * propertyCapacity + sizeof(IndexingHeader) + ArrayStorage::sizeFor(0));
         memmove(newButterfly->arrayStorage()->m_vector, storage->m_vector, sizeof(JSValue) * usedVectorLength);
         
         for (unsigned i = requiredVectorLength; i < newVectorLength; i++)
@@ -409,7 +411,7 @@ bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool
     }
 
     newButterfly->arrayStorage()->setVectorLength(newVectorLength);
-    newButterfly->arrayStorage()->m_indexBias = newIndexBias;
+    newButterfly->arrayStorage()->m_indexBias = preCapacity;
     
     setButterfly(vm, newButterfly);
 
index 5e93e26..1a0888e 100644 (file)
@@ -329,6 +329,10 @@ ALWAYS_INLINE bool JSObject::putDirectInternal(VM& vm, PropertyName propertyName
 
         validateOffset(offset);
         ASSERT(newStructure->isValidOffset(offset));
+
+        // This assertion verifies that the concurrent GC won't read garbage if the concurrentGC
+        // is running at the same time we put without transitioning.
+        ASSERT(!getDirect(offset) || !JSValue::encode(getDirect(offset)));
         putDirect(vm, offset, value);
         setStructure(vm, newStructure);
         slot.setNewProperty(this, offset);