Last Comment Bug 781859 - Assertion failure: lifetime && lifetime->head == uint32_t(head - outerScript->code) && lifetime->entry == uint32_t(entryTarget - outerScript->code), at methodjit/LoopState.cpp:80
: Assertion failure: lifetime && lifetime->head == uint32_t(head - outerScript-...
Status: VERIFIED FIXED
[js:p1:fx18] [jsbugmon:update,ignore]...
: assertion, regression, sec-critical, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla19
Assigned To: Brian Hackett (:bhackett)
:
:
Mentors:
: 741110 770089 (view as bug list)
Depends on: 720316
Blocks: langfuzz 797163
  Show dependency treegraph
 
Reported: 2012-08-10 10:23 PDT by Christian Holler (:decoder)
Modified: 2013-03-18 13:06 PDT (History)
17 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
+
fixed
+
fixed
+
fixed
unaffected


Attachments
Part 1: Add SRC_DO (10.01 KB, patch)
2012-10-02 08:28 PDT, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Part 2: Emit JSOP_NOP at the top of while loops (10.40 KB, patch)
2012-10-02 08:33 PDT, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Part 3: Add shell function to test the decompiler (3.79 KB, patch)
2012-10-02 09:13 PDT, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch v2 (5.88 KB, patch)
2012-10-02 12:06 PDT, Jan de Mooij [:jandem]
dvander: review+
gary: feedback+
Details | Diff | Splinter Review
patch (730 bytes, patch)
2012-10-22 17:46 PDT, Brian Hackett (:bhackett)
jdemooij: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-08-10 10:23:06 PDT
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.
Comment 1 Christian Holler (:decoder) 2012-08-21 05:26:13 PDT
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
Comment 2 Christian Holler (:decoder) 2012-08-21 05:30:01 PDT
Waldo or Jason, mind taking a look at this?
Comment 3 Jason Orendorff [:jorendorff] 2012-08-21 06:34:23 PDT
sure.
Comment 4 David Bolter [:davidb] 2012-08-22 10:19:20 PDT
Provisionally calling this sec-critical. Always welcome thoughts from those closer to the code.
Comment 5 Jason Orendorff [:jorendorff] 2012-08-22 13:46:14 PDT
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)
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-08-25 17:39:56 PDT
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.
Comment 7 Gary Kwong [:gkw] [:nth10sd] 2012-08-30 13:13:27 PDT
Synchronizing flags with bug 741110 and bug 770089.
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2012-09-27 12:07:42 PDT
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.)
Comment 9 David Bolter [:davidb] 2012-09-28 07:22:45 PDT
Thanks Jeff, it helps (really). Hopefully work on the other two bugs helps illuminate this one.
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2012-10-01 13:58:08 PDT
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.  :-\
Comment 11 Jan de Mooij [:jandem] 2012-10-02 04:21:50 PDT
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.
Comment 12 Jan de Mooij [:jandem] 2012-10-02 08:28:01 PDT
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.
Comment 13 Jan de Mooij [:jandem] 2012-10-02 08:33:16 PDT
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.
Comment 14 Jan de Mooij [:jandem] 2012-10-02 09:13:01 PDT
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.
Comment 15 Jan de Mooij [:jandem] 2012-10-02 09:16:38 PDT
*** Bug 770089 has been marked as a duplicate of this bug. ***
Comment 16 Jan de Mooij [:jandem] 2012-10-02 09:17:01 PDT
*** Bug 741110 has been marked as a duplicate of this bug. ***
Comment 17 Jan de Mooij [:jandem] 2012-10-02 12:06:24 PDT
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.
Comment 20 Gary Kwong [:gkw] [:nth10sd] 2012-10-03 14:08:22 PDT
jandem, do you mind adding the testcase from bug 797163 as well, when you fix up and reland the patch?
Comment 21 Jan de Mooij [:jandem] 2012-10-04 00:30:11 PDT
(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!
Comment 22 Gary Kwong [:gkw] [:nth10sd] 2012-10-08 11:24:26 PDT
Definitely too late for 16.
Comment 23 Jan de Mooij [:jandem] 2012-10-09 11:37:51 PDT
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!
Comment 24 Gary Kwong [:gkw] [:nth10sd] 2012-10-09 15:02:43 PDT
> 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 25 Gary Kwong [:gkw] [:nth10sd] 2012-10-10 14:33:52 PDT
Comment on attachment 667095 [details] [diff] [review]
Patch v2

I didn't see anything here after a day of fuzzing with the patch.
Comment 26 Brian Hackett (:bhackett) 2012-10-22 17:46:37 PDT
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).
Comment 27 Jan de Mooij [:jandem] 2012-10-22 18:27:38 PDT
Comment on attachment 674069 [details] [diff] [review]
patch

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

Nice minimal fix, thanks.
Comment 28 Brian Hackett (:bhackett) 2012-10-23 07:45:53 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d60a2c574f4
Comment 29 Al Billings [:abillings] 2012-10-23 15:25:46 PDT
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.
Comment 30 Alex Keybl [:akeybl] 2012-10-23 16:09:36 PDT
(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.
Comment 31 Christian Holler (:decoder) 2012-10-23 20:00:45 PDT
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 93cc1ee94291).
Comment 32 Ryan VanderMeulen [:RyanVM] 2012-10-23 20:03:18 PDT
https://hg.mozilla.org/mozilla-central/rev/0d60a2c574f4
Comment 33 Christian Holler (:decoder) 2012-10-23 21:35:13 PDT
JSBugMon: This bug has been automatically verified fixed.
Comment 34 David Anderson [:dvander] 2012-10-29 15:00:26 PDT
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:
Comment 35 Gary Kwong [:gkw] [:nth10sd] 2012-10-29 15:18:17 PDT
Needs landing for aurora/beta.
Comment 37 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-06 14:30:24 PST
Is there somewhere I can look at test results so I can mark this verified for Firefox 17, 18, and 19?
Comment 38 Matt Wobensmith [:mwobensmith][:matt:] 2012-11-20 14:42:00 PST
+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.

Note You need to log in before you can comment on or make changes to this bug.