Title: [228488] trunk
Revision
228488
Author
sbar...@apple.com
Date
2018-02-14 15:25:52 -0800 (Wed, 14 Feb 2018)

Log Message

Setting a VMTrap shouldn't look at topCallFrame since that may imply we're in C code and holding the malloc lock
https://bugs.webkit.org/show_bug.cgi?id=182801

Reviewed by Keith Miller.

JSTests:

* stress/watchdog-dont-malloc-when-in-c-code.js: Added.

Source/_javascript_Core:

VMTraps would sometimes install traps when it paused the JS thread when it
was in C code. This is wrong, as installing traps mallocs, and the JS thread
may have been holding the malloc lock while in C code. This could lead to a
deadlock when C code was holding the malloc lock.

This patch makes it so that we only install traps when we've proven the PC
is in JIT or LLInt code. If we're in JIT/LLInt code, we are guaranteed that
we're not holding the malloc lock.

* jsc.cpp:
(GlobalObject::finishCreation):
(functionMallocInALoop):
* runtime/VMTraps.cpp:
(JSC::VMTraps::tryInstallTrapBreakpoints):

Modified Paths

Added Paths

Diff

Modified: trunk/JSTests/ChangeLog (228487 => 228488)


--- trunk/JSTests/ChangeLog	2018-02-14 22:45:50 UTC (rev 228487)
+++ trunk/JSTests/ChangeLog	2018-02-14 23:25:52 UTC (rev 228488)
@@ -1,3 +1,12 @@
+2018-02-14  Saam Barati  <sbar...@apple.com>
+
+        Setting a VMTrap shouldn't look at topCallFrame since that may imply we're in C code and holding the malloc lock
+        https://bugs.webkit.org/show_bug.cgi?id=182801
+
+        Reviewed by Keith Miller.
+
+        * stress/watchdog-dont-malloc-when-in-c-code.js: Added.
+
 2018-02-14  Ryan Haddad  <ryanhad...@apple.com>
 
         Skip JSC test stress/activation-sink-default-value-tdz-error.js on debug.

Added: trunk/JSTests/stress/watchdog-dont-malloc-when-in-c-code.js (0 => 228488)


--- trunk/JSTests/stress/watchdog-dont-malloc-when-in-c-code.js	                        (rev 0)
+++ trunk/JSTests/stress/watchdog-dont-malloc-when-in-c-code.js	2018-02-14 23:25:52 UTC (rev 228488)
@@ -0,0 +1,5 @@
+//@ runFTLEagerWatchdog
+
+for (let i = 0; i < 7000; ++i) {
+    mallocInALoop();
+}

Modified: trunk/Source/_javascript_Core/ChangeLog (228487 => 228488)


--- trunk/Source/_javascript_Core/ChangeLog	2018-02-14 22:45:50 UTC (rev 228487)
+++ trunk/Source/_javascript_Core/ChangeLog	2018-02-14 23:25:52 UTC (rev 228488)
@@ -1,3 +1,25 @@
+2018-02-14  Saam Barati  <sbar...@apple.com>
+
+        Setting a VMTrap shouldn't look at topCallFrame since that may imply we're in C code and holding the malloc lock
+        https://bugs.webkit.org/show_bug.cgi?id=182801
+
+        Reviewed by Keith Miller.
+
+        VMTraps would sometimes install traps when it paused the JS thread when it
+        was in C code. This is wrong, as installing traps mallocs, and the JS thread
+        may have been holding the malloc lock while in C code. This could lead to a
+        deadlock when C code was holding the malloc lock.
+        
+        This patch makes it so that we only install traps when we've proven the PC
+        is in JIT or LLInt code. If we're in JIT/LLInt code, we are guaranteed that
+        we're not holding the malloc lock.
+
+        * jsc.cpp:
+        (GlobalObject::finishCreation):
+        (functionMallocInALoop):
+        * runtime/VMTraps.cpp:
+        (JSC::VMTraps::tryInstallTrapBreakpoints):
+
 2018-02-14  Michael Saboff  <msab...@apple.com>
 
         REGRESSION(225695) : com.apple.WebKit.WebContent at com.apple._javascript_Core: JSC::RegExp::match + 630 :: stack overflow

Modified: trunk/Source/_javascript_Core/jsc.cpp (228487 => 228488)


