Bug 195613

Summary: REGRESSION (iOS 12.2): Webpage using CoffeeScript crashes
Product: WebKit Reporter: eelco
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Critical CC: ews-watchlist, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 12   
Hardware: iPhone / iPad   
OS: iOS 12   
Attachments:
Description Flags
Files demonstrating the crash
none
Patch mark.lam: review+

Description eelco 2019-03-12 02:35:54 PDT
Created attachment 364373 [details]
Files demonstrating the crash

On iOS 12.2 beta 4, using the CoffeeScript compiler* on a web page causes the page to crash.

This will happen for all but the most trivial CoffeeScript code.

To reproduce:
- Unzip attached `crash.zip`
- Host the files somewhere (`python -m SimpleHTTPServer`)
- Open the index.html on iOS 12.2 (either in the simulator or on a device)

Expected:
Page loads, console output is printed

Observed:
Page crashes on load.

*This might not happen with all version of the CoffeeScript compiler, I have not narrowed it down further.
Comment 1 Radar WebKit Bug Importer 2019-03-12 09:49:05 PDT
<rdar://problem/48811099>
Comment 2 Michael Saboff 2019-03-12 11:59:14 PDT
I have a reduced repro case.  Working on a fix now.
Comment 3 Michael Saboff 2019-03-12 15:29:39 PDT
Testing a fix now.
Comment 4 Michael Saboff 2019-03-12 17:34:34 PDT
Created attachment 364481 [details]
Patch
Comment 5 Mark Lam 2019-03-12 17:52:45 PDT
Comment on attachment 364481 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364481&action=review

r=me.

> Source/JavaScriptCore/ChangeLog:11
> +        The bug here is in Yarr JIT backreference matching code.  We are incorrectly
> +        using a checkedOffset correction when checking for length left in a string.
> +        In some cases, this allows us to go past the subject string's length.
> +        Removed these adjustments.

This could be misread as the use of checkedOffset being the issue.  The actual issue here is that we're adjusting patternTemp at all (which incidentally involves checkedOffset in the adjustment computation).  Can you reword this to be a bit clearer?  Thanks.
Comment 6 Michael Saboff 2019-03-12 18:06:23 PDT
(In reply to Mark Lam from comment #5)
> Comment on attachment 364481 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364481&action=review
> 
> r=me.
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        The bug here is in Yarr JIT backreference matching code.  We are incorrectly
> > +        using a checkedOffset correction when checking for length left in a string.
> > +        In some cases, this allows us to go past the subject string's length.
> > +        Removed these adjustments.
> 
> This could be misread as the use of checkedOffset being the issue.  The
> actual issue here is that we're adjusting patternTemp at all (which
> incidentally involves checkedOffset in the adjustment computation).  Can you
> reword this to be a bit clearer?  Thanks.

I changed this to:

    The bug here is in Yarr JIT backreference matching code.  We are incorrectly
    using a checkedOffset / inputPosition correction when checking for the available
    length left in a string.  It is improper to do these corrections as a backreference's
    match length is based on what was matched in the referenced capture group and not
    part of the checkedOffset and inputPosition computed when we compiled the RegExp.
    In some cases, the resulting incorrect calculation would allow us to go past
    the subject string's length.  Removed these adjustments.
Comment 7 Michael Saboff 2019-03-12 18:26:31 PDT
Committed r242838: <https://trac.webkit.org/changeset/242838>
Comment 8 Ryosuke Niwa 2019-03-12 22:57:54 PDT
Are there any websites that are affected by this crash?
Comment 9 eelco 2019-03-12 23:27:57 PDT
(In reply to Ryosuke Niwa from comment #8)
> Are there any websites that are affected by this crash?

Most (if not all) Framer prototypes on share.framerjs.com and framer.cloud, e.g. https://framer.cloud/IgLBR. I don’t exactly know how many there are (not all are public), but it’s a lot.

I don’t expect many “normal” websites out there to still use CoffeeScript, especially not in a way where it is compiled on the fly, but I’m sure they exist.
Comment 10 eelco 2019-03-19 09:36:52 PDT
I’m not sure how releases are cut for iOS, but given this is not in the latest beta, will this make the iOS 12.2 release?
Comment 11 Ryosuke Niwa 2019-03-19 13:55:10 PDT
(In reply to eelco from comment #9)
> (In reply to Ryosuke Niwa from comment #8)
> > Are there any websites that are affected by this crash?
> 
> Most (if not all) Framer prototypes on share.framerjs.com and framer.cloud,
> e.g. https://framer.cloud/IgLBR. I don’t exactly know how many there are
> (not all are public), but it’s a lot.
> 
> I don’t expect many “normal” websites out there to still use CoffeeScript,
> especially not in a way where it is compiled on the fly, but I’m sure they
> exist.

Thanks for the information! This helps us prioritize this fix.

(In reply to eelco from comment #10)
> I’m not sure how releases are cut for iOS, but given this is not in the
> latest beta, will this make the iOS 12.2 release?

Unfortunately, we can't comment on if or when a given feature or a bug fix will be included in a future Apple product.
Comment 12 eelco 2019-03-19 16:05:56 PDT
(In reply to Ryosuke Niwa from comment #11)
> (In reply to eelco from comment #10)
> > I’m not sure how releases are cut for iOS, but given this is not in the
> > latest beta, will this make the iOS 12.2 release?
> 
> Unfortunately, we can't comment on if or when a given feature or a bug fix
> will be included in a future Apple product.

I understand :) 

I’m looking at the releases and tags in the source code though and I see the first couple tags that have the fix are 607.2.1 and 608.1.9, but when I compare the latest iOS releases with the tags, it seems that they take a tag pretty much exactly 1 month before release. With an Apple event next week, I’m not feeling good about this fix making it in on time :(
Comment 13 Ryosuke Niwa 2019-04-12 20:55:42 PDT
Thanks for the patience. iOS 12.3 Beta 2 (16F5129d) should have this fix.