Changeset 249706 in webkit

Timestamp:

Sep 9, 2019, 10:03:13 PM (7 years ago)

Author:

ysuzuki@apple.com

Message:

[JSC] CodeBlock::m_constantRegisters should be guarded by ConcurrentJSLock when Vector reallocate memory
​https://bugs.webkit.org/show_bug.cgi?id=201622

Reviewed by Mark Lam.

CodeBlock::visitChildren takes ConcurrentJSLock while iterating m_constantRegisters, some of the places reallocate
this Vector without taking a lock. If a Vector memory is reallocated while iterating it in concurrent collector,
the concurrent collector can see a garbage. This patch guards m_constantRegisters reallocation with ConcurrentJSLock.

• bytecode/CodeBlock.cpp:

(JSC::CodeBlock::finishCreation):
(JSC::CodeBlock::setConstantRegisters):

• bytecode/CodeBlock.h:

(JSC::CodeBlock::addConstant):
(JSC::CodeBlock::addConstantLazily):

• dfg/DFGDesiredWatchpoints.cpp:

(JSC::DFG::ArrayBufferViewWatchpointAdaptor::add):
(JSC::DFG::SymbolTableAdaptor::add):
(JSC::DFG::FunctionExecutableAdaptor::add):

• dfg/DFGGraph.cpp:

(JSC::DFG::Graph::registerFrozenValues):

• dfg/DFGJITFinalizer.cpp:

(JSC::DFG::JITFinalizer::finalizeCommon):

• dfg/DFGLazyJSValue.cpp:

(JSC::DFG::LazyJSValue::emit const):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• bytecode/CodeBlock.cpp (modified) (2 diffs)
• bytecode/CodeBlock.h (modified) (2 diffs)
• dfg/DFGDesiredWatchpoints.cpp (modified) (3 diffs)
• dfg/DFGGraph.cpp (modified) (2 diffs)
• dfg/DFGJITFinalizer.cpp (modified) (1 diff)
• dfg/DFGLazyJSValue.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-09-09  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] CodeBlock::m_constantRegisters should be guarded by ConcurrentJSLock when Vector reallocate memory
+        https://bugs.webkit.org/show_bug.cgi?id=201622
+
+        Reviewed by Mark Lam.
+
+        CodeBlock::visitChildren takes ConcurrentJSLock while iterating m_constantRegisters, some of the places reallocate
+        this Vector without taking a lock. If a Vector memory is reallocated while iterating it in concurrent collector,
+        the concurrent collector can see a garbage. This patch guards m_constantRegisters reallocation with ConcurrentJSLock.
+
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::finishCreation):
+        (JSC::CodeBlock::setConstantRegisters):
+        * bytecode/CodeBlock.h:
+        (JSC::CodeBlock::addConstant):
+        (JSC::CodeBlock::addConstantLazily):
+        * dfg/DFGDesiredWatchpoints.cpp:
+        (JSC::DFG::ArrayBufferViewWatchpointAdaptor::add):
+        (JSC::DFG::SymbolTableAdaptor::add):
+        (JSC::DFG::FunctionExecutableAdaptor::add):
+        * dfg/DFGGraph.cpp:
+        (JSC::DFG::Graph::registerFrozenValues):
+        * dfg/DFGJITFinalizer.cpp:
+        (JSC::DFG::JITFinalizer::finalizeCommon):
+        * dfg/DFGLazyJSValue.cpp:
+        (JSC::DFG::LazyJSValue::emit const):
+
 2019-09-09  Robin Morisset  <rmorisset@apple.com>
 

trunk/Source/JavaScriptCore/bytecode/CodeBlock.cpp

@@ -597 +597 @@
                     // Keep the linked module environment strongly referenced.
                     if (stronglyReferencedModuleEnvironments.add(jsCast<JSModuleEnvironment*>(op.lexicalEnvironment)).isNewEntry)
-                        addConstant(op.lexicalEnvironment);
+                        addConstant(ConcurrentJSLocker(m_lock), op.lexicalEnvironment);
                     metadata.m_lexicalEnvironment.set(vm, this, op.lexicalEnvironment);
                 } else
@@ -900 +900 @@
     ASSERT(constants.size() == constantsSourceCodeRepresentation.size());
     size_t count = constants.size();
