Changeset 260913 in webkit

Timestamp:

Apr 29, 2020, 1:39:03 PM (5 years ago)

Author:

mark.lam@apple.com

Message:

Freezing of Gigacage and JSC Configs should be thread safe.
​https://bugs.webkit.org/show_bug.cgi?id=211201
<rdar://problem/62597619>

Reviewed by Yusuke Suzuki.

Source/bmalloc:

• bmalloc/Gigacage.cpp:

(Gigacage::bmalloc::permanentlyFreezeGigacageConfig):

Source/JavaScriptCore:

If a client creates multiple VM instances in different threads concurrently, the
following race can occur:

Config::permanentlyFreeze() contains the following code:

if (!g_jscConfig.isPermanentlyFrozen) Point P1

g_jscConfig.isPermanentlyFrozen = true; Point P2

Let's say there are 2 threads T1 and T2.

1. T1 creates a VM and gets to point P1, and sees that g_jscConfig.isPermanentlyFrozen is not set. T1 is about to execute P2 when it gets pre-empted.

2. T2 creates a VM and gets to point P1, and sees that g_jscConfig.isPermanentlyFrozen is not set. T2 proceeds to point P2 and sets g_jscConfig.isPermanentlyFrozen to true. T2 goes on to freeze the Config and makes it not writable.

3. T1 gets to run again, and proceeds to point P2. T1 tries to set g_jscConfig.isPermanentlyFrozen to true. But because the Config has been frozen against writes, the write to g_jscConfig.isPermanentlyFrozen results in a crash.

This is a classic TOCTOU bug. The fix is simply to ensure that only one thread
can enter Config::permanentlyFreeze() at a time.

Ditto for Gigacage::permanentlyFreezeGigacageConfig().

• runtime/JSCConfig.cpp:

(JSC::Config::permanentlyFreeze):

Location:

trunk/Source

Files:

• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/runtime/JSCConfig.cpp (modified) (3 diffs)
• bmalloc/ChangeLog (modified) (1 diff)
• bmalloc/bmalloc/Gigacage.cpp (modified) (4 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-04-29  Mark Lam  <mark.lam@apple.com>
+
+        Freezing of Gigacage and JSC Configs should be thread safe.
+        https://bugs.webkit.org/show_bug.cgi?id=211201
+        <rdar://problem/62597619>
+
+        Reviewed by Yusuke Suzuki.
+
+        If a client creates multiple VM instances in different threads concurrently, the
+        following race can occur:
+
+        Config::permanentlyFreeze() contains the following code:
+
+            if (!g_jscConfig.isPermanentlyFrozen)         // Point P1
+                g_jscConfig.isPermanentlyFrozen = true;   // Point P2
+
+        Let's say there are 2 threads T1 and T2.
+
+        1. T1 creates a VM and gets to point P1, and sees that g_jscConfig.isPermanentlyFrozen is not set.
+           T1 is about to execute P2 when it gets pre-empted.
+
+        2. T2 creates a VM and gets to point P1, and sees that g_jscConfig.isPermanentlyFrozen is not set.
+           T2 proceeds to point P2 and sets g_jscConfig.isPermanentlyFrozen to true.
+           T2 goes on to freeze the Config and makes it not writable.
+
+        3. T1 gets to run again, and proceeds to point P2.
+           T1 tries to set g_jscConfig.isPermanentlyFrozen to true.
+           But because the Config has been frozen against writes, the write to
+           g_jscConfig.isPermanentlyFrozen results in a crash.
+
+        This is a classic TOCTOU bug.  The fix is simply to ensure that only one thread
+        can enter Config::permanentlyFreeze() at a time.
+
+        Ditto for Gigacage::permanentlyFreezeGigacageConfig().
+
+        * runtime/JSCConfig.cpp:
+        (JSC::Config::permanentlyFreeze):
+
 2020-04-29  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/runtime/JSCConfig.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2019-2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -27 +27 @@
 #include "JSCConfig.h"
 
+#include <wtf/Lock.h>
 #include <wtf/ResourceUsage.h>
 #include <wtf/StdLibExtras.h>
@@ -54 +55 @@
 void Config::permanentlyFreeze()
 {
+    static Lock configLock;
+    auto locker = holdLock(configLock);
+
     RELEASE_ASSERT(roundUpToMultipleOf(pageSize(), ConfigSizeToProtect) == ConfigSizeToProtect);
 

trunk/Source/bmalloc/ChangeLog

@@ +1 @@
+2020-04-29  Mark Lam  <mark.lam@apple.com>
+
+        Freezing of Gigacage and JSC Configs should be thread safe.
+        https://bugs.webkit.org/show_bug.cgi?id=211201
+        <rdar://problem/62597619>
+
+        Reviewed by Yusuke Suzuki.
+
+        * bmalloc/Gigacage.cpp:
+        (Gigacage::bmalloc::permanentlyFreezeGigacageConfig):
+
 2020-04-25  Darin Adler  <darin@apple.com>
 

trunk/Source/bmalloc/bmalloc/Gigacage.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2017-2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2017-2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -28 +28 @@
 #include "CryptoRandom.h"
 #include "Environment.h"
+#include "Mutex.h"
 #include "ProcessCheck.h"
 #include "StaticPerProcess.h"
@@ -34 +35 @@
 #include "bmalloc.h"
 #include <cstdio>
-#include <mutex>
 
 #if BOS(DARWIN)
@@ -118 +118 @@
 static void permanentlyFreezeGigacageConfig()
 {
+    static Mutex configLock;
+    LockHolder locking(configLock);
+
     if (!g_gigacageConfig.isPermanentlyFrozen) {
         unfreezeGigacageConfig();