Changeset 223409 in webkit


Ignore:
Timestamp:
Oct 16, 2017 9:19:29 AM (6 years ago)
Author:
jfbastien@apple.com
Message:

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.

Location:
trunk/Source/JavaScriptCore
Files:
2 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/JavaScriptCore/ChangeLog

    r223331 r223409  
     12017-10-16  JF Bastien  <jfbastien@apple.com>
     2
     3        JSRunLoopTimer: reduce likely race when used improperly
     4        https://bugs.webkit.org/show_bug.cgi?id=178298
     5        <rdar://problem/32899816>
     6
     7        Reviewed by Saam Barati.
     8
     9        If an API user sets a timer on JSRunLoopTimer, and then racily
     10        destroys the JSRunLoopTimer while the timer is firing then it's
     11        possible for timerDidFire to cause a use-after-free and / or crash
     12        because e.g. m_apiLock becomes a nullptr while timerDidFire is
     13        executing. That results from an invalid use of JSRunLoopTimer, but
     14        we should try to be more resilient for that type of misuse because
     15        it's not necessarily easy to catch by inspection.
     16
     17        With this change the only remaining race is if the timer fires,
     18        and then only timerDidFire's prologue executes, but not the load
     19        of the m_apiLock pointer from `this`. It's a much smaller race.
     20
     21        Separately, I'll reach out to API users who are seemingly misusing
     22        the API.
     23
     24        * runtime/JSRunLoopTimer.cpp:
     25        (JSC::JSRunLoopTimer::timerDidFire): put m_apiLock on the stack,
     26        and checks for nullptr. This prevents loading it twice off of
     27        `this` and turns a nullptr deref into "just" a use-after-free.
     28        (JSC::JSRunLoopTimer::~JSRunLoopTimer): acquire m_apiLock before
     29        calling m_vm->unregisterRunLoopTimer(this), which in turn does
     30        CFRunLoopRemoveTimer / CFRunLoopTimerInvalidate. This prevents
     31        timerDidFire from doing much while the timers are un-registered.
     32        ~JSRunLoopTimer also needs to set m_apiLock to nullptr before
     33        releasing the lock, so it needs its own local copy.
     34
    1352017-10-15  Yusuke Suzuki  <utatane.tea@gmail.com>
    236
  • trunk/Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp

    r218381 r223409  
    4141#endif
    4242
     43#include <mutex>
     44
    4345namespace JSC {
    4446
     
    4749void JSRunLoopTimer::timerDidFire()
    4850{
    49     m_apiLock->lock();
    50 
    51     RefPtr<VM> vm = m_apiLock->vm();
    52     if (!vm) {
    53         // The VM has been destroyed, so we should just give up.
    54         m_apiLock->unlock();
     51    JSLock* apiLock = m_apiLock.get();
     52    if (!apiLock) {
     53        // Likely a buggy usage: the timer fired while JSRunLoopTimer was being destroyed.
    5554        return;
    5655    }
    5756
    58     {
    59         JSLockHolder locker(vm.get());
    60         doWork();
     57    std::lock_guard<JSLock> lock(*apiLock);
     58    RefPtr<VM> vm = apiLock->vm();
     59    if (!vm) {
     60        // The VM has been destroyed, so we should just give up.
     61        return;
    6162    }
    6263
    63     m_apiLock->unlock();
     64    doWork();
    6465}
    6566
     
    9394JSRunLoopTimer::~JSRunLoopTimer()
    9495{
     96    JSLock* apiLock = m_apiLock.get();
     97    std::lock_guard<JSLock> lock(*apiLock);
    9598    m_vm->unregisterRunLoopTimer(this);
     99    m_apiLock = nullptr;
    96100}
    97101
Note: See TracChangeset for help on using the changeset viewer.