The default bug view has changed. See this FAQ.

Assertion failure: lifetime && lifetime->head == uint32_t(head - outerScript->code) && lifetime->entry == uint32_t(entryTarget - outerScript->code), at methodjit/LoopState.cpp:80

VERIFIED FIXED in Firefox 17

Status

()

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

People

(Reporter: decoder, Assigned: bhackett)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, 4 keywords)

Trunk
mozilla19
x86_64
Linux
assertion, regression, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15 wontfix, firefox16 wontfix, firefox17+ fixed, firefox18+ fixed, firefox19+ fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [js:p1:fx18] [jsbugmon:update,ignore][adv-track-main17+])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
The following test asserts on mozilla-central revision b5ae446888f5 (options -m -n -a):


function e() {
    try {} catch (e) {
    return (actual = "FAIL");
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
    }
    while (t) continue;
}
e();


I believe this is something different than bug 741110 and bug 770089 since the test looks entirely different and doesn't depend on mjitChunkLimit. S-s due to previous bugs with that assertion being security-relevant.
(Reporter)

Updated

5 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,bisect]
(Reporter)

Updated

5 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(Reporter)

Comment 1

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

The first bad revision is:
changeset:   87695:f4e955f78de9
user:        Jeff Walden
date:        Fri Feb 03 18:53:29 2012 -0800
summary:     Bug 720316 - Use uint32_t indexes for JOF_ATOM opcodes.  r=jorendorff
(Reporter)

Comment 2

5 years ago
Waldo or Jason, mind taking a look at this?
sure.
Assignee: general → jorendorff

Updated

5 years ago
Depends on: 720316
Keywords: regression
Provisionally calling this sec-critical. Always welcome thoughts from those closer to the code.
Keywords: sec-critical
Full assertion is:
    JS_ASSERT(lifetime &&
              lifetime->head == uint32_t(head - outerScript->code) &&
              lifetime->entry == uint32_t(entryTarget - outerScript->code));

