Title: [223670] branches/safari-604-branch/Source/_javascript_Core
- Revision
- 223670
- Author
- [email protected]
- Date
- 2017-10-18 22:14:44 -0700 (Wed, 18 Oct 2017)
Log Message
Cherry-pick r223409. rdar://problem/34792148
Modified Paths
Diff
Modified: branches/safari-604-branch/Source/_javascript_Core/ChangeLog (223669 => 223670)
--- branches/safari-604-branch/Source/_javascript_Core/ChangeLog 2017-10-19 05:14:42 UTC (rev 223669)
+++ branches/safari-604-branch/Source/_javascript_Core/ChangeLog 2017-10-19 05:14:44 UTC (rev 223670)
@@ -1,5 +1,43 @@
2017-10-18 Jason Marcell <[email protected]>
+ Cherry-pick r223409. rdar://problem/34792148
+
+ 2017-10-16 JF Bastien <[email protected]>
+
+ 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-18 Jason Marcell <[email protected]>
+
Cherry-pick r223614. rdar://problem/34920288
2017-10-18 Mark Lam <[email protected]>
Modified: branches/safari-604-branch/Source/_javascript_Core/runtime/JSRunLoopTimer.cpp (223669 => 223670)
--- branches/safari-604-branch/Source/_javascript_Core/runtime/JSRunLoopTimer.cpp 2017-10-19 05:14:42 UTC (rev 223669)
+++ branches/safari-604-branch/Source/_javascript_Core/runtime/JSRunLoopTimer.cpp 2017-10-19 05:14:44 UTC (rev 223670)
@@ -40,6 +40,8 @@
#include <wtf/glib/RunLoopSourcePriority.h>
#endif
+#include <mutex>
+
namespace JSC {
const Seconds JSRunLoopTimer::s_decade { 60 * 60 * 24 * 365 * 10 };
@@ -46,21 +48,20 @@
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 @@
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)
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
