From e9abb04d118477359cd6bd5a09083eefe9ff4812 Mon Sep 17 00:00:00 2001 From: Mark Lam Date: Mon, 18 May 2015 12:02:56 +0000 Subject: [PATCH] Merge r181305 - 8-bit version of weakCompareAndSwap() can cause an infinite loop. https://webkit.org/b/142513> Reviewed by Filip Pizlo. Source/JavaScriptCore: Added a test that exercises the 8-bit CAS from multiple threads. The threads will contend to set bits in a large array of bytes using the CAS function. * API/tests/CompareAndSwapTest.cpp: Added. (Bitmap::Bitmap): (Bitmap::numBits): (Bitmap::clearAll): (Bitmap::concurrentTestAndSet): (setBitThreadFunc): (testCompareAndSwap): * API/tests/testapi.c: (main): * JavaScriptCore.vcxproj/testapi/testapi.vcxproj: * JavaScriptCore.vcxproj/testapi/testapi.vcxproj.filters: * JavaScriptCore.xcodeproj/project.pbxproj: Source/WTF: Presently, Bitmap::concurrentTestAndSet() uses the 8-bit version of weakCompareAndSwap() (which compares and swaps an uint8_t value). Bitmap::concurrentTestAndSet() has a loop that checks if a bit in the byte of interest has been set. If not, it will call the 8-bit CAS function to set the bit. Under the covers, for ARM, the 8-bit CAS function actually works with a 32-bit CAS. The 8-bit CAS will first fetch the 32-bit value in memory that should contain the 8-bit value, and check if it contains the expected byte. If the value in memory doesn't have the expected byte, it will return early to its caller. The expectation is that the caller will reload the byte from memory and call the 8-bit CAS again. Unfortunately, this code path that returns early does not have a compiler fence. Without a compiler fence, the C++ compiler can optimize away the reloading of the expected byte value, leaving it unchanged. As a result, we'll have a infinite loop here that checks a value that will never change, and the loop will not terminate until the value changes. The fix is to eliminate the early return check in the 8-bit CAS, and have it always call down to the 32-bit CAS. The 32-bit CAS has a compiler fence which will prevent this issue. * wtf/Atomics.h: (WTF::weakCompareAndSwap): --- .../API/tests/CompareAndSwapTest.cpp | 118 ++++++++++++++++++ Source/JavaScriptCore/API/tests/testapi.c | 3 + Source/JavaScriptCore/ChangeLog | 23 ++++ Source/WTF/ChangeLog | 34 +++++ Source/WTF/wtf/Atomics.h | 28 +++-- 5 files changed, 196 insertions(+), 10 deletions(-) create mode 100644 Source/JavaScriptCore/API/tests/CompareAndSwapTest.cpp diff --git a/Source/JavaScriptCore/API/tests/CompareAndSwapTest.cpp b/Source/JavaScriptCore/API/tests/CompareAndSwapTest.cpp new file mode 100644 index 0000000000000..efe0a72a25bc7 --- /dev/null +++ b/Source/JavaScriptCore/API/tests/CompareAndSwapTest.cpp @@ -0,0 +1,118 @@ +/* + * Copyright (C) 2015 Apple Inc. All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS + * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF + * THE POSSIBILITY OF SUCH DAMAGE. + */ + +#include "config.h" + +#include +#include +#include + +// Regression test for webkit.org/b/142513 +extern "C" void testCompareAndSwap(); + +class Bitmap { +public: + Bitmap() { clearAll(); } + + inline void clearAll(); + inline bool concurrentTestAndSet(size_t n); + inline size_t numBits() const { return words * wordSize; } + +private: + static const size_t Size = 4096*10; + + static const unsigned wordSize = sizeof(uint8_t) * 8; + static const unsigned words = (Size + wordSize - 1) / wordSize; + static const uint8_t one = 1; + + uint8_t bits[words]; +}; + +inline void Bitmap::clearAll() +{ + memset(&bits, 0, sizeof(bits)); +} + +inline bool Bitmap::concurrentTestAndSet(size_t n) +{ + uint8_t mask = one << (n % wordSize); + size_t index = n / wordSize; + uint8_t* wordPtr = &bits[index]; + uint8_t oldValue; + do { + oldValue = *wordPtr; + if (oldValue & mask) + return true; + } while (!WTF::weakCompareAndSwap(wordPtr, oldValue, oldValue | mask)); + return false; +} + +struct Data { + Bitmap* bitmap; + int id; + int numThreads; +}; + +static void* setBitThreadFunc(void* p) +{ + Data* data = reinterpret_cast(p); + Bitmap* bitmap = data->bitmap; + size_t numBits = bitmap->numBits(); + + // The computed start index here is heuristic that seems to maximize (anecdotally) + // the chance for the CAS issue to manifest. + size_t start = (numBits * (data->numThreads - data->id)) / data->numThreads; + + printf(" started Thread %d\n", data->id); + for (size_t i = start; i < numBits; i++) + while (!bitmap->concurrentTestAndSet(i)) { } + for (size_t i = 0; i < start; i++) + while (!bitmap->concurrentTestAndSet(i)) { } + + printf(" finished Thread %d\n", data->id); + pthread_exit(nullptr); +} + +void testCompareAndSwap() +{ + Bitmap bitmap; + const int numThreads = 5; + pthread_t threadIDs[numThreads]; + Data data[numThreads]; + + printf("Starting %d threads for CompareAndSwap test. Test should complete without hanging.\n", numThreads); + for (int i = 0; i < numThreads; i++) { + data[i].bitmap = &bitmap; + data[i].id = i; + data[i].numThreads = numThreads; + pthread_create(&threadIDs[i], 0, &setBitThreadFunc, &data[i]); + } + + printf("Waiting for %d threads to join\n", numThreads); + for (int i = 0; i < numThreads; i++) + pthread_join(threadIDs[i], nullptr); + + printf("PASS: CompareAndSwap test completed without a hang\n"); +} diff --git a/Source/JavaScriptCore/API/tests/testapi.c b/Source/JavaScriptCore/API/tests/testapi.c index 3cb40c664b3a1..a6383ebaf26f2 100644 --- a/Source/JavaScriptCore/API/tests/testapi.c +++ b/Source/JavaScriptCore/API/tests/testapi.c @@ -46,6 +46,7 @@ #if JSC_OBJC_API_ENABLED void testObjectiveCAPI(void); #endif +void testCompareAndSwap(); extern void JSSynchronousGarbageCollectForDebugging(JSContextRef); @@ -1174,6 +1175,8 @@ int main(int argc, char* argv[]) ::SetErrorMode(0); #endif + testCompareAndSwap(); + #if JSC_OBJC_API_ENABLED testObjectiveCAPI(); #endif diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog index a168fa446e9c0..c8f4960029252 100644 --- a/Source/JavaScriptCore/ChangeLog +++ b/Source/JavaScriptCore/ChangeLog @@ -1,3 +1,26 @@ +2015-03-09 Mark Lam + + 8-bit version of weakCompareAndSwap() can cause an infinite loop. + https://webkit.org/b/142513> + + Reviewed by Filip Pizlo. + + Added a test that exercises the 8-bit CAS from multiple threads. The threads + will contend to set bits in a large array of bytes using the CAS function. + + * API/tests/CompareAndSwapTest.cpp: Added. + (Bitmap::Bitmap): + (Bitmap::numBits): + (Bitmap::clearAll): + (Bitmap::concurrentTestAndSet): + (setBitThreadFunc): + (testCompareAndSwap): + * API/tests/testapi.c: + (main): + * JavaScriptCore.vcxproj/testapi/testapi.vcxproj: + * JavaScriptCore.vcxproj/testapi/testapi.vcxproj.filters: + * JavaScriptCore.xcodeproj/project.pbxproj: + 2014-12-04 Oliver Hunt Serialization of MapData object provides unsafe access to internal types diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog index fa3c72db64265..39ab65babcd32 100644 --- a/Source/WTF/ChangeLog +++ b/Source/WTF/ChangeLog @@ -1,3 +1,37 @@ +2015-03-09 Mark Lam + + 8-bit version of weakCompareAndSwap() can cause an infinite loop. + https://webkit.org/b/142513> + + Reviewed by Filip Pizlo. + + Presently, Bitmap::concurrentTestAndSet() uses the 8-bit version of + weakCompareAndSwap() (which compares and swaps an uint8_t value). + Bitmap::concurrentTestAndSet() has a loop that checks if a bit in the + byte of interest has been set. If not, it will call the 8-bit CAS + function to set the bit. + + Under the covers, for ARM, the 8-bit CAS function actually works with a + 32-bit CAS. The 8-bit CAS will first fetch the 32-bit value in memory + that should contain the 8-bit value, and check if it contains the + expected byte. If the value in memory doesn't have the expected byte, + it will return early to its caller. The expectation is that the caller + will reload the byte from memory and call the 8-bit CAS again. + + Unfortunately, this code path that returns early does not have a + compiler fence. Without a compiler fence, the C++ compiler can + optimize away the reloading of the expected byte value, leaving it + unchanged. As a result, we'll have a infinite loop here that checks a + value that will never change, and the loop will not terminate until the + value changes. + + The fix is to eliminate the early return check in the 8-bit CAS, and + have it always call down to the 32-bit CAS. The 32-bit CAS has a + compiler fence which will prevent this issue. + + * wtf/Atomics.h: + (WTF::weakCompareAndSwap): + 2014-10-22 Byungseon Shin String(new Date(Mar 30 2014 01:00:00)) is wrong in CET diff --git a/Source/WTF/wtf/Atomics.h b/Source/WTF/wtf/Atomics.h index 3d18e3e78a107..9946898e666b4 100644 --- a/Source/WTF/wtf/Atomics.h +++ b/Source/WTF/wtf/Atomics.h @@ -292,16 +292,24 @@ inline bool weakCompareAndSwap(uint8_t* location, uint8_t expected, uint8_t newV unsigned* alignedLocation = bitwise_cast(alignedLocationValue); // Make sure that this load is always issued and never optimized away. unsigned oldAlignedValue = *const_cast(alignedLocation); - union { - uint8_t bytes[sizeof(unsigned)]; - unsigned word; - } u; - u.word = oldAlignedValue; - if (u.bytes[locationOffset] != expected) - return false; - u.bytes[locationOffset] = newValue; - unsigned newAlignedValue = u.word; - return weakCompareAndSwap(alignedLocation, oldAlignedValue, newAlignedValue); + + struct Splicer { + static unsigned splice(unsigned value, uint8_t byte, uintptr_t byteIndex) + { + union { + unsigned word; + uint8_t bytes[sizeof(unsigned)]; + } u; + u.word = value; + u.bytes[byteIndex] = byte; + return u.word; + } + }; + + unsigned expectedAlignedValue = Splicer::splice(oldAlignedValue, expected, locationOffset); + unsigned newAlignedValue = Splicer::splice(oldAlignedValue, newValue, locationOffset); + + return weakCompareAndSwap(alignedLocation, expectedAlignedValue, newAlignedValue); #endif #else UNUSED_PARAM(location);