855236 - Crash with SIGTRAP

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

mjitChunkLimit(11);
options('strict');
function h(...d) {};
eval("\
    toSource = function () {\
        x = 0;\
        return function() {\
            ++x;\
            if (x % 4) {\
                this[0] = 1;\
                this.h();\
            }\
        };\
    }();\
");
Object.preventExtensions(this);
g = function() {}
eval("function f(){}");

crashes js debug shell on m-c changeset 178a4a770bb1 with --no-ion -a at SIGTRAP.

During the course of reduction I had to alter the mjitChunkLimit value as needed to keep crashing with smaller testcases.

Program received signal SIGTRAP, Trace/breakpoint trap.
0x009b3e80 in ?? ()
gdb $ bt
#0  0x009b3e80 in ?? ()
#1  0x00b3cd40 in ?? ()
Previous frame inner to this frame (gdb could not unwind past this frame)
gdb $

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   106754:a26469331bf9
user:        Brian Hackett
date:        Thu Aug 30 14:44:15 2012 -0600
summary:     Re-enable JM inlining when IonMonkey is disabled, bug 786925. r=dvander

This iteration took 123.879 seconds to run.

Comment 2

[Christian Holler (:decoder)]

Needinfo from Brian based on comment 1 :)

Comment 3

[Christian Holler (:decoder)]

Also marking s-s since a SIGTRAP could indicate an invalid jump.

Comment 4

[Daniel Veditz [:dveditz]]

Does the BaselineCompiler change/fix this code? Or is it still a problem?

Comment 5

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision b5cb88ccd907).

Comment 6

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

(In reply to Daniel Veditz [:dveditz] from comment #4)
> Does the BaselineCompiler change/fix this code? Or is it still a problem?

BaselineCompiler does fix this, specifically m-c changeset e34e126f817e. The following date shown is inaccurate, as BaselineCompiler just landed this morning.

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   127151:e34e126f817e
user:        Jan de Mooij
date:        Mon Dec 17 10:55:02 2012 +0100
summary:     Bug 821315 - Compile BINDGNAME, SETPROP, SETGNAME, SETNAME. r=djvj

(The supposed regressing changeset a26469331bf9 landed on all branches except ESR 17)

Comment 7

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 8

[bhavana bajaj [:bajaj]]

Can we get a security rating on this ?

Comment 9

[Alex Keybl [:akeybl]]

(In reply to bhavana bajaj [:bajaj] from comment #8)
> Can we get a security rating on this ?

Still holding to understand this.

Comment 10

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

I just discussed this with nbp and he mentions that a SIGTRAP crash is likely to be deliberately caused by our engine, and it is likely to be a safe crash.

Perhaps jandem can double-confirm here?

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #10)
> I just discussed this with nbp and he mentions that a SIGTRAP crash is
> likely to be deliberately caused by our engine, and it is likely to be a
> safe crash.
> 
> Perhaps jandem can double-confirm here?

In many cases it is, but here the SIGTRAP is in freed memory. Patch coming up.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Patch

If a script has multiple chunks, and we hit an abort while compiling one of them (in this case because we are trying to inline an uncompilable script), we will release all script code. However, if another chunk's code is still on the stack, we will return to freed memory. The patch calls mjit::Recompiler::clearStackReferences before releasing the code.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 736247 [details] [diff] [review]
Patch

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

Not easy, especially not in versions with IonMonkey enabled. But with some work it's probably possible somehow.

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?

Firefox 9-22 (JM is disabled in 23.)

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

Patch is simple, should apply to older branches.

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

Unlikely to cause regressions.

Comment 14

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

You'll need to nominate the patch for branches.

Comment 15

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

This looks ready for landing.

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #15)
> This looks ready for landing.

Thanks Gary, I will push this today or tomorrow. Not clearing needinfo? flag so that I won't forget about it.

Comment 17

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f74d1d6771ff

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 736247 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): JM
User impact if declined: Security bug, crashes.
Testing completed (on m-c, etc.): On m-i.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f74d1d6771ff

Comment 20

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 21

[bhavana bajaj [:bajaj]]

Comment on attachment 736247 [details] [diff] [review]
Patch

low risk, sec-crit crash regression. Approving on branches.

Can you please prepare the b2g patch needed here and nominate for approval-mozilla-b2g18 : ? Thanks !

Comment 22

[bhavana bajaj [:bajaj]]

(In reply to Jan de Mooij [:jandem] from comment #17)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f74d1d6771ff

Please make sure to land the mozilla-beta patch by tomorrow morning PT as we are targeting beta 4 go-to-build around noon PT.Thank you.

Comment 23

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f98c5b7415c6
https://hg.mozilla.org/releases/mozilla-beta/rev/fc0b144921d0

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 736247 [details] [diff] [review]
Patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): JM
User impact if declined: Security bugs, crashes
Testing completed: On m-c, aurora/beta branches
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/db887846d6fb

Comment 26

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/db887846d6fb

Kinda overlooked the whole b2g18 not having zones thing, though. :fail:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/bc27f5a366b5

Comment 27

[u279076]

Marking status-firefox23:verified based on comment 20.