[JSC] CodeBlock::calleeSaveRegisters should not see half-baked JITData
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 10 Sep 2019 23:18:11 +0000 (23:18 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 10 Sep 2019 23:18:11 +0000 (23:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=201664
<rdar://problem/52126927>

Reviewed by Tadeu Zagallo.

We are hitting the crash accessing invalid-pointer as CodeBlock::calleeSaveRegisters result.
This is because concurrent Baseline JIT compiler can access m_jitData without taking a lock through CodeBlock::calleeSaveRegisters.
Since m_jitData can be initialized in the main thread while calling CodeBlock::calleeSaveRegisters from concurrent Baseline JIT compiler thread,
we can see half-baked JITData structure which holds garbage pointers.

But we do not want to make CodeBlock::calleeSaveRegisters() call with CodeBlock::m_lock due to several reasons.

1. This function is very primitive one and it is called from various AssemblyHelpers functions and other code-generation functions. Some of these functions are
   called while taking this exact same lock, so dead-lock can happen.
2. JITData::m_calleeSaveRegisters is filled only for DFG and FTL CodeBlock. And DFG and FTL code accesses these field after initializing properly. For Baseline JIT
   compiler case, only thing we should do is that JITData should say m_calleeSaveRegisters is nullptr and it won't be filled for this CodeBlock.

Instead of guarding CodeBlock::calleeSaveRegisters() function with CodeBlock::m_lock, this patch inserts WTF::storeStoreFence when filling m_jitData. This ensures that
JITData::m_calleeSaveRegisters is initialized with nullptr when this JITData pointer is exposed to concurrent Baseline JIT compiler thread.

* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::ensureJITDataSlow):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CodeBlock.cpp

index e3ec524..1f7c2ec 100644 (file)
@@ -1,5 +1,31 @@
 2019-09-10  Yusuke Suzuki  <ysuzuki@apple.com>
 
 2019-09-10  Yusuke Suzuki  <ysuzuki@apple.com>
 
+        [JSC] CodeBlock::calleeSaveRegisters should not see half-baked JITData
+        https://bugs.webkit.org/show_bug.cgi?id=201664
+        <rdar://problem/52126927>
+
+        Reviewed by Tadeu Zagallo.
+
+        We are hitting the crash accessing invalid-pointer as CodeBlock::calleeSaveRegisters result.
+        This is because concurrent Baseline JIT compiler can access m_jitData without taking a lock through CodeBlock::calleeSaveRegisters.
+        Since m_jitData can be initialized in the main thread while calling CodeBlock::calleeSaveRegisters from concurrent Baseline JIT compiler thread,
+        we can see half-baked JITData structure which holds garbage pointers.
+
+        But we do not want to make CodeBlock::calleeSaveRegisters() call with CodeBlock::m_lock due to several reasons.
+
+        1. This function is very primitive one and it is called from various AssemblyHelpers functions and other code-generation functions. Some of these functions are
+           called while taking this exact same lock, so dead-lock can happen.
+        2. JITData::m_calleeSaveRegisters is filled only for DFG and FTL CodeBlock. And DFG and FTL code accesses these field after initializing properly. For Baseline JIT
+           compiler case, only thing we should do is that JITData should say m_calleeSaveRegisters is nullptr and it won't be filled for this CodeBlock.
+
+        Instead of guarding CodeBlock::calleeSaveRegisters() function with CodeBlock::m_lock, this patch inserts WTF::storeStoreFence when filling m_jitData. This ensures that
+        JITData::m_calleeSaveRegisters is initialized with nullptr when this JITData pointer is exposed to concurrent Baseline JIT compiler thread.
+
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::ensureJITDataSlow):
+
+2019-09-10  Yusuke Suzuki  <ysuzuki@apple.com>
+
         [JSC] ResultType implementation is wrong for bit ops, and ends up making ArithDiv take the DFG Int32 fast path even if Baseline constantly produces Double result
         https://bugs.webkit.org/show_bug.cgi?id=198253
 
         [JSC] ResultType implementation is wrong for bit ops, and ends up making ArithDiv take the DFG Int32 fast path even if Baseline constantly produces Double result
         https://bugs.webkit.org/show_bug.cgi?id=198253
 
index f18226b..1382f69 100644 (file)
@@ -1344,7 +1344,11 @@ void CodeBlock::finalizeLLIntInlineCaches()
 CodeBlock::JITData& CodeBlock::ensureJITDataSlow(const ConcurrentJSLocker&)
 {
     ASSERT(!m_jitData);
 CodeBlock::JITData& CodeBlock::ensureJITDataSlow(const ConcurrentJSLocker&)
 {
     ASSERT(!m_jitData);
-    m_jitData = makeUnique<JITData>();
+    auto jitData = makeUnique<JITData>();
+    // calleeSaveRegisters() can access m_jitData without taking a lock from Baseline JIT. This is OK since JITData::m_calleeSaveRegisters is filled in DFG and FTL CodeBlocks.
+    // But we should not see garbage pointer in that case. We ensure JITData::m_calleeSaveRegisters is initialized as nullptr before exposing it to BaselineJIT by store-store-fence.
+    WTF::storeStoreFence();
+    m_jitData = WTFMove(jitData);
     return *m_jitData;
 }
 
     return *m_jitData;
 }