Bug 211201 - Freezing of Gigacage and JSC Configs should be thread safe.
Summary: Freezing of Gigacage and JSC Configs should be thread safe.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-29 13:16 PDT by Mark Lam
Modified: 2020-04-29 13:40 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch. (4.66 KB, patch)
2020-04-29 13:25 PDT, Mark Lam
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2020-04-29 13:16:19 PDT
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 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().

<rdar://problem/62597619>
Comment 1 Mark Lam 2020-04-29 13:25:07 PDT
Created attachment 397992 [details]
proposed patch.
Comment 2 Yusuke Suzuki 2020-04-29 13:27:50 PDT
Comment on attachment 397992 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=397992&action=review

> Source/JavaScriptCore/runtime/JSCConfig.cpp:58
> +    static Lock configLock;
> +    LockHolder locker(configLock);

Why not using std::call_once?

> Source/bmalloc/bmalloc/Gigacage.cpp:122
> +    static Mutex configLock;
> +    LockHolder locking(configLock);
> +

Ditto.
Comment 3 Mark Lam 2020-04-29 13:32:33 PDT
Comment on attachment 397992 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=397992&action=review

>> Source/JavaScriptCore/runtime/JSCConfig.cpp:58
>> +    LockHolder locker(configLock);
> 
> Why not using std::call_once?

Because I want each call to permanentlyFreeze() to go thru the vm_protect and affirm that the setting is as we expect it.  This is why there's a RELEASE_ASSERT on the result.
Comment 4 Yusuke Suzuki 2020-04-29 13:32:54 PDT
Comment on attachment 397992 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=397992&action=review

Discussed with Mark. We actually want to call this function multiple times, so std::call_once is not suitable. But while executing this function, this function breaks an invariant temporarily which is required by the call from the other thread. So guarding this with Lock is the appropriate way.

>> Source/JavaScriptCore/runtime/JSCConfig.cpp:58
>> +    LockHolder locker(configLock);
> 
> Why not using std::call_once?

Use `auto locker = holdLock(configLock)` in WTF.
Comment 5 Mark Lam 2020-04-29 13:40:23 PDT
Thanks for the review.

(In reply to Yusuke Suzuki from comment #4)
> Use `auto locker = holdLock(configLock)` in WTF.

Applied.

Landed in r260913: <http://trac.webkit.org/r260913>.