Closed Bug 855236 Opened 7 years ago Closed 7 years ago

Crash with SIGTRAP

Categories

(Core :: JavaScript Engine, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox20 --- affected
firefox21 + fixed
firefox22 + fixed
firefox23 + verified
firefox-esr17 --- unaffected
b2g18 21+ fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- affected

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update,ignore][adv-main21+])

Attachments

(1 file)

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 $
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
Needinfo from Brian based on comment 1 :)
Flags: needinfo?(bhackett1024)
Also marking s-s since a SIGTRAP could indicate an invalid jump.
Group: core-security
Does the BaselineCompiler change/fix this code? Or is it still a problem?
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision b5cb88ccd907).
(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)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Can we get a security rating on this ?
(In reply to bhavana bajaj [:bajaj] from comment #8)
> Can we get a security rating on this ?

Still holding to understand this.
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?
Flags: needinfo?(bhackett1024) → needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij)
Attached patch PatchSplinter Review
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.
Assignee: general → jdemooij
Status: VERIFIED → REOPENED
Attachment #736247 - Flags: review?(bhackett1024)
Resolution: FIXED → ---
Attachment #736247 - Flags: review?(bhackett1024) → review+
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.
Attachment #736247 - Flags: sec-approval?
Attachment #736247 - Flags: sec-approval? → sec-approval+
You'll need to nominate the patch for branches.
This looks ready for landing.
Flags: needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij)
Flags: needinfo?(jdemooij)
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.
Attachment #736247 - Flags: approval-mozilla-beta?
Attachment #736247 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f74d1d6771ff
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Target Milestone: --- → mozilla23
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
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 !
Attachment #736247 - Flags: approval-mozilla-beta?
Attachment #736247 - Flags: approval-mozilla-beta+
Attachment #736247 - Flags: approval-mozilla-aurora?
Attachment #736247 - Flags: approval-mozilla-aurora+
(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.
Flags: needinfo?(jdemooij)
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
Attachment #736247 - Flags: approval-mozilla-b2g18?
Attachment #736247 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
(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
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main21+]
Marking status-firefox23:verified based on comment 20.
Group: core-security
You need to log in before you can comment on or make changes to this bug.