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?

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.

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.

Given that this is highly likely to be a website breaking issue, tracking for ESR52 also.

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.

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.

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?

> // 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

[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

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

Pinging back - what's the progress on backporting this fix to current release and stable versions?

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

[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

Note that the ESR52 backport changes JS7_ISLET rather than replacing it so that less code need be touched.

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

Comment on attachment 8968440 [details] [diff] [review] Beta backport work around isalpha breakage on recent windows 10 builds, approved for 60.0b14

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.

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)

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.

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?

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?

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.

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