780549 - crash access violation mozjs!js::types::AutoEnterCompilation::~AutoEnterCompilation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wayne Mery (:wsmwk)]

Attachment: windbg stacktrace

+++ This bug was initially created as a clone of Bug #743221 +++

perhaps related to bug 743221 - unclear as yet. captured by windbg. Having trouble recalling steps at the moment.


(74e0.d44): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=2238525c ebx=2866c000 ecx=22385000 edx=0749df00 esi=7eb00000 edi=002c8370
eip=678d1eaa esp=002c8358 ebp=002c8378 iopl=0         nv up ei pl nz na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010206
*** WARNING: Unable to verify checksum for C:\Program Files\Mozilla.org\FF 17.0a1 2012-08-02\mozjs.dll
mozjs!js::types::AutoEnterCompilation::~AutoEnterCompilation+0x1a:
678d1eaa 8b0e            mov     ecx,dword ptr [esi]  ds:0023:7eb00000=????????
0:000> ~* kp;!analyze -f;lm

.  0  Id: 74e0.d44 Suspend: 1 Teb: 7ffdf000 Unfrozen
ChildEBP RetAddr  
002c835c 678ebf04 mozjs!js::types::AutoEnterCompilation::~AutoEnterCompilation(void)+0x1a [e:\builds\moz2_slave\m-cen-w32-ntly\build\js\src\jsinferinlines.h @ 286]
002c8378 678ec0fa mozjs!js::mjit::Compiler::performCompilation(void)+0x94 [e:\builds\moz2_slave\m-cen-w32-ntly\build\js\src\methodjit\compiler.cpp @ 510]
002c8388 678ec45f mozjs!js::mjit::Compiler::compile(void)+0xa [e:\builds\moz2_slave\m-cen-w32-ntly\build\js\src\methodjit\compiler.cpp @ 112]
002cc4a0 67848f99 mozjs!js::mjit::CanMethodJIT(struct JSContext * cx = 0x146a3de0, struct JSScript * script = 0x2866c028, unsigned char * pc = 0x17d25c44 ">W", bool construct = false, js::mjit::CompileRequest request = CompileRequest_Interpreter (0n0), class js::StackFrame * frame = 0x04de01b0)+0x2af [e:\builds\moz2_slave\m-cen-w32-ntly\build\js\src\methodjit\compiler.cpp @ 998]
002ccd9c 67851d70 mozjs!js::Interpret(struct JSContext * cx = 0x146a3de0, class js::StackFrame * entryFrame = 0x04de0068, js::InterpMode interpMode = JSINTERP_NORMAL (0n0))+0xf49 [e:\builds\moz2_slave\m-cen-w32-ntly\build\js\src\jsinterp.cpp @ 2446]
002cce10 678523fa mozjs!js::InvokeKernel(struct JSContext * cx = 0x146a3de0, class JS::CallArgs args = class JS::CallArgs, js::MaybeConstruct construct = NO_CONSTRUCT (0n0))+0x610 [e:\builds\moz2_slave\m-cen-w32-ntly\build\js\src\jsinterp.cpp @ 356]
002cce58 67872ea1 mozjs!js::Invoke(struct JSContext * cx = 0x146a3de0, class JS::Value * thisv = 0x002cce80, class JS::Value * fval = 0x002cce9c, unsigned int argc = 0, class JS::Value * argv = 0x00000000, class JS::Value * rval = 0x002ccee0)+0x1ba [e:\builds\moz2_slave\m-cen-w32-ntly\build\js\src\jsinterp.cpp @ 388]
*** WARNING: Unable to verify checksum for C:\Program Files\Mozilla.org\FF 17.0a1 2012-08-02\xul.dll
002cce8c 59a6035a mozjs!JS_CallFunctionValue(struct JSContext * cx = 0x146a3de0, struct JSObject * objArg = 0x1fb55040, class JS::Value fval = class JS::Value, unsigned int argc = 0, class JS::Value * argv = 0x00000000, class JS::Value * rval = 0x002ccee0)+0x41 [e:\builds\moz2_slave\m-cen-w32-ntly\build\js\src\jsapi.cpp @ 5836]
002cd030 59a31f3b xul!nsJSContext::CallEventHandler(class nsISupports * aTarget = 0x1c08cc40, struct JSObject * aScope = 0x1fb55040, struct JSObject * aHandler = 0x286a6960, class nsIArray * aargv = 0x1859b9e0, class nsIVariant ** arv = 0x002cd080)+0x47a [e:\builds\moz2_slave\m-cen-w32-ntly\build\dom\base\nsjsenvironment.cpp @ 1929]
002cd08c 59a3a428 xul!nsGlobalWindow::RunTimeoutHandler(struct nsTimeout * aTimeout = 0x2238525c, class nsIScriptContext * aScx = 0x1747c8d0)+0xeb [e:\builds\moz2_slave\m-cen-w32-ntly\build\dom\base\nsglobalwindow.cpp @ 9555]
002cd118 59b7bacc xul!nsGlobalWindow::RunTimeout(struct nsTimeout * aTimeout = 0x185d1ce0)+0x1e8 [e:\builds\moz2_slave\m-cen-w32-ntly\build\dom\base\nsglobalwindow.cpp @ 9806]
002cd130 59a73a54 xul!nsGlobalWindow::TimerCallback(class nsITimer * aTimer = 0x185d26d0, void * aClosure = 0x185d1ce0)+0x1b [e:\builds\moz2_slave\m-cen-w32-ntly\build\dom\base\nsglobalwindow.cpp @ 10072]
002cd160 59aa6960 xul!nsTimerImpl::Fire(void)+0xf4 [e:\builds\moz2_slave\m-cen-w32-ntly\build\xpcom\threads\nstimerimpl.cpp @ 473]
002cd1d0 59a6e6d9 xul!nsThread::ProcessNextEvent(bool mayWait = false, bool * resu

Comment 1

[Scoobidiver (away)]

It first appeared in 17.0a1/20120719, but it has been discontinuous across builds.

More reports at:
https://crash-stats.mozilla.com/report/list?signature=js%3A%3Atypes%3A%3AAutoEnterCompilation%3A%3A~AutoEnterCompilation%28%29

Comment 2

[Wayne Mery (:wsmwk)]

(to clarify)
I left [@ EMPTY: no crashing thread identified; corrupt dump ] in crash sigs because if I were not running windbg, it eventually crashes with [@ EMPTY: no crashing thread identified; corrupt dump ]. Or at least, that's what all my crash reports have been - which are numerous (5-10 per month). The most recent:

bp-1bbf76a2-877c-40b0-82d7-26aba2120805 8/5/20129:07 AM
bp-c0319d4c-0ea2-4d84-a3ab-857082120805 8/5/20121:46 AM
bp-e09a0a9d-0d05-4bca-a1c2-e89ba2120805 8/4/201211:31 PM
bp-6778689c-3ef2-407e-b48a-8dd652120803 8/2/201211:36 PM
bp-4e2f767d-528b-4031-9ee3-e8bd62120803 8/2/20129:22 PM
bp-caad95ad-7013-4817-a9c6-e11f421208017/31/201210:22 PM

Comment 4

[Wayne Mery (:wsmwk)]

nbp, I'm OK to close if you think this should be gone with that bug fixed.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Wayne Mery (:wsmwk) from comment #4)
> nbp, I'm OK to close if you think this should be gone with that bug fixed.

This bug should be definitively fixed with Bug 777537 patch.

CC me if any identical failures including AutoEnterCompilation::~AutoEnterCompilation and/or CompilerOutput::isValid appear in a backtrace.

Comment 6

[Wayne Mery (:wsmwk)]

Scoobidiver, if bug 777537 was fixed in FF17 (has TM=17) then why is this not also TM=17?

Comment 7

[Scoobidiver (away)]

(In reply to Wayne Mery (:wsmwk) from comment #6)
> Scoobidiver, if bug 777537 was fixed in FF17 (has TM=17) then why is this
> not also TM=17?
I don't have access to bug 780274 to see if the patch landed in 17.0 or 18.0 but I can assure you there are still crashes in 17.0 Beta. See the link in comment 1.

Comment 8

[Wayne Mery (:wsmwk)]

(In reply to Scoobidiver from comment #7)
> (In reply to Wayne Mery (:wsmwk) from comment #6)
> > Scoobidiver, if bug 777537 was fixed in FF17 (has TM=17) then why is this
> > not also TM=17?
> I don't have access to bug 780274 to see if the patch landed in 17.0 or 18.0
> but I can assure you there are still crashes in 17.0 Beta. See the link in
> comment 1.

#73 crash for FF17. And occurs on current trunk.
for example bp-ed137fa9-cee8-482f-8c9d-d10582121225

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Wayne Mery (:wsmwk) from comment #8)
> (In reply to Scoobidiver from comment #7)
> > (In reply to Wayne Mery (:wsmwk) from comment #6)
> > > Scoobidiver, if bug 777537 was fixed in FF17 (has TM=17) then why is this
> > > not also TM=17?
> > I don't have access to bug 780274 to see if the patch landed in 17.0 or 18.0
> > but I can assure you there are still crashes in 17.0 Beta. See the link in
> > comment 1.
> 
> #73 crash for FF17. And occurs on current trunk.
> for example bp-ed137fa9-cee8-482f-8c9d-d10582121225

I looked again at this crash report, and it seems that we have 2 different kind of crashes, the previous one that you give the link to is a bad read access, while others recent one are bad write access to random memory (base + 0x0004) where base is not-NULL.  It looks like “base + 0x0004” is likely to be coming from the next line. (in fact it is …)

By reading the code, I remarked that a few cases to handle allocation issues might cause the init function to fail.  If the init function fails, we will still enter the destructor which need to check that we are definitely failing the compilation and that the compiler output registered by the init function is not available.

Mark this bug as s-s sec-critical because of random+0x0004 write.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Handle init function failures in the destructor.

Reset the outputIndex in case of failure and use it to check that we have a valid compilerOutput.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a66a93f8625

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Backed out in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9afe21048168

for causing mochitest failures.

See https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4a66a93f8625

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw] from comment #12)
> Backed out in:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9afe21048168
> 
> for causing mochitest failures.
> 
> See https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4a66a93f8625

The assertion added in this code seems to fail because threaded compilation is starting 2 compilations and the destructor is seems to be called later on.  I will investigate if there is any consequence to the current code, but threaded compilation has been enabled recently and this should not affect 17 / 18.

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff367beb8 in js::types::AutoEnterCompilation::~AutoEnterCompilation (this=0x7fffffff4440, __in_chrg=<optimized out>)
    at /home/nicolas/mozilla/ionmonkey/js/src/jsinferinlines.h:469
469             JS_ASSERT(info.outputIndex + 1 == cx->compartment->types.constrainedOutputs->length());
(gdb) p cx->compartment->types.constrainedOutputs->length()
$1 = 14
(gdb) p info.outputIndex
$2 = 12
(gdb) bt
#0  0x00007ffff367beb8 in js::types::AutoEnterCompilation::~AutoEnterCompilation (this=0x7fffffff4440, __in_chrg=<optimized out>)
    at /home/nicolas/mozilla/ionmonkey/js/src/jsinferinlines.h:469
#1  0x00007ffff3742251 in js::ion::AttachFinishedCompilations (cx=0x7fffd67950c0)
    at /home/nicolas/mozilla/ionmonkey/js/src/ion/Ion.cpp:1106
#2  0x00007ffff3375868 in js_InvokeOperationCallback (cx=0x7fffd67950c0) at /home/nicolas/mozilla/ionmonkey/js/src/jscntxt.cpp:1066
#3  0x00007ffff33758d1 in js_HandleExecutionInterrupt (cx=0x7fffd67950c0) at /home/nicolas/mozilla/ionmonkey/js/src/jscntxt.cpp:1083
#4  0x00007ffff38b3a3f in js::mjit::stubs::Interrupt (f=..., pc=0x7fffd688d5ba ":")
    at /home/nicolas/mozilla/ionmonkey/js/src/methodjit/StubCalls.cpp:805
#5  0x00007fffe414efe7 in ?? ()

Comment 14

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58d263c1c3dc

The threaded compilation does an init for each compilation which is spawned, and a second AutoEnterCompilation when it links the generated code.  This means that we can have multiple compilation queued in the AutoEnterCompilation.  So I changed the assert to make sure the the outputIndex is belowed the contrainedOutputs length.

Comment 15

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

Why was this not nominated for sec-approval before being checked in, since it affects branches?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58d263c1c3dc

Though I noticed, after merging to m-c of course (fail), that this may have caused fairly frequent OSX debug xpcshell crashes like the one in the log below. Retriggers are already pending. So there may be a backout coming.
https://tbpl.mozilla.org/php/getParsedLog.php?id=18301491&tree=Mozilla-Inbound

Comment 17

[Nicolas B. Pierron [:nbp]]

(In reply to Ryan VanderMeulen [Intermittent Availability Until Jan. 2] from comment #16)
> https://hg.mozilla.org/mozilla-central/rev/58d263c1c3dc
> 
> Though I noticed, after merging to m-c of course (fail), that this may have
> caused fairly frequent OSX debug xpcshell crashes like the one in the log
> below. Retriggers are already pending. So there may be a backout coming.
> https://tbpl.mozilla.org/php/getParsedLog.php?id=18301491&tree=Mozilla-
> Inbound

Looking at inbound history and annotation it seems that this was related to another changed backout at https://hg.mozilla.org/integration/mozilla-inbound/rev/f2a500997116 .

I am re-opening this bug, because I noticed that the returned value of the init function was not checked during Ion compilation, which means that we will write the -1 offset of the same pointer if there is an OOM, and segv/leak while garbage collecting the next time.  I'll submit the follow-up patch in a few seconds.

Comment 18

[Nicolas B. Pierron [:nbp]]

Attachment: Check returned value of AutoEnterCompilation::init in Ion compilation.

I added checks to make sure that we don't ignore the return value of init in AutoEnterCompilation, but it seems that it was first ignored in Ion compilation.

This means, with the previous patch, that in case of OOM we would have corrupted some allocated memory.

This patch does not show as part of the original issue, but as the previous patch for this issue check that we consider the returned value of the init function, I think it is worth going with this issue as the init function has been modified to reset the outputIndex.

Comment 19

[Nicolas B. Pierron [:nbp]]

Comment on attachment 696252 [details] [diff] [review]
Check returned value of AutoEnterCompilation::init in Ion compilation.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Need an OOM during the compilation. not trivial for me.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

all since 17, but with the previous patch, this exchange on random write (with crash) by a silent one.

If not all supported branches, which bug introduced the flaw?

This one, this patch should be merged with the previous one before being back-ported to all branches.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Not yet.  Except a recent modification in Ion.cpp which is modified by this file, this should be easy to provide.

How likely is this patch to cause regressions; how much testing does it need?

No regression expected for this patch.

Comment 20

[Nicolas B. Pierron [:nbp]]

Attachment: inbound patch [part 2, rebased]

This fix a compilation issue related to the change of the return value type made a few hours ago. (same logic as attachment 696252 [details] [diff] [review])

Comment 21

[Nicolas B. Pierron [:nbp]]

Attachment: aurora patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 772509

User impact if declined: crash, https://crash-stats.mozilla.com/report/list?signature=js%3A%3Atypes%3A%3AAutoEnterCompilation%3A%3A~AutoEnterCompilation%28%29

Testing completed (on m-c, etc.):
part 1, yes
part 2: to be landed after sec-approval.
(This patch merge both into one)

Risk to taking this patch (and alternatives if risky):
seems low. (JS test suite ok)

String or UUID changes made by this patch: N/A

Comment 22

[Nicolas B. Pierron [:nbp]]

Attachment: beta patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 772509

User impact if declined: crash, https://crash-stats.mozilla.com/report/list?signature=js%3A%3Atypes%3A%3AAutoEnterCompilation%3A%3A~AutoEnterCompilation%28%29

Testing completed (on m-c, etc.):
part 1, yes
part 2: to be landed after sec-approval.
(This patch merge both into one)

Risk to taking this patch (and alternatives if risky):
seems low.

String or UUID changes made by this patch: N/A

Comment 23

[Nicolas B. Pierron [:nbp]]

Comment on attachment 695782 [details] [diff] [review]
Handle init function failures in the destructor.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 772509

User impact if declined: crash, https://crash-stats.mozilla.com/report/list?signature=js%3A%3Atypes%3A%3AAutoEnterCompilation%3A%3A~AutoEnterCompilation%28%29

Testing completed (on m-c, etc.):
part 1, yes
part 2: to be landed after sec-approval.
(This patch merge both into one)

Risk to taking this patch (and alternatives if risky):
seems low.

String or UUID changes made by this patch: N/A



[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
sec-critical, cause crashes by writing at random memory addresses. (#73)

User impact if declined: crash, https://crash-stats.mozilla.com/report/list?signature=js%3A%3Atypes%3A%3AAutoEnterCompilation%3A%3A~AutoEnterCompilation%28%29

Fix Landed on Version: inbound & central, the part 2 is not needed as the second part of this bug deals with IonMonkey which landed in Fx 18 (beta).

Risk to taking this patch (and alternatives if risky): 
seems low.

String or UUID changes made by this patch: N/A

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

Comment 24

[Nicolas B. Pierron [:nbp]]

Comment on attachment 696430 [details] [diff] [review]
beta patch

[Approval Request Comment]
See comment #22 (same patch)

Comment 25

[Nicolas B. Pierron [:nbp]]

Comment on attachment 696422 [details] [diff] [review]
inbound patch [part 2, rebased]

Obsolete because bug 823884 as been backout at changeset e497c2e40dc6.
Use attachment 696252 [details] [diff] [review] instead.

Comment 26

[Nicolas B. Pierron [:nbp]]

Comment on attachment 696252 [details] [diff] [review]
Check returned value of AutoEnterCompilation::init in Ion compilation.

Use attachment 696422 [details] [diff] [review] if bug 823884 is re-landed before this patch.

Comment 27

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8468d858595c (part 2)

This part of the patch should go in all version after fx 18, including fx 18. as it deals with IonMonkey missing check.

Comment 28

[Robert Kaiser]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #27)
> This part of the patch should go in all version after fx 18, including fx
> 18. as it deals with IonMonkey missing check.

As 18 is closed for anything other than the last two high-value fixes we are tracking, you should probably email release-mgmt in addition to those approval requests if this really should still land there.

Comment 29

[bhavana bajaj [:bajaj]]

This does not seem to be a top-crasher in FF18 and we already have our scheduled final beta 6 out . We are spinning an urgent beta 7 only for blocker issues at this time for Fx18.Hence approving the patch only for aurora/b2g18 .

In addition I am not approving the patch for release, as we would not be chemspilling for this issue.

Comment 30

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/8468d858595c

Comment 31

[Robert Kaiser]

Oh, and we check in a sec-crit fix more than a week before releasing a vulnerable version? That doesn't sound good to me. :(

Comment 32

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ffa57091f9fa

Comment 33

[Nicolas B. Pierron [:nbp]]

What is the meaning of 19+ for tracking flags?

Comment 34

[Andrew McCreight [:mccr8]]

I think it means the version of esr17 (or whatever) that will be released at the same time as Firefox 19.

Comment 35

[Alex Keybl [:akeybl]]

(In reply to Andrew McCreight [:mccr8] from comment #34)
> I think it means the version of esr17 (or whatever) that will be released at
> the same time as Firefox 19.

Correct.

Comment 36

[u279076]

Do we have steps to reproduce this crash which QA can use for verification?

Comment 37

[Nicolas B. Pierron [:nbp]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #36)
> Do we have steps to reproduce this crash which QA can use for verification?

You need to be at the limit to the OOM, and have plenty of concurrent Ion compilations, then after this is a question of luck to hit this error.

One potential script which might trigger OOM in a similar area would be something like:

var nFun = 10000;
var triggerIon = 15000;
var funs = new Array(nFun);

for (var i = 0; i < nFun; i++)
  funs[i] = new Function("");

for (var j = 0; j < triggerIon; j++) {
  // Will cause plenty of Ion compilation in parallel.
  for (var f = 0; f < nFun; f++)
     funs[f]();
}

Sadly this would be hardly reproducible as you are more likely to have OOM for other reasons than in the AutoEnterCompilation.  If you want to attack such OOM cases, try to increase nFun to cause the grow (reallocation) of the compilerOutput vector during the append function contained in the init function.

Comment 38

[u279076]

Thanks Nicolas. Unfortunately, given the complexity to reproduce this crash, I don't think QA won't be able to reliably verify this is fixed in a timely manner. Marking this bug as [qa-].

Would this be something we could explore covering with automation?

Comment 39

[Lukas Blakk [:lsblakk] use ?needinfo]

Leaving the approval nom until we are ready to take landings for v1-train on b2g18 branch as per https://wiki.mozilla.org/Release_Management/B2G_Landing

Comment 40

[bhavana bajaj [:bajaj]]

Please go ahead and land this for esr17 at this time.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Landed on esr17 minus the Ion.cpp bits.
https://hg.mozilla.org/releases/mozilla-esr17/rev/54f5dba3caf4

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8cd47ef18c2f

Comment 43

[Alex Keybl [:akeybl]]

Since Firefox 20 is now released, v1.0.1 branches are still open, and this is considered low risk, we can uplift to v1.0.1. Marking as tef+ and flipping status-b2g18-v1.0.1 to affected.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

In before the branch.