--- trunk/Source/_javascript_Core/jsc.cpp	2018-02-14 22:45:50 UTC (rev 228487)
+++ trunk/Source/_javascript_Core/jsc.cpp	2018-02-14 23:25:52 UTC (rev 228488)
@@ -342,6 +342,7 @@
 static EncodedJSValue JSC_HOST_CALL functionHeapCapacity(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionFlashHeapAccess(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionDisableRichSourceInfo(ExecState*);
+static EncodedJSValue JSC_HOST_CALL functionMallocInALoop(ExecState*);
 
 struct Script {
     enum class StrictMode {
@@ -599,6 +600,7 @@
         addFunction(vm, "flashHeapAccess", functionFlashHeapAccess, 0);
 
         addFunction(vm, "disableRichSourceInfo", functionDisableRichSourceInfo, 0);
+        addFunction(vm, "mallocInALoop", functionMallocInALoop, 0);
     }
     
     void addFunction(VM& vm, JSObject* object, const char* name, NativeFunction function, unsigned arguments)
@@ -1748,6 +1750,16 @@
     return JSValue::encode(jsUndefined());
 }
 
+EncodedJSValue JSC_HOST_CALL functionMallocInALoop(ExecState*)
+{
+    Vector<void*> ptrs;
+    for (unsigned i = 0; i < 5000; ++i)
+        ptrs.append(fastMalloc(1024 * 2));
+    for (void* ptr : ptrs)
+        fastFree(ptr);
+    return JSValue::encode(jsUndefined());
+}
+
 template<typename ValueType>
 typename std::enable_if<!std::is_fundamental<ValueType>::value>::type addOption(VM&, JSObject*, Identifier, ValueType) { }
 

Modified: trunk/Source/_javascript_Core/runtime/VMTraps.cpp (228487 => 228488)


--- trunk/Source/_javascript_Core/runtime/VMTraps.cpp	2018-02-14 22:45:50 UTC (rev 228487)
+++ trunk/Source/_javascript_Core/runtime/VMTraps.cpp	2018-02-14 23:25:52 UTC (rev 228488)
@@ -72,23 +72,6 @@
     return !vm.entryScope && !vm.ownerThread();
 }
 
-inline CallFrame* sanitizedTopCallFrame(CallFrame* topCallFrame)
-{
-#if !defined(NDEBUG) && !CPU(ARM) && !CPU(MIPS)
-    // prepareForExternalCall() in DFGSpeculativeJIT.h may set topCallFrame to a bad word
-    // before calling native functions, but tryInstallTrapBreakpoints() below expects
-    // topCallFrame to be null if not set.
-#if USE(JSVALUE64)
-    const uintptr_t badBeefWord = 0xbadbeef0badbeef;
-#else
-    const uintptr_t badBeefWord = 0xbadbeef;
-#endif
-    if (topCallFrame == reinterpret_cast<CallFrame*>(badBeefWord))
-        topCallFrame = nullptr;
-#endif
-    return topCallFrame;
-}
-
 static bool isSaneFrame(CallFrame* frame, CallFrame* calleeFrame, EntryFrame* entryFrame, StackBounds stackBounds)
 {
     if (reinterpret_cast<void*>(frame) >= reinterpret_cast<void*>(entryFrame))
@@ -104,22 +87,19 @@
     // Let's get the thread to break at invalidation points if needed.
     VM& vm = this->vm();
     void* trapPC = context.trapPC;
+    // We must ensure we're in JIT/LLint code. If we are, we know a few things:
+    // - The JS thread isn't holding the malloc lock. Therefore, it's safe to malloc below.
+    // - The JS thread isn't holding the CodeBlockSet lock.
+    // If we're not in JIT/LLInt code, we can't run the C++ code below because it
+    // mallocs, and we must prove the JS thread isn't holding the malloc lock
+    // to be able to do that without risking a deadlock.
+    if (!isJITPC(trapPC) && !LLInt::isLLIntPC(trapPC))
+        return;
 
     CallFrame* callFrame = reinterpret_cast<CallFrame*>(context.framePointer);
 
-    auto& lock = vm.heap.codeBlockSet().getLock();
-    // If the target thread is in C++ code it might be holding the codeBlockSet lock.
-    // if it's in JIT code then it cannot be holding that lock but the GC might be.
-    auto codeBlockSetLocker = isJITPC(trapPC) ? holdLock(lock) : tryHoldLock(lock);
-    if (!codeBlockSetLocker)
-        return; // Let the SignalSender try again later.
+    auto codeBlockSetLocker = holdLock(vm.heap.codeBlockSet().getLock());
 
-    if (!isJITPC(trapPC) && !LLInt::isLLIntPC(trapPC)) {
-        // We resort to topCallFrame to see if we can get anything
-        // useful. We usually get here when we're executing C code.
-        callFrame = sanitizedTopCallFrame(vm.topCallFrame);
-    }
-
     CodeBlock* foundCodeBlock = nullptr;
     EntryFrame* entryFrame = vm.topEntryFrame;
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to