Changeset 223409 in webkit
- Timestamp:
- Oct 16, 2017 9:19:29 AM (6 years ago)
- Location:
- trunk/Source/JavaScriptCore
- Files:
-
- 2 edited
Legend:
- Unmodified
- Added
- Removed
-
trunk/Source/JavaScriptCore/ChangeLog
r223331 r223409 1 2017-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 1 35 2017-10-15 Yusuke Suzuki <utatane.tea@gmail.com> 2 36 -
trunk/Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp
r218381 r223409 41 41 #endif 42 42 43 #include <mutex> 44 43 45 namespace JSC { 44 46 … … 47 49 void JSRunLoopTimer::timerDidFire() 48 50 { 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. 55 54 return; 56 55 } 57 56 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; 61 62 } 62 63 63 m_apiLock->unlock();64 doWork(); 64 65 } 65 66 … … 93 94 JSRunLoopTimer::~JSRunLoopTimer() 94 95 { 96 JSLock* apiLock = m_apiLock.get(); 97 std::lock_guard<JSLock> lock(*apiLock); 95 98 m_vm->unregisterRunLoopTimer(this); 99 m_apiLock = nullptr; 96 100 } 97 101
Note: See TracChangeset
for help on using the changeset viewer.