1351278 - Crash on github.com with mingw-w64 compiled Firefox based on ESR 45.8.0

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Georg Koppen]

A user recently reported a reproducible crash with Tor Browser, which is based on the latest ESR 45 code, when visiting https://help.github.com/categories/writing-on-github/ (https://lists.torproject.org/pipermail/tor-talk/2017-March/043064.html). Tracking this back with our builds leads to the conclusion it got introduced with the switch from ESR 38 to ESR 45.

This crash is reproducible on all Windows 7 machines we had the browser tested on but we found other Windows versions are fine (like Windows 2012 Server or Windows 2016 Server).

We produced a reduced testcase (https://people.torproject.org/~boklm/bug21795/t5.html) which indicates that it is necessary to have a worker involved. Otherwise the browser is not crashing. It is still crashing after removing all of our custom patches leaving just the ones that are necessary for cross-compilation. This is the reason why I put this ticket here into the DOM: Workers category.

Taking a 45.8.0esr build Mozilla ships does not trigger that crash.

For cross-compiling Firefox with just the bare minimum of patches we used the following .mozconfig:

CROSS_COMPILE=1

ac_add_options --enable-application=browser
ac_add_options --target=i686-w64-mingw32
ac_add_options --enable-default-toolkit=cairo-windows
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-mingw
mk_add_options MOZ_APP_DISPLAYNAME="Tor Browser"
mk_add_options MOZILLA_OFFICIAL=1
mk_add_options BUILD_OFFICIAL=1

ac_add_options --disable-debug
ac_add_options --enable-optimize
ac_add_options --enable-strip
ac_add_options --enable-official-branding

ac_add_options --disable-updater
ac_add_options --without-intl-api
ac_add_options --disable-sandbox
ac_add_options --disable-eme
ac_add_options --disable-crashreporter
ac_add_options --disable-maintenance-service
ac_add_options --disable-webrtc
ac_add_options --disable-tests

We use GCC 5.1.0 and mingw-w64 commit a0cd5afeb60be3be0860e9a203314c10485bb9b8. The additional patches we used on top of 45.8.0esr were:

https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.8.0esr-6.5-2&id=f54b28dc50354bb8a076a0012b63fba42df967f1
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.8.0esr-6.5-2&id=84e97cbd25dcc406440529b8e482f94b5956c3c6
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.8.0esr-6.5-2&id=999a2048e8f365b49fb0eb940ce708683009b8f6
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.8.0esr-6.5-2&id=7856a64221b169dd9c4d2c8e75a4dd213d043484
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.8.0esr-6.5-2&id=aebe95a1b1cfd9af5b309e4d25efbeac0a8e76a4
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.8.0esr-6.5-2&id=ba28d792ed2c2dc81eb1391f9f06b327eff3fffd

We use wrapper scripts for compilation to harden the browser on Windows:

https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/gitian/build-helpers/i686-w64-mingw32-g++?h=maint-6.5
https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/gitian/build-helpers/i686-w64-mingw32-gcc?h=maint-6.5
https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/gitian/build-helpers/i686-w64-mingw32-ld?h=maint-6.5

Removing the SSP from those wrappers does not prevent the browser from crashing.

Comment 1

[Tom Ritter [:tjr]]

On Windows 7 I can reproduce with TB 6.5.1 and in WinDBG I get a Stack Overflow. But TB Release doesn't have any symbols or anything so that's all I get without staring at assembly. I also reproduced it in TB 7.0a2, but again, no symbols. 

7.0a2:
{{{
WARNING: Stack overflow detected. The unwound frames are extracted from outside normal stack bounds.
eax=00000e5e ebx=000072f8 ecx=0871f400 edx=17f42d90 esi=ffffff88 edi=17acac40
eip=2cd18531 esp=06806da8 ebp=17b27b70 iopl=0         nv up ei pl nz na po nc
}}}

I tried a few things to get more info (like searching for the assembly instructions at eip on a different PC (same browser) in IDA - but no dice.)

Comment 2

[Andrew McCreight [:mccr8]]

If it is still crashing without your patches, and it doesn't crash in Firefox ESR45, it seems like it is some compiler issue, either like a bug or maybe it is doing some aggressive optimization that takes advantage of undefined behavior in our code.

Comment 3

[Andrew McCreight [:mccr8]]

Nathan, any ideas?

Comment 4

[(Away)]

Is this reproducible on local builds? If so, could you provide a stack? Or even better, a minidump?

Comment 5

[(Away)]

Er, a minidump from a local build wouldn't be useful unless you want to share out the PDBs as well, so nevermind. A stack would be a good starting point.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Nathan, any ideas?

Uh.  I suppose TB doesn't have any sort of crash reporting infrastructure...

You don't --enable-release, so this isn't a problem from ICF.

Is the stack overflow on the main thread or somewhere else?  If it was somewhere else, we could rule out JS.

Do any of those extra compilation options, or particular things you use in your runtime, cause extra stuff to be stored on the thread stack at startup?  Our multithreaded JS parsing cares a lot about how much stack space is available, and if there's a mismatch, parsing complex JS can easily overflow the stack:

https://hg.mozilla.org/releases/mozilla-esr45/file/tip/js/src/vm/HelperThreads.cpp#l471

Maybe try increasing those constants?

Just some out-loud thoughts, nothing concrete, sorry. :(

Comment 7

[Georg Koppen]

Thanks. No, we don't have such an infrastructure yet but we are thinking about setting one up. I am currently trying to get some sort of stacktrace. So far I only got corrupted stacks that were not that meaningful. :(

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Georg Koppen from comment #7)
> Thanks. No, we don't have such an infrastructure yet but we are thinking
> about setting one up. I am currently trying to get some sort of stacktrace.
> So far I only got corrupted stacks that were not that meaningful. :(

Out of curiosity, what makes you say the stacks are corrupted?

Comment 9

[Georg Koppen]

(In reply to Nathan Froyd [:froydnj] from comment #8)
> (In reply to Georg Koppen from comment #7)
> > Thanks. No, we don't have such an infrastructure yet but we are thinking
> > about setting one up. I am currently trying to get some sort of stacktrace.
> > So far I only got corrupted stacks that were not that meaningful. :(
> 
> Out of curiosity, what makes you say the stacks are corrupted?

Because I get

[New Thread 2796.0xed0]
warning: [JavaScript Error: "The character encoding of the HTML document was not
 declared. The document will render with garbled text in some browser configurat
ions if the document contains characters from outside the US-ASCII range. The ch
aracter encoding of the page must be declared in the document or in the transfer
 protocol." {file: "https://people.torproject.org/~boklm/bug21795/t5.html" line:
 0}]


Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 2796.0xed0]
0x0e158534 in ?? ()
(gdb) bt
#0  0x0e158534 in ?? ()
#1  0xffffff88 in ?? ()
#2  0x22c27180 in ?? ()
#3  0x1d315e60 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

in GDB. I tried to make sure the frame pointer is not omitted in case that could cause such a result but that did not change things. The GDB version I have does not like all the symbols either it seems. It hangs if I don't strip the build.

But it seems like Andrew's comment 2 might be a good point: if I compile with --disable-optimize the build does not crash. It seems as well that the issue is not on the main thread given the stack trace snippet above.

Not sure what I could do to get a better stack trace. I guess one thing is trying a newer GDB. I can upload the build somewhere as well in case anyone else has ideas and want to look at it.

One further data point: the crash does not happen with ESR52. So, I could try bisecting my way to the problematic code as well although I had some hope I could avoid that as this is usually a huge PITA.

Comment 10

[Andrew McCreight [:mccr8]]

The ?? could be JIT code. In your debugger, you can try running DumpJSStack().

Comment 11

[Andrew McCreight [:mccr8]]

I'm marking this sec-other because it does not affect Firefox per se.

Comment 12

[Georg Koppen]

(In reply to Andrew McCreight [:mccr8] from comment #10)
> The ?? could be JIT code. In your debugger, you can try running
> DumpJSStack().

That just gives me: "Cannot access memory at address 0x1fce74b4"

Comment 13

[Georg Koppen]

Okay, a newer GDB copes with the non-stripped build it seems. But still no change regarding the stack trace. :(

Comment 14

[(Away)]

I don't speak GDB very well, but if I had this crash in front of me in WinDbg (hi tjr!), I'd do:

- "!address @esp" (plus also on the next higher block) and see whether the allocation data looks reasonable, to try to differentiate between a once-sane stack pointer that simply grew too far, versus an altogether garbage one (maybe clobbered by a bad calling convention or something).

- "dps @esp L10000" to dump symbolicated pointers until we get back down to known territory (assuming esp isn't complete garbage).

Comment 15

[Tom Ritter [:tjr]]

Attachment: WinDbg Output for dmajor #1

(In reply to David Major [:dmajor] from comment #14)
> I don't speak GDB very well, but if I had this crash in front of me in
> WinDbg (hi tjr!), I'd do:

Hi!

I was just prepping some work on this when I saw this.  This is what I see in Visual Studio: http://i.imgur.com/95aiM3i.png The strange thing, is if I search for the instruction right before it, I can't find it in any .exe/.dll in the directory: binwalk -R "\x0f\x84\x1d\x00\x00\x00" *

> - "!address @esp" (plus also on the next higher block) and see whether the
> allocation data looks reasonable, to try to differentiate between a
> once-sane stack pointer that simply grew too far, versus an altogether
> garbage one (maybe clobbered by a bad calling convention or something).

I don't think I can get to "the next higher block" (you mean stack frame?) .frame only shows one frame.
 
> - "dps @esp L10000" to dump symbolicated pointers until we get back down to
> known territory (assuming esp isn't complete garbage).

Attached the output of 
!address @esp
.frame
dps @esp L10000

Comment 16

[Georg Koppen]

I just uploaded the build I've been working with today in case someone wants to skip all the Tor cruft and just look at Firefox code:

https://people.torproject.org/~gk/testbuilds/firefox_45.8.0_github_crash.zip
https://people.torproject.org/~gk/testbuilds/firefox_45.8.0_github_crash.zip.asc

Comment 17

[(Away)]

In Tom's file -
[0b1d0000, 0b2d0000) is the total area of the stack. 
Of that,
[0b1d0000, 0b2c8000) is reserved but not yet commited.
[0b2c8000, 0b2c9000) is the guard page.
[0b2c9000, 0b2d0000) is a reasonable-looking stack.

esp at the crash is 0b2c7418, so it looks like we had a sane stack until esp got decremented by some amount greater than one page, so we bypassed the guard page and the OS couldn't do fixup, and instead we landed in the uncommitted region.

Comment 18

[(Away)]

> I was just prepping some work on this when I saw this.  This is what I see
> in Visual Studio: http://i.imgur.com/95aiM3i.png The strange thing, is if I
> search for the instruction right before it, I can't find it in any .exe/.dll
> in the directory: binwalk -R "\x0f\x84\x1d\x00\x00\x00" *

I suspect we're in JIT code. Those FFFFFF88's look like some of the magic jsval constants.

Comment 19

[Georg Koppen]

I finally got the problematic bug via bisection: it is bug 1108290. rev 274223 is unaffected and rev 274226 is crashing (I can't build rev 274224 or 274225).
I've uploaded respective builds in case anyone wants to test that out:

https://people.torproject.org/~gk/testbuilds/firefox-274223.zip
https://people.torproject.org/~gk/testbuilds/firefox-274223.zip.asc

https://people.torproject.org/~gk/testbuilds/firefox-274226.zip
https://people.torproject.org/~gk/testbuilds/firefox-274226.zip.asc

Lars (or anybody else), any ideas what is going wrong here? If not, how could we debug this further?

Comment 20

[Lars T Hansen [:lth]]

There were several fixes posted over time to deal with weaknesses in that changeset.  Changeset 280656 (bug 1231024) was a local fix to deal with a register allocation bug in the optimization itself.  Changeset 318399 (bug 1309903) was a larger change to the register allocator and how it is used to make it more resilient; the problem is that on x86, Apply uses all available registers and it was pushing at the boundaries of the register allocator.  None of those made it into 45, I think.

Don't know why this should be crashing on x64, though, as it is not register-starved.

You can test whether this optimization is the problem.  In IonBuilder.cpp, look for code that looks roughly like this in jsop_funapply:

        if (native && native->isNative() && native->native() == fun_apply &&
            objTypes &&
            objTypes->getKnownClass(constraints()) == &ArrayObject::class_ &&
            !objTypes->hasObjectFlags(constraints(), OBJECT_FLAG_LENGTH_OVERFLOW) &&
            ElementAccessIsPacked(constraints(), argument))
        {
            return jsop_funapplyarray(argc);
        }

If you insert "false &&" as the first conjunct in that "if" then the optimization should be skipped entirely.

Comment 21

[(Away)]

Lars: As I understand it, the problem is that we've decremented esp by such a large amount that it's gone outside the currently-committed stack region _and_ skipped over the guard page entirely, so the OS didn't get an opportunity to do an on-demand commit.

In Tom's repro:
> http://i.imgur.com/95aiM3i.png
I can see `sub esp, ebx`, and ebx itself was recently `shl`d, so the delta might have been big enough to be a problem.

I see that bug 1108290 made some changes so how we do `sub` instructions. That seems to me more likely than the fun_apply optimization -- unless they're one and the same and I'm misunderstanding something?

Does the masm just need to know that esp can't be decremented by more than one page at a time?

Comment 22

[Lars T Hansen [:lth]]

Oh, nasty.  The "apply" code is the only code in the JIT that uses variable subtract from SP (subFromStackPtr with a Register argument), apart from one use on ARM64 that is probably safe by construction (didn't look closely, the ARM64 JIT is not currently operational anyway).  And the common implementation of subFromStackPtr(Register) just performs a simple subtract, which is wrong, at least on Windows.

Nice find.

Yes, subFromStackPtr(Register) needs to be taught about the guard pages, as the Imm32-taking variant does.

Comment 23

[Georg Koppen]

(In reply to Lars T Hansen [:lth] from comment #20)
> There were several fixes posted over time to deal with weaknesses in that
> changeset.  Changeset 280656 (bug 1231024) was a local fix to deal with a
> register allocation bug in the optimization itself.  Changeset 318399 (bug
> 1309903) was a larger change to the register allocator and how it is used to
> make it more resilient; the problem is that on x86, Apply uses all available
> registers and it was pushing at the boundaries of the register allocator. 
> None of those made it into 45, I think.
> 
> Don't know why this should be crashing on x64, though, as it is not
> register-starved.
> 
> You can test whether this optimization is the problem.  In IonBuilder.cpp,
> look for code that looks roughly like this in jsop_funapply:
> 
>         if (native && native->isNative() && native->native() == fun_apply &&
>             objTypes &&
>             objTypes->getKnownClass(constraints()) == &ArrayObject::class_ &&
>             !objTypes->hasObjectFlags(constraints(),
> OBJECT_FLAG_LENGTH_OVERFLOW) &&
>             ElementAccessIsPacked(constraints(), argument))
>         {
>             return jsop_funapplyarray(argc);
>         }
> 
> If you insert "false &&" as the first conjunct in that "if" then the
> optimization should be skipped entirely.

That seems to work around the problem. I tested several builds with and without the change you suggested and builds are only crashing if I don't apply it. (I hope that makes sense). Thanks. I guess we pick that change up for Tor Browser 6.5.2 but it seems to me this is kind of an issue that would be good having a fix for on esr45 in general as well.

Do you know whether that kind of JIT code is covered by any preference we could flip? We set `javascript.options.ion.content`, `javascript.options.baselinejit.content`, and `javascript.options.typeinference` to `false` in our security slider (medium level) but the browser is still crashing then. That's a bit surprising as we were under the assumption that flipping those three prefs would be enough to avoid bugs in Firefox's JIT code.

Comment 24

[Lars T Hansen [:lth]]

I don't know anything about those preferences.  I can't find any trace of .ion.content and .baselinejit.content in the current sources, and looking in modules/libpref/init/all.js for both esr38 and esr45 does not suggest that they exist.  Are these local adaptations for Tor perhaps?  Let's ask Jan if he knows anything about them.

Even with the fix I suggested I think (I need to investigate more deeply but that was the impression I had from reading the source) you should be able to crash Firefox if you apply an arguments object that is large enough, but at least we're all agreed on the problem.

Comment 25

[Jan de Mooij [:jandem]]

(In reply to Lars T Hansen [:lth] from comment #24)
> I don't know anything about those preferences.  I can't find any trace of
> .ion.content and .baselinejit.content in the current sources, and looking in
> modules/libpref/init/all.js for both esr38 and esr45 does not suggest that
> they exist.  Are these local adaptations for Tor perhaps?  Let's ask Jan if
> he knows anything about them.

Ugh yes, we used to have separate chrome and content JIT prefs, but that changed when we moved these flags to the runtime (bug 939562).

Georg, you want to flip the following prefs:

javascript.options.ion
javascript.options.baselinejit

I'd also suggest:

javascript.options.native_regexp (regex JIT, it uses the regex interpreter if false)
javascript.options.asmjs

Comment 26

[Lars T Hansen [:lth]]

The CodeGenerator actually guards against "too many" arguments (see CodeGenerator::visitApplyArrayGeneric) but this only considers the configurable limit maxStackArgs, which is not related to guard pages and so on.  As a quick workaround we could in principle insert a fixed, small limit there in Windows builds.  However, the code for applying the arguments object (see CodeGenerator::visitApplyArgsGeneric) does not have that guard and likely has the same bug.  Also, these are just hacks.

The problem with fixing subFromStackPtr(Register) properly is that it needs at least one temp, and we are already highly starved for registers in the situation where we need that temp.  But it is certainly the way to go for mozilla-central.  For ESR 52 a backport might be reasonable.  For ESR 45 I don't know yet; likely we should disable the apply(Array) case there given all the bug fixes that have come in that we aren't going to backport, but we may wish to try to fix subFromStackPtr(Register) anyway.  All that said, I have the sense that this stack overflow is not actually an exploitable crash, it is just a crash.  If it weren't crashing on something as high profile as github we might let it slide.

Comment 27

[Lars T Hansen [:lth]]

Inserting a stack probe at the beginning of emitAllocateSpaceForApply is plausible, we have a spare register there and can push/pop a value to free up another.  Then we don't have to worry about probing when we actually subtract from the SP, I hope.  The following sketch is crude and needs testing:

#ifdef XP_WIN
    Label probeLoop;
    masm.push(argcreg);
    masm.lshiftPtr(Imm32(ValueShift), argcreg);
    masm.negq(argcreg);                                 // FIXME
    masm.addPtr(Assembler::getStackPointer(), argcreg); // Lower limit, roughly
    masm.movePtr(Assembler::getStackPointer(), extraStackSpace);
    masm.bind(&probeLoop);
    masm.subPtr(ImmWord(4096), extraStackSpace);
    masm.or32(Imm32(0), Address(extraStackSpace, 0));
    masm.branchPtr(Assembler::AboveOrEqual, extraStackSpace, argcreg, &probeLoop);
    masm.pop(argcreg);
#endif

Comment 28

[Jan de Mooij [:jandem]]

(In reply to Lars T Hansen [:lth] from comment #26)
> The CodeGenerator actually guards against "too many" arguments (see
> CodeGenerator::visitApplyArrayGeneric) but this only considers the
> configurable limit maxStackArgs, which is not related to guard pages and so
> on.  As a quick workaround we could in principle insert a fixed, small limit
> there in Windows builds.  However, the code for applying the arguments
> object (see CodeGenerator::visitApplyArgsGeneric) does not have that guard
> and likely has the same bug.  Also, these are just hacks.

FWIW I think the Baseline ICs for this have similar limits. I don't think it's too bad to be conservative by optimizing only the sane-argc cases.

Comment 29

[Lars T Hansen [:lth]]

Attachment: bug1351278-dont-apply-too-much.patch

What do you think of this as a PoC?  If you agree, I also think I ought to clean up the apply(arguments) case, and then I need to get somebody who can repro the bug to test it on Windows.

(The code is for m-c, may need additional backport for esr.)

Comment 30

[Jan de Mooij [:jandem]]

Comment on attachment 8855758 [details] [diff] [review]
bug1351278-dont-apply-too-much.patch

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

The bailout is not something we can easily avoid with an OOL VM call? This is probably fine though, definitely the simplest fix to backport.

I'd prefer using the more conservative limit on all platforms, so we don't get Windows-only (performance) bug reports we can't repro.

Comment 31

[Lars T Hansen [:lth]]

Attachment: bug1351278-dont-apply-too-much-v2.patch

(This still needs testing by somebody who can repro the original problem.)

Pretty local fix.  The test case / benchmark included clearly shows the cost of bailout but the cutoff is large enough (at 375) that it's probably not a real hardship.  Some programs will no doubt regress.

Comment 32

[Tom Ritter [:tjr]]

Flagging GK for review also.

Comment 33

[Georg Koppen]

While I not feel I am a good reviewer of the code itself I can try to test whether the patch fixes the problem (for us). Contrarily to what I said in comment 9 I seem to have found a way to get ESR52-based Tor Browsers crashed and the crash looks like the one we are dealing with in this bug. Lars, do you think you could give me an ESR52 version of your patch as this would make it much easier for me to verify that it fixes the problem. Or do you need a confirmation for m-c first (not sure if I get that one crashed with the testcase I have, though)?

Comment 34

[Lars T Hansen [:lth]]

Georg, I will prep a patch suitable for ESR52 and ping you, probably won't be until Tuesday though.

Comment 35

[Lars T Hansen [:lth]]

Georg, the patch that's already attached to this bug applies cleanly to ESR52 (just checked), and you should be able to test with that.

Comment 36

[Lars T Hansen [:lth]]

Comment on attachment 8857437 [details] [diff] [review]
bug1351278-dont-apply-too-much-v2.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Probably hard: Not obvious that an exploit is even possible, this may just be a crasher.  In the limit it may be possible to exploit it if the heap is right up against the stack but ASLR and other mechanisms tend to prevent that.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The test cases and commit message more or less explain what's going on.

Which older supported branches are affected by this flaw?
All branches back to ESR45.

If not all supported branches, which bug introduced the flaw?
Arguably bug 1108290 (Nov 24, 2015) though there have been follow-ups for that.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The current patch applies cleanly to ESR52 and should apply to FF53 and newer.  It looks like it might also apply to ESR45 but I have not tried.

How likely is this patch to cause regressions; how much testing does it need?
It is likely to cause some performance regressions in programs that apply() many arguments (more than 375) but no functional regressions.  We have OK test coverage with what's in the repo already and with what's included in this bug.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

We're shipping Fx53 and ESR 45.9 today (our final planned release off that branch), so unless this becomes a chemspill-worthy issue, we won't be fixing this for those releases.

Comment 38

[Al Billings [:abillings - ex-MoCo]]

This is a sec-other rated bug. Why is sec-approval, which only applies to high and critical rated issues, being asked for here?

Comment 39

[Lars T Hansen [:lth]]

abillings, probably just because I don't know the drill properly.

Comment 40

[Lars T Hansen [:lth]]

Comment on attachment 8857437 [details] [diff] [review]
bug1351278-dont-apply-too-much-v2.patch

Cancelling request for sec-approval per abillings msg (above).

Comment 41

[Gerry Chang [:gchang]]

Track 54+/55+ as security issue.

Comment 42

[Georg Koppen]

Attachment: 0001-test-for-bug-1351278.patch

This was a huge PITA, sorry for the delay. It turns our while I had a build based on a recent commit on the ESR52 branch that crashed, building again without the workaround mentioned in comment:20 does not crash the resulting browser. I tried different things but no dice.

So, I looked at the patch and created a version for ESR45 which is attached. I reverted the above mentioned workaround and the build crashed. Then I applied the ESR45 patch and the build does not crash anymore. Thus, I think the patch does indeed fix the bug.

Comment 43

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/288f9c879d41b61c90f16af0e002d6331df921bd
Bug 1351278 - Avoid stepping over Windows stack guard page, r=jandem

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/288f9c879d41

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Please request Beta/ESR52 approval on this when you're comfortable doing so.

Comment 46

[Lars T Hansen [:lth]]

Comment on attachment 8857437 [details] [diff] [review]
bug1351278-dont-apply-too-much-v2.patch

Approval Request Comment
[Feature/Bug causing the regression]:
An optimization of the Function.prototype.apply operator in JS

[User impact if declined]:
Possible to create DOS (crash) situations on Windows, exploit potential unclear.

[Is this code covered by automated tests?]:
Yes, included in patch.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Not really.

[Why is the change risky/not risky?]:
Local patch, well understood code, we're really just tightening a parameter.

[String changes made/needed]:
None.

Comment 47

[Lars T Hansen [:lth]]

Comment on attachment 8857437 [details] [diff] [review]
bug1351278-dont-apply-too-much-v2.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Problem was discovered by the Tor team on the ESR branch, good to fix it.

User impact if declined: 
Possible DOS crashes on Windows, exploit potential unclear, probably low.

Fix Landed on Version:
55

Risk to taking this patch (and alternatives if risky): 
Low risk.

String or UUID changes made by this patch: 
None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 48

[Gerry Chang [:gchang]]

Comment on attachment 8857437 [details] [diff] [review]
bug1351278-dont-apply-too-much-v2.patch

Fix a security issue. Beta54+. Should be in 54 beta 3.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a13f60339131

Comment 50

[Andrei Vaida [:avaida]]

(In reply to Lars T Hansen [:lth] from comment #46)
> [Is this code covered by automated tests?]:
> Yes, included in patch.
> [Has the fix been verified in Nightly?]:
> Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: 
> No.

Setting qe-verify- based on Lars' assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 51

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8857437 [details] [diff] [review]
bug1351278-dont-apply-too-much-v2.patch

We uplifted this to Beta54, let's keep the ESR52.2 in sync.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/2cd730a42445