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>
Created attachment 397992 [details] proposed patch.
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 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 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.
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>.