Closed Bug 728342 Opened 12 years ago Closed 12 years ago

Crash near null with testcase involving mjitChunkLimit

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla13
Tracking Status
firefox11 --- unaffected
firefox12 --- fixed
firefox13 --- fixed
firefox-esr10 --- unaffected

People

(Reporter: gkw, Assigned: bhackett1024)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [sg:dos] js-triage-needed)

Attachments

(2 files)

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
Attached patch patch #1Splinter Review
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)
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+
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?
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
(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
Closed: 12 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+
Target Milestone: mozilla13 → mozilla12
Target Milestone: mozilla12 → mozilla13
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.

Attachment

General

Created:
Updated:
Size: