JSArray has some object scanning races
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 10 Jan 2017 18:44:45 +0000 (18:44 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 10 Jan 2017 18:44:45 +0000 (18:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=166874

Reviewed by Mark Lam.

This fixes two separate bugs, both of which I detected by running
array-splice-contiguous.js in extreme anger:

1) Some of the paths of shifting and unshifting were not grabbing the internal cell
   lock. This was causing the array storage scan to crash, even though it was well
   synchronized (the scan does hold the lock). The fix is just to hold the lock anywhere
   that memmoves the innards of the butterfly.

2) Out of line property scanning was synchronized using double collect snapshot. Array
   storage scanning was synchronized using locks. But what if array storage
   transformations messed up the out of line properties? It turns out that we actually
   need to hoist the array storage scanner's locking up into the double collect
   snapshot.

I don't know how to write a test that does any better of a job of catching this than
array-splice-contiguous.js.

* heap/DeferGC.h: Make DisallowGC usable even if NDEBUG.
* runtime/JSArray.cpp:
(JSC::JSArray::unshiftCountSlowCase):
(JSC::JSArray::shiftCountWithArrayStorage):
(JSC::JSArray::unshiftCountWithArrayStorage):
* runtime/JSObject.cpp:
(JSC::JSObject::visitButterflyImpl):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/DeferGC.h
Source/JavaScriptCore/runtime/JSArray.cpp
Source/JavaScriptCore/runtime/JSArray.h
Source/JavaScriptCore/runtime/JSObject.cpp

index b4f4d6d..75496df 100644 (file)
@@ -1,3 +1,35 @@
+2017-01-09  Filip Pizlo  <fpizlo@apple.com>
+
+        JSArray has some object scanning races
+        https://bugs.webkit.org/show_bug.cgi?id=166874
+
+        Reviewed by Mark Lam.
+        
+        This fixes two separate bugs, both of which I detected by running
+        array-splice-contiguous.js in extreme anger:
+        
+        1) Some of the paths of shifting and unshifting were not grabbing the internal cell
+           lock. This was causing the array storage scan to crash, even though it was well
+           synchronized (the scan does hold the lock). The fix is just to hold the lock anywhere
+           that memmoves the innards of the butterfly.
+        
+        2) Out of line property scanning was synchronized using double collect snapshot. Array
+           storage scanning was synchronized using locks. But what if array storage
+           transformations messed up the out of line properties? It turns out that we actually
+           need to hoist the array storage scanner's locking up into the double collect
+           snapshot.
+        
+        I don't know how to write a test that does any better of a job of catching this than
+        array-splice-contiguous.js.
+
+        * heap/DeferGC.h: Make DisallowGC usable even if NDEBUG.
+        * runtime/JSArray.cpp:
+        (JSC::JSArray::unshiftCountSlowCase):
+        (JSC::JSArray::shiftCountWithArrayStorage):
+        (JSC::JSArray::unshiftCountWithArrayStorage):
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::visitButterflyImpl):
+
 2017-01-10  Andreas Kling  <akling@apple.com>
 
         Crash when GC heap grows way too large.
index c65a4b5..2c0336e 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -67,31 +67,41 @@ private:
     Heap& m_heap;
 };
 
-#ifndef NDEBUG
 class DisallowGC {
     WTF_MAKE_NONCOPYABLE(DisallowGC);
 public:
     DisallowGC()
     {
+#ifndef NDEBUG
         WTF::threadSpecificSet(s_isGCDisallowedOnCurrentThread, reinterpret_cast<void*>(true));
+#endif
     }
 
     ~DisallowGC()
     {
+#ifndef NDEBUG
         WTF::threadSpecificSet(s_isGCDisallowedOnCurrentThread, reinterpret_cast<void*>(false));
+#endif
     }
 
     static bool isGCDisallowedOnCurrentThread()
     {
+#ifndef NDEBUG
         return !!WTF::threadSpecificGet(s_isGCDisallowedOnCurrentThread);
+#else
+        return false;
+#endif
     }
     static void initialize()
     {
+#ifndef NDEBUG
         WTF::threadSpecificKeyCreate(&s_isGCDisallowedOnCurrentThread, 0);
+#endif
     }
 
+#ifndef NDEBUG
     JS_EXPORT_PRIVATE static WTF::ThreadSpecificKey s_isGCDisallowedOnCurrentThread;
+#endif
 };
-#endif // NDEBUG
 
 } // namespace JSC
