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.
<rdar://problem/32899816>
Created attachment 323757 [details] patch Just to confirm, the assembly now looks like this: __ZN3JSC14JSRunLoopTimer12timerDidFireEv: __ZN3JSC14JSRunLoopTimer12timerDidFireEv: sub sp, sp, #0x30 sub sp, sp, #0x40 stp x22, x21, [sp, #0x10] stp x20, x19, [sp, #0x10] stp x20, x19, [sp, #0x20] stp x29, x30, [sp, #0x20] stp x29, x30, [sp, #0x30] add x29, sp, #0x20 add x29, sp, #0x30 mov x20, x0 mov x21, x0 ldr x0, [x20, #0x18] ldr x19, [x21, #0x18] cbz x19, 0x??? mov x0, x19 bl 0x??? bl 0x??? ldr x0, [x20, #0x18] ldr x19, [x0, #0x20] ldr x20, [x19, #0x20] cbz x19, 0x??? cbz x20, 0x??? ldaxr w8, [x19] ldaxr w8, [x20] The main safeguard is the acquisition of m_apiLock, though.
Attachment 323757 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:11: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:57: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 323757 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=323757&action=review It doesn't seem like we actually solve the bug here. Your code implies we may end up reading a field even after it dies. What if we used ref counting to ref the timer when it has a timer scheduled? > Source/JavaScriptCore/ChangeLog:20 > + `this` and turns a nullptr deref into âjustâ a use-after-free. remove unicode > Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:52 > + std::lock_guard<JSLock> lock(*apiLock); why not holdLock? > Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:58 > + JSLockHolder locker(vm.get()); Why do we need this if we're holding it up there? > Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:94 > + std::lock_guard<JSLock> lock(*apiLock); ditto w.r.t holdLock
Created attachment 323761 [details] patch Adress in-person comments from Andy. > It doesn't seem like we actually solve the bug here. Your code implies we > may end up reading a field even after it dies. What if we used ref counting > to ref the timer when it has a timer scheduled? I added a comment to that effect in the ChangeLog. I can explore refcounting separately, but I don't think it fits the scope of a small and targeted quick fix. At the end of the day we can try to be as robust as we want, but if the API is misused we'll be in trouble anyways, no? This seems like an easy way to avoid the simplest issues without being invasive. > > Source/JavaScriptCore/ChangeLog:20 > > + `this` and turns a nullptr deref into âjustâ a use-after-free. > > remove unicode Done. > > Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:52 > > + std::lock_guard<JSLock> lock(*apiLock); > > why not holdLock? lock_guard is used 2:1 in WebKit. Seems weird to duplicate a simple idiomatic thing from C++11 that does exactly the same thing. > > Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:58 > > + JSLockHolder locker(vm.get()); > > Why do we need this if we're holding it up there? You're right, I forgot those were the same! Gone.
Attachment 323761 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:11: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
I'm not a fan, but I won't stop you from landing. My reason for wanting holdLock is it's more idiomatic JSC code.
(In reply to Saam Barati from comment #7) > I'm not a fan, but I won't stop you from landing. Agreed, I'm not a fan either, but it seems like the simplest fix for a frequent crash. Will hold off committing until I hear from Divya.
Comment on attachment 323761 [details] patch LGTM too
Comment on attachment 323761 [details] patch Talked to pizlo, committing to trunk.
Comment on attachment 323761 [details] patch Clearing flags on attachment: 323761 Committed r223409: <https://trac.webkit.org/changeset/223409>
All reviewed patches have been landed. Closing bug.