[JSC] FunctionOverrides should have a lock to ensure concurrent access to hash table...
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Sep 2019 02:52:46 +0000 (02:52 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Sep 2019 02:52:46 +0000 (02:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=201485

Reviewed by Tadeu Zagallo.

FunctionOverrides is a per-process singleton for registering overrides information. But we are accessing
it without taking a lock. If multiple threads with multiple VMs are accessing this concurrently, we have
a race issue like,

1. While one thread is adding overrides information,
2. Another thread is accessing this hash table.

This patch adds a lock to make sure that only one thread can access this registry.

* tools/FunctionOverrides.cpp:
(JSC::FunctionOverrides::FunctionOverrides):
(JSC::FunctionOverrides::reinstallOverrides):
(JSC::FunctionOverrides::initializeOverrideFor):
(JSC::FunctionOverrides::parseOverridesInFile):
* tools/FunctionOverrides.h:
(JSC::FunctionOverrides::clear):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/tools/FunctionOverrides.cpp
Source/JavaScriptCore/tools/FunctionOverrides.h

index 3c616fb..f1e5831 100644 (file)
@@ -1,5 +1,29 @@
 2019-09-04  Yusuke Suzuki  <ysuzuki@apple.com>
 
+        [JSC] FunctionOverrides should have a lock to ensure concurrent access to hash table does not happen
+        https://bugs.webkit.org/show_bug.cgi?id=201485
+
+        Reviewed by Tadeu Zagallo.
+
+        FunctionOverrides is a per-process singleton for registering overrides information. But we are accessing
+        it without taking a lock. If multiple threads with multiple VMs are accessing this concurrently, we have
+        a race issue like,
+
+        1. While one thread is adding overrides information,
+        2. Another thread is accessing this hash table.
+
+        This patch adds a lock to make sure that only one thread can access this registry.
+
+        * tools/FunctionOverrides.cpp:
+        (JSC::FunctionOverrides::FunctionOverrides):
+        (JSC::FunctionOverrides::reinstallOverrides):
+        (JSC::FunctionOverrides::initializeOverrideFor):
+        (JSC::FunctionOverrides::parseOverridesInFile):
+        * tools/FunctionOverrides.h:
+        (JSC::FunctionOverrides::clear):
+
+2019-09-04  Yusuke Suzuki  <ysuzuki@apple.com>
+
         [JSC] Make Promise implementation faster
         https://bugs.webkit.org/show_bug.cgi?id=200898
 
index c1154f5..fffa4a1 100644 (file)
@@ -102,15 +102,16 @@ FunctionOverrides& FunctionOverrides::overrides()
     
 FunctionOverrides::FunctionOverrides(const char* overridesFileName)
 {
-    parseOverridesInFile(overridesFileName);
+    parseOverridesInFile(holdLock(m_lock), overridesFileName);
 }
 
 void FunctionOverrides::reinstallOverrides()
 {
     FunctionOverrides& overrides = FunctionOverrides::overrides();
+    auto locker = holdLock(overrides.m_lock);
     const char* overridesFileName = Options::functionOverrides();
-    overrides.clear();
-    overrides.parseOverridesInFile(overridesFileName);
+    overrides.clear(locker);
+    overrides.parseOverridesInFile(locker, overridesFileName);
 }
 
 static void initializeOverrideInfo(const SourceCode& origCode, const String& newBody, FunctionOverrides::OverrideInfo& info)
@@ -151,11 +152,16 @@ bool FunctionOverrides::initializeOverrideFor(const SourceCode& origCode, Functi
         return false;
     String sourceBodyString = sourceString.substring(sourceBodyStart);
 
-    auto it = overrides.m_entries.find(sourceBodyString);
-    if (it == overrides.m_entries.end())
-        return false;
+    String newBody;
+    {
+        auto locker = holdLock(overrides.m_lock);
+        auto it = overrides.m_entries.find(sourceBodyString.isolatedCopy());
+        if (it == overrides.m_entries.end())
+            return false;
+        newBody = it->value.isolatedCopy();
+    }
 
-    initializeOverrideInfo(origCode, it->value, result);
+    initializeOverrideInfo(origCode, newBody, result);
     return true;
 }
 
@@ -227,7 +233,7 @@ static String parseClause(const char* keyword, size_t keywordLength, FILE* file,
     FAIL_WITH_ERROR(SYNTAX_ERROR, ("'", keyword, "' clause end delimiter '", delimiter, "' not found:\n", builder.toString(), "\n", "Are you missing a '}' before the delimiter?\n"));
 }
 
-void FunctionOverrides::parseOverridesInFile(const char* fileName)
+void FunctionOverrides::parseOverridesInFile(const AbstractLocker&, const char* fileName)
 {
     if (!fileName)
         return;
index 4a42bbc..8829670 100644 (file)
@@ -27,6 +27,7 @@
 
 #include "SourceCode.h"
 #include <wtf/HashMap.h>
+#include <wtf/Lock.h>
 #include <wtf/text/WTFString.h>
 
 namespace JSC {
@@ -56,10 +57,11 @@ public:
     JS_EXPORT_PRIVATE static void reinstallOverrides();
 
 private:
-    void parseOverridesInFile(const char* fileName);
-    void clear() { m_entries.clear(); }
+    void parseOverridesInFile(const AbstractLocker&, const char* fileName);
+    void clear(const AbstractLocker&) { m_entries.clear(); }
 
     HashMap<String, String> m_entries;
+    Lock m_lock;
 };
 
 } // namespace JSC