-    m_constantRegisters.resizeToFit(count);
+    {
+        ConcurrentJSLocker locker(m_lock);
+        m_constantRegisters.resizeToFit(count);
+    }
     for (size_t i = 0; i < count; i++) {
         JSValue constant = constants[i].get();

trunk/Source/JavaScriptCore/bytecode/CodeBlock.h

@@ -547 +547 @@
     Vector<WriteBarrier<Unknown>>& constants() { return m_constantRegisters; }
     Vector<SourceCodeRepresentation>& constantsSourceCodeRepresentation() { return m_constantsSourceCodeRepresentation; }
-    unsigned addConstant(JSValue v)
+    unsigned addConstant(const ConcurrentJSLocker&, JSValue v)
     {
         unsigned result = m_constantRegisters.size();
@@ -556 +556 @@
     }
 
-    unsigned addConstantLazily()
+    unsigned addConstantLazily(const ConcurrentJSLocker&)
     {
         unsigned result = m_constantRegisters.size();

trunk/Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.cpp

@@ -44 +44 @@
         ArrayBufferNeuteringWatchpointSet::create(vm);
     neuteringWatchpoint->set().add(watchpoint);
-    codeBlock->addConstant(neuteringWatchpoint);
+    codeBlock->addConstant(ConcurrentJSLocker(codeBlock->m_lock), neuteringWatchpoint);
     // FIXME: We don't need to set this watchpoint at all for shared buffers.
     // https://bugs.webkit.org/show_bug.cgi?id=164108
@@ -53 +53 @@
     CodeBlock* codeBlock, SymbolTable* symbolTable, CommonData& common)
 {
-    codeBlock->addConstant(symbolTable); // For common users, it doesn't really matter if it's weak or not. If references to it go away, we go away, too.
+    codeBlock->addConstant(ConcurrentJSLocker(codeBlock->m_lock), symbolTable); // For common users, it doesn't really matter if it's weak or not. If references to it go away, we go away, too.
     symbolTable->singleton().add(common.watchpoints.add(codeBlock));
 }
@@ -60 +60 @@
     CodeBlock* codeBlock, FunctionExecutable* executable, CommonData& common)
 {
-    codeBlock->addConstant(executable); // For common users, it doesn't really matter if it's weak or not. If references to it go away, we go away, too.
+    codeBlock->addConstant(ConcurrentJSLocker(codeBlock->m_lock), executable); // For common users, it doesn't really matter if it's weak or not. If references to it go away, we go away, too.
     executable->singleton().add(common.watchpoints.add(codeBlock));
 }

trunk/Source/JavaScriptCore/dfg/DFGGraph.cpp

@@ -1406 +1406 @@
 void Graph::registerFrozenValues()
 {
+    ConcurrentJSLocker locker(m_codeBlock->m_lock);
     m_codeBlock->constants().shrink(0);
     m_codeBlock->constantsSourceCodeRepresentation().resize(0);
@@ -1421 +1422 @@
         }
         case StrongValue: {
-            unsigned constantIndex = m_codeBlock->addConstantLazily();
+            unsigned constantIndex = m_codeBlock->addConstantLazily(locker);
             // We already have a barrier on the code block.
             m_codeBlock->constants()[constantIndex].setWithoutWriteBarrier(value->value());

trunk/Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp

@@ -83 +83 @@
 {
     // Some JIT finalizers may have added more constants. Shrink-to-fit those things now.
-    m_plan.codeBlock()->constants().shrinkToFit();
-    m_plan.codeBlock()->constantsSourceCodeRepresentation().shrinkToFit();
+    {
+        ConcurrentJSLocker locker(m_plan.codeBlock()->m_lock);
+        m_plan.codeBlock()->constants().shrinkToFit();
+        m_plan.codeBlock()->constantsSourceCodeRepresentation().shrinkToFit();
+    }
 
 #if ENABLE(FTL_JIT)

trunk/Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp

@@ -255 +255 @@
             RELEASE_ASSERT(realValue.isCell());
 
-            codeBlock->addConstant(realValue);
+            codeBlock->addConstant(ConcurrentJSLocker(codeBlock->m_lock), realValue);
             
             if (thisValue.m_kind == NewStringImpl)