Changeset 249518 in webkit

Timestamp:

Sep 4, 2019 7:52:46 PM (5 years ago)

Author:

ysuzuki@apple.com

Message:

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

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• tools/FunctionOverrides.cpp (modified) (4 diffs)
• tools/FunctionOverrides.h (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/tools/FunctionOverrides.cpp

@@ -103 +103 @@
 FunctionOverrides::FunctionOverrides(const char* overridesFileName)
 {
-    parseOverridesInFile(overridesFileName);
+    parseOverridesInFile(holdLock(m_lock), overridesFileName);
 }
 
@@ -109 +109 @@
 {
     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);
 }
 
@@ -152 +153 @@
     String sourceBodyString = sourceString.substring(sourceBodyStart);
 
-    auto it = overrides.m_entries.find(sourceBodyString);
-    if (it == overrides.m_entries.end())
-        return false;
-
-    initializeOverrideInfo(origCode, it->value, result);
+    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, newBody, result);
     return true;
 }
@@ -228 +234 @@
 }
 
-void FunctionOverrides::parseOverridesInFile(const char* fileName)
+void FunctionOverrides::parseOverridesInFile(const AbstractLocker&, const char* fileName)
 {
     if (!fileName)

trunk/Source/JavaScriptCore/tools/FunctionOverrides.h

@@ -28 +28 @@
 #include "SourceCode.h"
 #include <wtf/HashMap.h>
+#include <wtf/Lock.h>
 #include <wtf/text/WTFString.h>
 
@@ -57 +58 @@
 
 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;
 };