JSRunLoopTimer: reduce likely race when used improperly

https://bugs.webkit.org/show_bug.cgi?id=178298
<rdar://problem/32899816>

Reviewed by Saam Barati.

If an API user sets a timer on JSRunLoopTimer, and then racily
destroys the JSRunLoopTimer while the timer is firing then it's
possible for timerDidFire to cause a use-after-free and / or crash
because e.g. m_apiLock becomes a nullptr while timerDidFire is
executing. That results from an invalid use of JSRunLoopTimer, but
we should try to be more resilient for that type of misuse because
it's not necessarily easy to catch by inspection.

With this change the only remaining race is if the timer fires,
and then only timerDidFire's prologue executes, but not the load
of the m_apiLock pointer from `this`. It's a much smaller race.

Separately, I'll reach out to API users who are seemingly misusing
the API.

* runtime/JSRunLoopTimer.cpp:
(JSC::JSRunLoopTimer::timerDidFire): put m_apiLock on the stack,
and checks for nullptr. This prevents loading it twice off of
`this` and turns a nullptr deref into "just" a use-after-free.
(JSC::JSRunLoopTimer::~JSRunLoopTimer): acquire m_apiLock before
calling m_vm->unregisterRunLoopTimer(this), which in turn does
CFRunLoopRemoveTimer / CFRunLoopTimerInvalidate. This prevents
timerDidFire from doing much while the timers are un-registered.
~JSRunLoopTimer also needs to set m_apiLock to nullptr before
releasing the lock, so it needs its own local copy.


Canonical link: https://commits.webkit.org/194554@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@223409 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,37 @@
+2017-10-16  JF Bastien  <jfbastien@apple.com>
+
+        JSRunLoopTimer: reduce likely race when used improperly
+        https://bugs.webkit.org/show_bug.cgi?id=178298
+        <rdar://problem/32899816>
+
+        Reviewed by Saam Barati.
+
+        If an API user sets a timer on JSRunLoopTimer, and then racily
+        destroys the JSRunLoopTimer while the timer is firing then it's
+        possible for timerDidFire to cause a use-after-free and / or crash
+        because e.g. m_apiLock becomes a nullptr while timerDidFire is
+        executing. That results from an invalid use of JSRunLoopTimer, but
+        we should try to be more resilient for that type of misuse because
+        it's not necessarily easy to catch by inspection.
+
+        With this change the only remaining race is if the timer fires,
+        and then only timerDidFire's prologue executes, but not the load
+        of the m_apiLock pointer from `this`. It's a much smaller race.
+
+        Separately, I'll reach out to API users who are seemingly misusing
+        the API.
+
+        * runtime/JSRunLoopTimer.cpp:
+        (JSC::JSRunLoopTimer::timerDidFire): put m_apiLock on the stack,
+        and checks for nullptr. This prevents loading it twice off of
+        `this` and turns a nullptr deref into "just" a use-after-free.
+        (JSC::JSRunLoopTimer::~JSRunLoopTimer): acquire m_apiLock before
+        calling m_vm->unregisterRunLoopTimer(this), which in turn does
+        CFRunLoopRemoveTimer / CFRunLoopTimerInvalidate. This prevents
+        timerDidFire from doing much while the timers are un-registered.
+        ~JSRunLoopTimer also needs to set m_apiLock to nullptr before
+        releasing the lock, so it needs its own local copy.
+
 2017-10-15  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] Perform module specifier validation at parsing time

Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp

@@ -40,27 +40,28 @@
 #include <wtf/glib/RunLoopSourcePriority.h>
 #endif
 
+#include <mutex>
+
 namespace JSC {
 
 const Seconds JSRunLoopTimer::s_decade { 60 * 60 * 24 * 365 * 10 };
 
 void JSRunLoopTimer::timerDidFire()
 {
-    m_apiLock->lock();
+    JSLock* apiLock = m_apiLock.get();
+    if (!apiLock) {
+        // Likely a buggy usage: the timer fired while JSRunLoopTimer was being destroyed.
+        return;
+    }
 
-    RefPtr<VM> vm = m_apiLock->vm();
+    std::lock_guard<JSLock> lock(*apiLock);
+    RefPtr<VM> vm = apiLock->vm();
     if (!vm) {
         // The VM has been destroyed, so we should just give up.
-        m_apiLock->unlock();
         return;
     }
 
-    {
-        JSLockHolder locker(vm.get());
-        doWork();
-    }
-
-    m_apiLock->unlock();
+    doWork();
 }
 
 #if USE(CF)
@@ -92,7 +93,10 @@ void JSRunLoopTimer::setRunLoop(CFRunLoopRef runLoop)
 
 JSRunLoopTimer::~JSRunLoopTimer()
 {
+    JSLock* apiLock = m_apiLock.get();
+    std::lock_guard<JSLock> lock(*apiLock);
     m_vm->unregisterRunLoopTimer(this);
+    m_apiLock = nullptr;
 }
 
 void JSRunLoopTimer::timerDidFireCallback(CFRunLoopTimerRef, void* contextPtr)