Bug 178298 – JSRunLoopTimer: reduce likely race when used improperly

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

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.

[JF Bastien]

<rdar://problem/32899816>

[JF Bastien]

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.

[Build Bot]

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.

[Saam Barati]

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

[JF Bastien]

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.

[Build Bot]

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.

[Saam Barati]

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.

[JF Bastien]

(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.

[Filip Pizlo]

Comment on attachment 323761 [details]
patch

LGTM too

[JF Bastien]

Comment on attachment 323761 [details]
patch

Talked to pizlo, committing to trunk.

[WebKit Commit Bot]

Comment on attachment 323761 [details]
patch

Clearing flags on attachment: 323761

Committed r223409: <https://trac.webkit.org/changeset/223409>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.