Bug 211201 – Freezing of Gigacage and JSC Configs should be thread safe.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

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>

[Mark Lam]

Created attachment 397992 [details]
proposed patch.

[Yusuke Suzuki]

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.

[Mark Lam]

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.

[Yusuke Suzuki]

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.

[Mark Lam]

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>.