Bug 195613 – REGRESSION (iOS 12.2): Webpage using CoffeeScript crashes

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

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.

[Radar WebKit Bug Importer]

<rdar://problem/48811099>

[Michael Saboff]

I have a reduced repro case.  Working on a fix now.

[Michael Saboff]

Testing a fix now.

[Michael Saboff]

Created attachment 364481 [details]
Patch

[Mark Lam]

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.

[Michael Saboff]

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

[Michael Saboff]

Committed r242838: <https://trac.webkit.org/changeset/242838>

[Ryosuke Niwa]

Are there any websites that are affected by this crash?

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

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?

[Ryosuke Niwa]

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

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

[Ryosuke Niwa]

Thanks for the patience. iOS 12.3 Beta 2 (16F5129d) should have this fix.