Closed Bug 1452619 Opened 6 years ago Closed 6 years ago

SyntaxError when creating literal RegExp in eval

Categories

(Core :: JavaScript Engine, defect, P1)

All
Windows 10
defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 59+ verified
firefox59 blocking verified
firefox60 + verified
firefox61 + verified

People

(Reporter: dukes.brian, Assigned: Waldo)

Details

Attachments

(4 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180404171943

Steps to reproduce:

The CMS we build websites on has an old script which is using `eval` to dynamically create a `RegExp`, using code like `eval('/~!~/g')` (however, the same issue occurs with any `RegExp`, regardless of content or flags).

An isolated version of this issue is at https://jsbin.com/lurazav/1/edit?js,console

That this issue can/should be worked around by using `new RegExp()` instead of `eval`


Actual results:

A `SyntaxError` is thrown, with the message "invalid regular expression flag ÿ"


Expected results:

A valid `RegExp` object should be returned from the `eval` call
I don't see SyntaxError by opening https://jsbin.com/lurazav/1/edit?js,console
on Firefox Developer Edition 60.0b10 (64-bit) with clean profile on macOS and windows 10.

is there any other steps required other than opening it?
does the issue happen on safe mode?
Flags: needinfo?(dukes.brian)
You can see this in the wild at a site like http://www.dnnsoftware.com/wiki

I can reproduce this bug on my PC with Windows 10 Enterprise, Version 1803, OS build 17133.1

This bug reproduces in both Firefox ESR and Firefox Dev Edition, and reproduces in safe mode.

I am not able to reproduce this bug in Chrome or Edge.

I am also not able to reproduce this bug via Browserstack in any browser (including any version of Firefox on any operating system).  Therefore, this may be related to a recent Windows update.
Flags: needinfo?(dukes.brian)
do you have other PC with same or similar configuration?
if so, is it reproducible with other PCs?

also, can you provide the following information?
  * is your OS 32bit or 64bit?
  * what's the exact version of your Firefox?
  * is your Firefox 32bit or 64bit?
Confirmed that this reproduces on Win10 build 1803, but not earlier builds. Affects all Firefox versions at least as far back as ESR52.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → Windows 10
Hardware: Unspecified → All
Version: 60 Branch → unspecified
Given that this is highly likely to be a website breaking issue, tracking for ESR52 also.
Priority: -- → P1
Flags: needinfo?(arai.unmht)
When we're parsing the regexp flags here: https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/js/src/frontend/TokenStream.cpp#2002

After the 'g' we send EOF through ucrtbase!isalpha (via JS7_ISLET: https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/js/src/util/Text.h#39)

Under 17133.1, ucrtbase!isalpha(-1) returns true (at least in English locale), and it throws off our parser's logic. Under 16299, ucrtbase!isalpha(-1) returned false.
In ischartype_l inside of isalpha, there is a table lookup.

In 16299, for the value -1 it looks like this

ucrtbase!ischartype_l+0x20:
00007ffd`2d3142f4 0fb71458        movzx   edx,word ptr [rax+rbx*2] ds:000001a9`f6a1d92e=0000
0:000> r rbx
rbx=ffffffffffffffff


In 17113 it looks like
ucrtbase!ischartype_l+0x2c:
00007ffc`84bd40ac 0fb70459        movzx   eax,word ptr [rcx+rbx*2] ds:00000252`321a718e=0302
0:000> r rbx
rbx=00000000000000ff

rbx is +255, not -1. Looks like ischartype_l changed some variable types.
Attached patch Patch (obsolete) — Splinter Review
Here's a patch that removes JS7_ISLET and the isalpha call, in case we want to fix this in FF.
I filed bug 1452693 for a static analysis to forbid calling isalpha in our tree.  There are only 21 hits for it in the tree now, and it shouldn't be that hard to rewrite all of them not use isalpha.
Attachment #8966312 - Flags: review?(nfroyd)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment on attachment 8966312 [details] [diff] [review]
Implement mozilla::IsAsciiAlpha

Review of attachment 8966312 [details] [diff] [review]:
-----------------------------------------------------------------

This works for me.  Can you file a followup about moving/unifying some of the text-related functions in xpcom/glue/nsCRTGlue.h into this file instead?
Attachment #8966312 - Flags: review?(nfroyd) → review+
Attachment #8966282 - Attachment is obsolete: true
>    // mozilla::MakeUnsigned doesn't work on char16_t -- arguably correctly -- so
>    // add a separate overload for it.

Do we also want to have a char32_t version for Unicode scalar values [1,2]?

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices
[2] https://groups.google.com/forum/?_escaped_fragment_=topic/mozilla.dev.platform/4XNA9x8ltZc
Flags: needinfo?(arai.unmht)
[Tracking Requested - why for this release]: Can we reconsider 59 tracking? If Microsoft's fix doesn't make it into the final 1803 release, and users pick up the update, things could be bad.
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ae76a38a514
Implement mozilla::IsAsciiAlpha.  r=froydnj
https://hg.mozilla.org/mozilla-central/rev/9ae76a38a514
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(In reply to Nathan Froyd [:froydnj] from comment #11)
> Can you file a followup about moving/unifying some of
> the text-related functions in xpcom/glue/nsCRTGlue.h into this file instead?

Filed bug 1453456 for doing a bunch of this, and other consolidating.
Verified that I'm seeing the expected results with the jsfiddle in #c0 now on m-c tip. Please nominate this for Beta and ESR52 approval when you get a chance, Waldo.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jwalden+bmo)
Pinging back - what's the progress on backporting this fix to current release and stable versions?
Attached patch Beta backportSplinter Review
Approval Request Comment
[Feature/Bug causing the regression]: MS changing stdlib behavior?  tho our misuse of isalpha goes back years, maybe even forever
[User impact if declined]: regexp parsing in various places gonna die
[Is this code covered by automated tests?]: not sure we can -- I don't think we have a way to run tests with different setlocale right now
[Has the fix been verified in Nightly?]: according to earlier comments here, yes
[Needs manual test from QE? If yes, steps to reproduce]: see comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?  Why is the change risky/not risky?]: IMO no, normal testing should be adequate.
[String changes made/needed]: N/A
Flags: needinfo?(jwalden+bmo)
Attachment #8968440 - Flags: review+
Attachment #8968440 - Flags: approval-mozilla-beta?
Attached patch ESR52 backportSplinter Review
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: regexp parsing in dead-simple cases is gonna die
User impact if declined: ^
Fix Landed on Version: trunk
Risk to taking this patch (and alternatives if risky): not too risky -- alternative is to hope MS changes its stdlib fast enough (i.e. ain't gonna happen)
String or UUID changes made by this patch: N/A
Attachment #8968451 - Flags: review+
Attachment #8968451 - Flags: approval-mozilla-esr52?
Note that the ESR52 backport changes JS7_ISLET rather than replacing it so that less code need be touched.
Attached patch Release backportSplinter Review
Approval Request Comment
[Feature/Bug causing the regression]: MS changes broke this; our code's been doing the wrong thing, previously harmlessly, for years now, tho
[User impact if declined]: regex parsing is gonna break, a lot
[Is this code covered by automated tests?]: can't, we don't have a way to run tests w/custom setlocale now
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: see comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?  Why is the change risky/not risky?]:  Not risky, all the math/logic here is very simple and the salient change is very minimal.
[String changes made/needed]: N/A
Attachment #8968490 - Flags: review+
Attachment #8968490 - Flags: approval-mozilla-release?
Comment on attachment 8968440 [details] [diff] [review]
Beta backport

work around isalpha breakage on recent windows 10 builds, approved for 60.0b14
Attachment #8968440 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
FYI, Nathan had a land a couple follow-up fixes to make gcc 4.9 happy:
https://hg.mozilla.org/releases/mozilla-beta/rev/4cb26dcd60e2
https://hg.mozilla.org/releases/mozilla-beta/rev/2ae0689283a5

I assume the release/ESR patches will need updating as well.
Flags: needinfo?(jwalden+bmo)
Flags: qe-verify+
This seems like a legitimate exception to our general rule of only taking major security fixes in late ESR. I'd count this as a major stability fix given that many users will likely be updating to the spring creators version of Win 10.  

Waldo, can you check that your patch applies ok to esr52 before we try to land it?
Just as an update, the new 17134.1 build doesn't fix this bug either.
I've managed to reproduce this bug using Win 10 x64, Version 1803 (OS Build 17134.1) by using the following link https://jsbin.com/lurazav/1/edit?js,console, on an affected Beta build - 60.0b13.

This is verified fixed on latest Nightly 61.0a1 (2018-04-23) and latest Beta 60.0b14 (20180419200216) running the same platform as above and on both 64 bit and 32 bit.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #26)
> Waldo, can you check that your patch applies ok to esr52 before we try to
> land it?

I built the JS engine with the backport patch before posting it.  Given that we don't enable C++14 that far back, I'm pretty sure we should be good.  Barring something truly extraordinary, i.e. an unknown build system bug, that's a proof the browser should also build with it.  (Note that the release/esr52 patches do not use the same style of C++14 relaxed constexpr code as the beta patch.)

Still, here you go for try-links running now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0015fe8d09c970204cc9bf54b0aeb025e0704153 (esr52)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad6d699d2866a9c91f0b31464c1fb7ef343ef282 (release)
Flags: needinfo?(jwalden+bmo)
Good to hear. Thanks waldo.  There was a test failure but the sheriffs are saying it's a failure of automation to find an artifact build, which shouldn't affect esr release. Let's uplift this!
Comment on attachment 8968451 [details] [diff] [review]
ESR52 backport

Let's uplift for esr52 since the Windows update will happen around the time of the next 52 release.
Attachment #8968451 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment on attachment 8968490 [details] [diff] [review]
Release backport

Let's uplift this to m-r in case we think it warrants a dot release - then we will be more prepared. 

I had not been thinking of this as a driver for a dot release but on second thought we might want to consider it. Waldo, what do you think? What kinds of sites might this break?
Attachment #8968490 - Flags: approval-mozilla-release? → approval-mozilla-release+
All kinds of sites, all number of sites.  Anything that uses regexes more or less.  It will not be pretty.
I'm reasonably certain this URL should be usable as testcase to determine whether a build demonstrates this bug or not:

data:text/html,<script>var s="FAIL";try{eval('/~!~/g');s="PASS";}catch(e){s="FAIL";}document.write(s);</script>
(In reply to David Major [:dmajor] from comment #7)
> In ischartype_l inside of isalpha, there is a table lookup.
> 
> In 16299, for the value -1 it looks like this
> 
> ucrtbase!ischartype_l+0x20:
> 
> In 17113 it looks like
> ucrtbase!ischartype_l+0x2c:

We ship ucrtbase.dll with Firefox and should be using app-local deployments of the universal CRT.

So why is the system's ucrtbase.dll coming into play here?

I know Windows 10 distributes ucrtbase.dll and that it is managed as part of the OS. Is there something special about the DLL search order for ucrtbase.dll that causes the system version to be loaded despite the presence of the DLL in the application's directory? Are we not incorporating ucrtbase.dll properly?

What I'm trying to say is... I thought our choice to use a local deployment of the universal CRT buffered us from unwanted changes like the one in this bug. What's going on?
Flags: needinfo?(dmajor)
Uplifted by aki to FIREFOX_52_7_4esr_RELBRANCH off the FIREFOX_52_7_3esr_RELEASE tag.
https://hg.mozilla.org/releases/mozilla-esr52/rev/93f6797768fc
(In reply to Gregory Szorc [:gps] from comment #37)
> We ship ucrtbase.dll with Firefox and should be using app-local deployments
> of the universal CRT.
> 
> So why is the system's ucrtbase.dll coming into play here?

That's a good point!

The "show loader snaps" checkbox in gflags has this to say:

3048:308c @ 80068546 - LdrpPreprocessDllName - INFO: DLL api-ms-win-crt-heap-l1-1-0.dll was redirected to C:\Windows\SYSTEM32\ucrtbase.dll by API set

According to https://blogs.msdn.microsoft.com/vcblog/2015/03/03/introducing-the-universal-crt/ , app-local use of the ucrt was once unsupported but is now allowed. I'm not sure if apiset redirection was included in that change? Might be a question for some Microsoft contacts.
Flags: needinfo?(dmajor)
This bug is also verified on 59.0.3-build1 (20180427210249) and 52.7.4esr-build3 (20180427222832) running Windows 10 x64/x86, Version 1803 (OS Build 17134.1). I've followed the STR from comment 0.
My understanding is that app-local deployment is only used for operating system versions where the ucrt might not be present (i.e. prior to Windows 10).
After applying the latest cumulative update for Windows 10 (build 17134.5), bug seems to be fixed.
(In reply to thibault.vlacich from comment #42)
> After applying the latest cumulative update for Windows 10 (build 17134.5),
> bug seems to be fixed.

Still reproduces for me with Fx58 on 17134.5. You sure you didn't get an updated Firefox build containing the fix from this bug too? :)
From a quick glance at disassembly I believe this is still present in the May Cumulative Update (17134.48).

For historical sake, this issue was resolved by UCRTBase 10.17134.191 in KB4340917

https://support.microsoft.com/en-us/topic/july-24-2018-kb4340917-os-build-17134-191-abc3502c-4945-6da2-63b2-16ce5aee28a1

Improves the ability of the Universal CRT Ctype family of functions to handle EOF as valid input.

(In reply to Masatoshi Kimura [:emk] from comment #41)

My understanding is that app-local deployment is only used for operating
system versions where the ucrt might not be present (i.e. prior to Windows
10).

Your understanding is correct, on W10 the System version will always be used.

You need to log in before you can comment on or make changes to this bug.