lifetime->entry                   is 1507 (points to JSOP_LOOPHEAD)
entryTarget - outerScript->code   is 1506 (points to JSOP_LOOPENTRY)
Assignee: jorendorff → bhackett1024
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
Bug 720316 is likely a bogus dependency, just as the original dependency in bug 770089 was, and this possibly goes back to chunked compilation notwithstanding the absence of mjitChunkLimit from the testcase.  (In effect every script has an implicit |mjitChunkLimit(#)| call in it, actually, for some value of # that I don't know offhand.  This just manages to crash with whatever that default limit is.)  Therefore this might well be a dup of one of the other two bugs, although there's no actual evidence beyond the common assertion -- and, come to see it, this having a try-catch-then-loop structure in common with the testcase for bug 741110.  If there's a dup relationship, and there may well not be one, I bet it's with that bug.
Synchronizing flags with bug 741110 and bug 770089.
status-firefox-esr10: --- → unaffected
status-firefox18: --- → affected
See Also: → bug 741110, bug 770089
Assignee: bhackett1024 → jwalden+bmo
Whiteboard: [jsbugmon:update] → [jsbugmon:update][js:p1:fx18]

Updated

5 years ago
status-firefox15: affected → wontfix
I'm sure critsmash is going to be interested in this bug (and the other bugs with similar-looking assertions) today, so...

Right before he left bhackett said something about thinking the assertion was wrong, but he didn't give any explanation at the time, and I haven't been able to verify that one way or the other yet.  So the short answer is that I don't know yet what's up here.  (And obviously that doesn't apply to the one similar-looking bug where an out-and-out crash was reported.)
Thanks Jeff, it helps (really). Hopefully work on the other two bugs helps illuminate this one.
billm and I looked at this for half an hour or so on Thursday afternoon.  Neither of us understands chunked compilation, so we're pretty much stumbling in the dark on this.  And I have been told chunked compilation -- as done in JM, at least -- doesn't really have things like invariants.  So there's apparently not really a good way to understand how chunked compilation works.

dvander says jandem is the person best positioned to understand JM chunked compilation -- maybe he'd be able to figure this out quickly?  'cause it's going to take me awhile to do this, if I'm to be the one to do it.  :-\
Yeah this is a chunked compilation issue. While-loops have the following bytecode structure:

00000:  goto 6 (+6)
00005:  loophead
--- body ---
00006:  loopentry
--- condition ---
00008:  ifne 5 (-3)
00013:  stop

When splitting the script in chunks, we try to ensure the initial GOTO is in the same chunk as the LOOPHEAD following it. The way this works is if we are at op X, we check if X + 2 is a LOOPHEAD and if so start a new chunk at X + 1 (the GOTO).

This works fine in most cases, except if the op at offset X, before the GOTO, is unreachable/dead. Unreachable ops are skipped so we don't run the check and hence it's possible to start a chunk at the LOOPHEAD, and the GOTO is in a different chunk.

I tried to fix this in MakeJITScript, but it's not easy. A much simpler fix I think is to emit a JSOP_NOP right before the JSOP_GOTO. This is nice anyway because it matches what we emit for other loops.
Assignee: jwalden+bmo → jdemooij
Status: NEW → ASSIGNED
Created attachment 667006 [details] [diff] [review]
Part 1: Add SRC_DO

Once while loops also have a JSOP_NOP, we need a way to distinguish while and do-while loops when we see a JSOP_NOP with SRC_WHILE source note.

This patch adds a separate SRC_DO source note. It also slightly simplifies things since it can store two offsets, instead of using a second source note for the next op.
Attachment #667006 - Flags: review?(jorendorff)
Created attachment 667010 [details] [diff] [review]
Part 2: Emit JSOP_NOP at the top of while loops

Add JSOP_NOP before the initial JSOP_GOTO, and move the source note to it.
Attachment #667010 - Flags: review?(jorendorff)
Created attachment 667022 [details] [diff] [review]
Part 3: Add shell function to test the decompiler

Adds decompileFunctionDeprecated to the shell, I used this to test the decompiler changes in the other patches.
Attachment #667022 - Flags: review?(jorendorff)

Updated

5 years ago
Duplicate of this bug: 770089

Updated

5 years ago
Duplicate of this bug: 741110
Created attachment 667095 [details] [diff] [review]
Patch v2

Jason asked some good questions, which made me realize it's actually simpler to allow a chunk boundary between the GOTO and LOOPHEAD, and fix the few places in the compiler where it assumes otherwise.
Attachment #667006 - Attachment is obsolete: true
Attachment #667010 - Attachment is obsolete: true
Attachment #667022 - Attachment is obsolete: true
Attachment #667006 - Flags: review?(jorendorff)
Attachment #667010 - Flags: review?(jorendorff)
Attachment #667022 - Flags: review?(jorendorff)
Attachment #667095 - Flags: review?(dvander)
Attachment #667095 - Flags: review?(dvander) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f31bcbcdbf4
Backed out for an approx 50% failure rate of Android-no-ion mochitest 8, of the form:
https://tbpl.mozilla.org/php/getParsedLog.php?id=15774715&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=15774814&tree=Mozilla-Inbound

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65404c2b69c4
jandem, do you mind adding the testcase from bug 797163 as well, when you fix up and reland the patch?
(In reply to Gary Kwong [:gkw, :nth10sd] from comment #20)
> jandem, do you mind adding the testcase from bug 797163 as well, when you
> fix up and reland the patch?

Sure I will add it, thanks for the tests!
Blocks: 797163
Definitely too late for 16.
status-firefox16: affected → wontfix
status-firefox19: --- → affected
Comment on attachment 667095 [details] [diff] [review]
Patch v2

gkw or decoder, could one of you fuzz this patch for a while with --no-ion? Reducing the M8 correctness bug will take a lot of time, so it would be very useful if the fuzzers could find a shell testcase in a few minutes. Thanks!
Attachment #667095 - Flags: feedback?(gary)
Attachment #667095 - Flags: feedback?(choller)
> gkw or decoder, could one of you fuzz this patch for a while with --no-ion?

Don't hold your hopes too high from me, I've fuzzed for an hour or two but nothing has showed up yet.
Comment on attachment 667095 [details] [diff] [review]
Patch v2

I didn't see anything here after a day of fuzzing with the patch.
Attachment #667095 - Flags: feedback?(gary) → feedback+
tracking-firefox17: --- → +
tracking-firefox18: --- → +
tracking-firefox19: --- → +
Created attachment 674069 [details] [diff] [review]
patch

This patch seems to work ok, it doesn't skip unreachable opcodes while constructing chunks but just treats them as non-branching.  Passes jit-tests -mna and the three testcases in the comment 17 patch (will include those testcases when pushing).  I'm not sure what's wrong with the comment 17 patch but the code handling loop heads wrt OSR, LICM and loop-carried registers is pretty delicate (why the chunked compilation patch originally opted to not insert chunk boundaries in weird places).
Assignee: jdemooij → bhackett1024
Attachment #674069 - Flags: review?(jdemooij)
Comment on attachment 674069 [details] [diff] [review]
patch

Review of attachment 674069 [details] [diff] [review]:
-----------------------------------------------------------------

Nice minimal fix, thanks.
Attachment #674069 - Flags: review?(jdemooij) → review+

Updated

5 years ago
Attachment #667095 - Flags: feedback?(choller)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d60a2c574f4
This is a sec-high bug. It should have been nominated with the sec-approval flag on the patch and received explicit approval for landing. This is now the policy for all sec-high and sec-critical issues. See posts to dev.planning on this and https://wiki.mozilla.org/Security/Bug_Approval_Process.
(In reply to Brian Hackett (:bhackett) [mostly gone until mid-November] from comment #28)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0d60a2c574f4

If this is low risk enough for uplift to FF17, please nominate for uplift asap.
(Reporter)

Updated

5 years ago
Whiteboard: [jsbugmon:update][js:p1:fx18] → [js:p1:fx18] [jsbugmon:update,ignore]
(Reporter)

Comment 31

5 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 93cc1ee94291).
https://hg.mozilla.org/mozilla-central/rev/0d60a2c574f4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox19: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 33

5 years ago
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 674069 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): chunked compilation (old)
User impact if declined: potential security bug
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): This patch only changes how we ignore unreachable JavaScript code. Low-risk.
String or UUID changes made by this patch:
Attachment #674069 - Flags: approval-mozilla-beta?
Attachment #674069 - Flags: approval-mozilla-aurora?
Attachment #674069 - Flags: approval-mozilla-beta?
Attachment #674069 - Flags: approval-mozilla-beta+
Attachment #674069 - Flags: approval-mozilla-aurora?
Attachment #674069 - Flags: approval-mozilla-aurora+
Needs landing for aurora/beta.
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/59c1b8fe4249
https://hg.mozilla.org/releases/mozilla-beta/rev/33a74acf43f9
Keywords: checkin-needed
status-firefox17: affected → fixed
status-firefox18: affected → fixed
Whiteboard: [js:p1:fx18] [jsbugmon:update,ignore] → [js:p1:fx18] [jsbugmon:update,ignore][adv-track-main17+]
Is there somewhere I can look at test results so I can mark this verified for Firefox 17, 18, and 19?
+1 to Anthony.

QA would love to regress this bug on affected branches. A link to test results OR a working test case that we can run would suffice. Thank you.
Group: core-security
You need to log in before you can comment on or make changes to this bug.