Crash near null with testcase involving mjitChunkLimit

VERIFIED FIXED in Firefox 12

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: gkw, Assigned: bhackett)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
mozilla13
x86
Mac OS X
crash, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 unaffected, firefox12 fixed, firefox13 fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [sg:dos] js-triage-needed)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
mjitChunkLimit(6)
try {
    (function() {
        function a([], x) {
            x.x::c
        }
        a()
    })()
} catch (e) {}

crashes js debug and opt 32-bit shell on m-c changeset 78fde7e54d92 with -m, -a and -n near null.

Locking s-s just-in-case even though this seems like a null deref.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   84929:d0c192e5bd41
user:        Brian Hackett
date:        Wed Jan 18 16:40:18 2012 -0800
summary:     Compile large scripts in chunks, bug 706914. r=dvander

Opt shell stack trace:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x0000000c
0x00f597e1 in ?? ()
(gdb) bt
#0  0x00f597e1 in ?? ()
#1  0x01b0af00 in ?? ()
Previous frame inner to this frame (gdb could not unwind past this frame)
(gdb) x/i $pc
0xf597e1:	mov    0xc(%esi),%ebx
(gdb) x/b $esi
0x0:	Cannot access memory at address 0x0
(gdb) x/b $ebx
0xb390d8:	0x02
(Assignee)

Comment 1

5 years ago
Created attachment 598886 [details] [diff] [review]
patch #1

The closure analysis done for NAME ops could return the current script, which is a problem if the script has not been marked as an outer function as its active call object will not be updated in prologues/epilogues.

This was not caused by chunk compilation, but was induced by it since this situation could not come up with whole method compilation (which would abort on the QNAMECONST and other opcodes which make the frontend throw up its hands in resolving local name accesses).  I'll put together another patch to make sure we do not do any compilation in scripts which contain any uncompileable opcodes (generalizing bug 726799), which should cut off any more strange new cases which could not have occurred before chunked compilation.
Assignee: general → bhackett1024
Attachment #598886 - Flags: review?(dvander)
(Assignee)

Comment 2

5 years ago
Created attachment 599141 [details] [diff] [review]
patch #2 (3b8ad7252ccb)

Patch to ensure no parts of scripts are compiled when they contain uncompileable opcodes.  This is pretty ugly but I think it's a good fail safe to put in and avoid hard-to-fuzz problems with weird rare opcodes.
Attachment #599141 - Flags: review?(dvander)
Attachment #598886 - Flags: review?(dvander) → review+
Attachment #599141 - Flags: review?(dvander) → review+
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0457004daa8c
(Assignee)

Comment 4

5 years ago
Comment on attachment 599141 [details] [diff] [review]
patch #2 (3b8ad7252ccb)

[Approval Request Comment]
User impact if declined: Defensive patch against potential crashes caused by compiling part of a script which would not have been compiled at all before bug 706914.
Risk to taking this patch (and alternatives if risky): Should be low risk, patch changes a fair bit of code but should not change semantics at all. If this is too much for aurora, patch #1 is more restricted (just fixes a crash).
Attachment #599141 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 5

5 years ago
Followup fix to compile scripts containing decomposed opcodes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3651d2e149
(In reply to Brian Hackett (:bhackett) from comment #5)
> Followup fix to compile scripts containing decomposed opcodes.

Will this fix these regressions?
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/CVxkOgplIuE
(Assignee)

Comment 7

5 years ago
(In reply to Ed Morley [:edmorley] from comment #6)
> (In reply to Brian Hackett (:bhackett) from comment #5)
> > Followup fix to compile scripts containing decomposed opcodes.
> 
> Will this fix these regressions?
> https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/
> CVxkOgplIuE

Yeah, it should.  The first push was causing us to not compile large portions of many benchmarks.
(In reply to Brian Hackett (:bhackett) from comment #7)
> Yeah, it should.  The first push was causing us to not compile large
> portions of many benchmarks.

Great :-)

Graphs-new has since confirmed this, eg:
http://graphs-new.mozilla.org/graph.html#tests=[[74,131,15]]&sel=1329852891000,1330025691000&displayrange=7&datatype=running
and
http://graphs-new.mozilla.org/graph.html#tests=[[77,131,15]]&sel=1329852891000,1330025691000&displayrange=7&datatype=running
https://hg.mozilla.org/mozilla-central/rev/0457004daa8c
https://hg.mozilla.org/mozilla-central/rev/bf3651d2e149
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Comment on attachment 599141 [details] [diff] [review]
patch #2 (3b8ad7252ccb)

Drivers decided to take patch 1 for aurora here. Minusing this one, and I'll approve the first patch.
Attachment #599141 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Comment on attachment 598886 [details] [diff] [review]
patch #1

[Triage Comment]
Attachment #598886 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/74efe408346a
(Assignee)

Updated

5 years ago
Target Milestone: mozilla13 → mozilla12

Updated

5 years ago
status-firefox12: --- → fixed
Target Milestone: mozilla12 → mozilla13
status-firefox-esr10: --- → unaffected
status-firefox11: --- → unaffected
status-firefox13: --- → fixed
verified fixed on trunk in nightly js shell.
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.