Changeset 228488 in webkit

Timestamp:

Feb 14, 2018 3:25:52 PM (6 years ago)

Author:

sbarati@apple.com

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/JavaScriptCore:

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):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/watchdog-dont-malloc-when-in-c-code.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/jsc.cpp (modified) (3 diffs)
• Source/JavaScriptCore/runtime/VMTraps.cpp (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2018-02-14  Saam Barati  <sbarati@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  <ryanhaddad@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2018-02-14  Saam Barati  <sbarati@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  <msaboff@apple.com>
 

trunk/Source/JavaScriptCore/jsc.cpp

@@ -343 +343 @@
 static EncodedJSValue JSC_HOST_CALL functionFlashHeapAccess(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionDisableRichSourceInfo(ExecState*);
+static EncodedJSValue JSC_HOST_CALL functionMallocInALoop(ExecState*);
 
 struct Script {
@@ -600 +601 @@
 
         addFunction(vm, "disableRichSourceInfo", functionDisableRichSourceInfo, 0);
+        addFunction(vm, "mallocInALoop", functionMallocInALoop, 0);
     }
     
@@ -1749 +1751 @@
 }
 
+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) { }

trunk/Source/JavaScriptCore/runtime/VMTraps.cpp

@@ -73 +73 @@
 }
 
-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)
 {
@@ -105 +88 @@
     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.
-
-    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);
-    }
+    auto codeBlockSetLocker = holdLock(vm.heap.codeBlockSet().getLock());
 
     CodeBlock* foundCodeBlock = nullptr;