index 2617253..df69825 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  Copyright (C) 1999-2000 Harri Porten (porten@kde.org)
- *  Copyright (C) 2003, 2007-2009, 2012-2013, 2015-2016 Apple Inc. All rights reserved.
+ *  Copyright (C) 2003-2017 Apple Inc. All rights reserved.
  *  Copyright (C) 2003 Peter Kelly (pmk@post.com)
  *  Copyright (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  *
@@ -298,7 +298,7 @@ void JSArray::getOwnNonIndexPropertyNames(JSObject* object, ExecState* exec, Pro
 }
 
 // This method makes room in the vector, but leaves the new space for count slots uncleared.
-bool JSArray::unshiftCountSlowCase(VM& vm, DeferGC&, bool addToFront, unsigned count)
+bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool addToFront, unsigned count)
 {
     ArrayStorage* storage = ensureArrayStorage(vm);
     Butterfly* butterfly = storage->butterfly();
@@ -892,6 +892,9 @@ bool JSArray::shiftCountWithArrayStorage(VM& vm, unsigned startIndex, unsigned c
     if (startIndex >= vectorLength)
         return true;
     
+    DisallowGC disallowGC;
+    auto locker = holdLock(*this);
+    
     if (startIndex + count > vectorLength)
         count = vectorLength - startIndex;
     
@@ -930,7 +933,7 @@ bool JSArray::shiftCountWithArrayStorage(VM& vm, unsigned startIndex, unsigned c
         // the start of the Butterfly, which needs to point at the first indexed property in the used
         // portion of the vector.
         Butterfly* butterfly = m_butterfly.get()->shift(structure(), count);
-        m_butterfly.setWithoutBarrier(butterfly);
+        setButterfly(vm, butterfly);
         storage = butterfly->arrayStorage();
         storage->m_indexBias += count;
 
@@ -1098,7 +1101,7 @@ bool JSArray::unshiftCountWithArrayStorage(ExecState* exec, unsigned startIndex,
         setButterfly(vm, newButterfly);
     } else if (!moveFront && vectorLength - length >= count)
         storage = storage->butterfly()->arrayStorage();
-    else if (unshiftCountSlowCase(vm, deferGC, moveFront, count))
+    else if (unshiftCountSlowCase(locker, vm, deferGC, moveFront, count))
         storage = arrayStorage();
     else {
         throwOutOfMemoryError(exec, scope);
index 0a57f4a..1631dd2 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  Copyright (C) 1999-2000 Harri Porten (porten@kde.org)
- *  Copyright (C) 2003, 2007, 2008, 2009, 2012, 2015-2016 Apple Inc. All rights reserved.
+ *  Copyright (C) 2003-2017 Apple Inc. All rights reserved.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Lesser General Public
@@ -177,7 +177,7 @@ private:
 
     bool unshiftCountWithAnyIndexingType(ExecState*, unsigned startIndex, unsigned count);
     bool unshiftCountWithArrayStorage(ExecState*, unsigned startIndex, unsigned count, ArrayStorage*);
-    bool unshiftCountSlowCase(VM&, DeferGC&, bool, unsigned);
+    bool unshiftCountSlowCase(const AbstractLocker&, VM&, DeferGC&, bool, unsigned);
 
     bool setLengthWithArrayStorage(ExecState*, unsigned newLength, bool throwException, ArrayStorage*);
     void setLengthWritable(ExecState*, bool writable);
index 1c62aec..d553fea 100644 (file)
@@ -383,6 +383,20 @@ ALWAYS_INLINE Structure* JSObject::visitButterflyImpl(SlotVisitor& visitor)
         return nullptr;
     structure = vm.getStructure(structureID);
     lastOffset = structure->lastOffset();
+    IndexingType indexingType = structure->indexingType();
+    Locker<JSCell> locker(NoLockingNecessary);
+    switch (indexingType) {
+    case ALL_CONTIGUOUS_INDEXING_TYPES:
+    case ALL_ARRAY_STORAGE_INDEXING_TYPES:
+        // We need to hold this lock to protect against changes to the innards of the butterfly
+        // that can happen when the butterfly is used for array storage. We conservatively
+        // assume that a contiguous butterfly may transform into an array storage one, though
+        // this is probably more conservative than necessary.
+        locker = Locker<JSCell>(*this);
+        break;
+    default:
+        break;
+    }
     WTF::loadLoadFence();
     butterfly = this->butterfly();
     if (!butterfly)
@@ -395,27 +409,17 @@ ALWAYS_INLINE Structure* JSObject::visitButterflyImpl(SlotVisitor& visitor)
     
     markAuxiliaryAndVisitOutOfLineProperties(visitor, butterfly, structure, lastOffset);
     
-    IndexingType oldType = structure->indexingType();
-    switch (oldType) {
+    ASSERT(indexingType == structure->indexingType());
+    
+    switch (indexingType) {
     case ALL_CONTIGUOUS_INDEXING_TYPES:
-    case ALL_ARRAY_STORAGE_INDEXING_TYPES: {
-        // This lock is here to protect Contiguous->ArrayStorage transitions, but we could make that
-        // race work if we needed to.
-        auto locker = holdLock(*this);
-        IndexingType newType = this->indexingType();
-        butterfly = this->butterfly();
-        switch (newType) {
-        case ALL_CONTIGUOUS_INDEXING_TYPES:
-            visitor.appendValuesHidden(butterfly->contiguous().data(), butterfly->publicLength());
-            break;
-        default: // ALL_ARRAY_STORAGE_INDEXING_TYPES
-            visitor.appendValuesHidden(butterfly->arrayStorage()->m_vector, butterfly->arrayStorage()->vectorLength());
-            if (butterfly->arrayStorage()->m_sparseMap)
-                visitor.append(butterfly->arrayStorage()->m_sparseMap);
-            break;
-        }
+        visitor.appendValuesHidden(butterfly->contiguous().data(), butterfly->publicLength());
+        break;
+    case ALL_ARRAY_STORAGE_INDEXING_TYPES:
+        visitor.appendValuesHidden(butterfly->arrayStorage()->m_vector, butterfly->arrayStorage()->vectorLength());
+        if (butterfly->arrayStorage()->m_sparseMap)
+            visitor.append(butterfly->arrayStorage()->m_sparseMap);
         break;
-    }
     default:
         break;
     }