Bug 178298

Summary: JSRunLoopTimer: reduce likely race when used improperly
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, buildbot, commit-queue, fpizlo, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch none

Description JF Bastien 2017-10-13 15:29:09 PDT
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.
Comment 1 JF Bastien 2017-10-13 15:29:40 PDT
<rdar://problem/32899816>
Comment 2 JF Bastien 2017-10-13 15:35:03 PDT
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.
Comment 3 Build Bot 2017-10-13 15:36:57 PDT
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 4 Saam Barati 2017-10-13 15:43:21 PDT
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
Comment 5 JF Bastien 2017-10-13 16:05:59 PDT
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.
Comment 6 Build Bot 2017-10-13 16:07:11 PDT
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.
Comment 7 Saam Barati 2017-10-13 16:07:40 PDT
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.
Comment 8 JF Bastien 2017-10-13 16:32:28 PDT
(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 9 Filip Pizlo 2017-10-16 08:49:31 PDT
Comment on attachment 323761 [details]
patch

LGTM too
Comment 10 JF Bastien 2017-10-16 08:51:07 PDT
Comment on attachment 323761 [details]
patch

Talked to pizlo, committing to trunk.
Comment 11 WebKit Commit Bot 2017-10-16 09:19:32 PDT
Comment on attachment 323761 [details]
patch

Clearing flags on attachment: 323761

Committed r223409: <https://trac.webkit.org/changeset/223409>
Comment 12 WebKit Commit Bot 2017-10-16 09:19:34 PDT
All reviewed patches have been landed.  Closing bug.