Closed Bug 799185 Opened 12 years ago Closed 12 years ago

IonMonkey: "Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth),"

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox17 --- unaffected
firefox18 + fixed
firefox19 + fixed
firefox20 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: gkw, Assigned: nbp)

References

Details

(4 keywords, Whiteboard: [jsbugmon:update,ignore][ion:p1:fx19][adv-main18-])

Attachments

(4 files, 7 obsolete files)

options('strict') f = (function() { for (var z = 0; z < 9; ++z) { x = z } try { i } catch (x if null) { let e } catch (l) { x.m } }) for (a in f()) {} Download: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-10-08-mozilla-central-debug/jsshell-linux-x86_64.zip This testcase asserts this js debug shell on m-c changeset d9e032542831 without any CLI arguments at Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth),
Guessing related to IonMonkey.
Blocks: IonFuzz
Nicolas also indicated that this assertion may be moderately bad, setting s-s.
Group: core-security
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,reconfirm]
Whiteboard: [jsbugmon:update,reconfirm] → [jsbugmon:update,reconfirm,ignore]
JSBugMon: This bug has been automatically confirmed to be still valid (reproduced on revision aa5e3b445810).
Whiteboard: [jsbugmon:update,reconfirm,ignore] → [jsbugmon:update,ignore]
Due to skipped revisions, the first bad revision could be any of: changeset: 106581:23a84dbb258f user: Alex Crichton date: Wed Jul 18 23:55:55 2012 -0700 summary: Bug 775782 - Instrument pro/epilogue of functions for the SPS profiler in ionmonkey. r=pierron,dvander changeset: 106582:b82fb4d04f60 parent: 106581:23a84dbb258f parent: 100107:d78729026fb9 user: David Anderson date: Mon Jul 23 12:37:49 2012 -0700 summary: Merge from mozilla-central. changeset: 106583:50e28df7ff8f parent: 106582:b82fb4d04f60 parent: 100276:5598b8c4f271 user: David Anderson date: Tue Jul 24 16:32:08 2012 -0700 summary: Merge from mozilla-central. changeset: 106584:7f0f1fdfa5e2 user: Nicolas B. Pierron date: Tue Jul 24 17:24:08 2012 -0700 summary: Bug 767349 - Simulate hidden instructions when the target is hidden. r=luke changeset: 106585:2af804d84437 user: Nicolas Pierron date: Tue Jul 24 17:24:07 2012 -0700 summary: Bug 767349 - Track bad resume points when snapshots are encoded. r=dvander changeset: 106586:eef915d5a18f user: Nicolas B. Pierron date: Tue Jul 24 17:48:47 2012 -0700 summary: Bug 776748 - Do not invalidate ionScript when JM is invalidated. r=dvander changeset: 106587:41f66d0e46b3 user: David Anderson date: Wed Jul 25 02:08:41 2012 -0700 summary: Backed out changeset eef915d5a18f changeset: 106588:d80fbd8493f1 parent: 106587:41f66d0e46b3 parent: 100401:d03aed049b7b user: David Anderson date: Wed Jul 25 14:30:08 2012 -0700 summary: Merge from mozilla-central. changeset: 106589:81146d7c9f51 user: Sean Stangl date: Wed Jul 25 17:10:20 2012 -0700 summary: Bug 777570 - visitMathFunctionD() should be isCall(). r=dvander changeset: 106590:02f44534f7f5 user: Nicolas B. Pierron date: Thu Jul 26 11:17:31 2012 -0700 summary: Bug 776748 - Do not invalidate ionScript when JM is invalidated. r=dvander changeset: 106591:31f9c38e4cb9 parent: 106590:02f44534f7f5 parent: 100585:f528e021ceb1 user: David Anderson date: Thu Jul 26 18:19:02 2012 -0700 summary: Merge from mozilla-central. changeset: 106592:1274d6819bae user: Jan de Mooij date: Fri Jul 27 13:08:24 2012 -0700 summary: Fix hasLazyType assertion (bug 777647, r=dvander). changeset: 106593:ee40f69169e9 user: Jan de Mooij date: Fri Jul 27 13:12:30 2012 -0700 summary: Don't go through GetPcScript to monitor AddValue edge cases (bug 776022, r=dvander). changeset: 106594:ba811ef4de1c user: David Anderson date: Fri Jul 27 14:57:07 2012 -0700 summary: Fix typo in recent merge. changeset: 106595:a21e8bf3531f user: David Anderson date: Fri Jul 27 16:13:02 2012 -0700 summary: Include loop entry types when determining OSR types (bug 774644, r=jandem). changeset: 106596:a9addbf7e526 user: Jan de Mooij date: Tue Jul 24 16:39:17 2012 +0200 summary: [mq]: heur changeset: 106597:db83474903a5 user: David Anderson date: Fri Jul 27 17:16:35 2012 -0700 summary: Backed out changeset a9addbf7e526 changeset: 106598:ae339e63d268 user: David Anderson date: Fri Jul 27 17:17:26 2012 -0700 summary: Backout due to orange. changeset: 106599:83c83b185199 user: Jan de Mooij date: Tue Jul 24 16:39:17 2012 +0200 summary: Implement JSOP_MOD for doubles (bug 716694, r=dvander). changeset: 106600:7a13838698ed user: Shu-yu Guo date: Sun Jul 29 11:52:45 2012 -0700 summary: Refactor |Compile| to be templated and not use fp (bug 773339, r=dvander). changeset: 106601:75f02a17f7cd user: Jan de Mooij date: Mon Jul 30 20:37:14 2012 +0200 summary: Bug 776880 - Fix dropArguments call in CallConstructor to include |this|. r=dvander changeset: 106602:54f9ee5403f0 user: Jan de Mooij date: Mon Jul 30 20:43:44 2012 +0200 summary: No bug - Add Compile to js::ion namespace to fix Clang build. r=dvander changeset: 106603:08187a7ea897 parent: 106602:54f9ee5403f0 parent: 100846:4ca1d7d1d2da user: David Anderson date: Mon Jul 30 13:15:39 2012 -0700 summary: Merge from mozilla-central.
Never compiles for IonMonkey, but it could still be related to IM changes.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
This is a decompiler issue, I added this *strong* assertion. It comes after I modified to check that every snapshot were correctly formated by moving the bailout assertion to the snapshot encoding in the CodeGen. One bug was raised at this time and it appeared that the reconstructPCStack function used by this assertion was wrong on one case. To make sure that the ReconstructPCStack was correct, I added this assertion, which compare the script analysis with the reconstructPCStack. The tricky thing is that the reconstructPCStack must not allocate memory since it is used by the expression decompiler. (as well as the bailout path, even in assertion) The good news is that we have 3 systems to make sure that we are evaluating the same thing. (Interpreter, Script analysis, and the ReconstructPCStack). I would not be surprised if most of the check that I've been adding since I added this assertion are stuff badly handled by the script analysis, but I am not worried for now since as we are not compiling catch blocks yet — where most of the bugs were seen.
Marking 17 affected as Mozilla Central went to 17 before the checkins in comment 4.
Summary: "Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth)," → IonMonkey: "Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth),"
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][ion:p1]
Attached patch Rewrite ReconstructPCStack. (obsolete) — Splinter Review
Once upon a time, a naive developer named Nicolas went to Bailouts swamp and decided to take a toad and bring it back to the home of the CodeGenerator! What a mistake he made. He did not realized that at this time a hidden bug entered the house with it. The toad seems fine in the first few days, but Nicolas' friend, Gary, really enjoy playing with toads! Every time Gary was pressing the toad to see if it was making any noise, a fly came out. Gary went to complain against Nicolas saying that they are to many flies in the room. Nicolas went checking the toad, looked deeply inside, and realized with horror how this poor toad was rotten. After trying for a while to chase the flies away and to heal the toad, no other solution was left to Nicolas, he had to find the potion which will definitively heal the toad and bring it back to life. The potion removed every weak organ of the toad by a new shiny one. … next part of the story coming in Firefox 19 (hopefully not) … Seriously, this patch rewrite the ReconstructPCStack in such a way that we have to go linearly through all opcodes by skipping the minimal set of opcodes, naming the T part of (C ? T : E) expressions when the target is not in T. The good thing is that we can be more confident with the stability of this function, because it goes through more bytecode instead of randomly (workaround-based) skipping branches. As this is not a critical function, and it does not impact much on debug builds performances. I would not recommend trying to optimize it ever again.
Attachment #673099 - Flags: review?(luke)
Attachment #673099 - Flags: review?(gary)
Comment on attachment 673099 [details] [diff] [review] Rewrite ReconstructPCStack. This requires some careful attention and I don't have the time to give it that atm. Do you suppose you could rope in another conspirator? I'm no more qualified than other SM peers on this.
Attachment #673099 - Flags: review?(luke)
Comment on attachment 673099 [details] [diff] [review] Rewrite ReconstructPCStack. Picking a random reviewer, since nobody knows anything about this code.
Attachment #673099 - Flags: review?(sstangl)
Comment on attachment 673099 [details] [diff] [review] Rewrite ReconstructPCStack. r+ based on the fact that it didn't seem to blow up the fuzzers after an hour or two.
Attachment #673099 - Flags: review?(gary) → review+
Attachment #673099 - Flags: review?(sstangl) → review?(jorendorff)
Whiteboard: [jsbugmon:update,ignore][ion:p1] → [jsbugmon:update,ignore][ion:p1:fx19]
Comment on attachment 673099 [details] [diff] [review] Rewrite ReconstructPCStack. Review of attachment 673099 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure how I feel about the AllocateInDebugMode construct. It should be put past the GC people. It's orthogonal to fixing ReconstructPCStack at least, right? Overall, the rewrite is nice. It certainly makes what's going clearer than the old one. Nice comments. I don't know whether it's correct in every case; I'll leave that to time and fuzzing. ::: js/src/gc/Root.h @@ +766,5 @@ > { > JS_ASSERT(!InNoGCScope()); > } > + > +class AllocateInDebugMode { Brace on next line. ::: js/src/jsopcode.cpp @@ +324,5 @@ > * If counts != NULL, include a counter of the number of times each op was executed. > */ > JS_FRIEND_API(JSBool) > +js_DisassembleAtPC(JSContext *cx, JSScript *script_, JSBool lines, > + jsbytecode *pc, bool showAll, Sprinter *sp) Is there a problem with having showAll on all the time? It seems strictly more useful. @@ +6454,5 @@ > > LOCAL_ASSERT(script->code <= target && target < script->code + script->length); > jsbytecode *pc = script->code; > unsigned pcdepth = 0; > + /* Space. @@ +6456,5 @@ > jsbytecode *pc = script->code; > unsigned pcdepth = 0; > + /* > + * PC Depth counter used to save the state of hidden instructions to recover > + * the depth if a branch is not taken. Nice CPP comments? Here and below. @@ +6462,3 @@ > unsigned hpcdepth = unsigned(-1); > + bool breakFlow = false; > + /* Space. @@ +6469,5 @@ > ptrdiff_t oplen; > + > + /* > + * The condition stop later than expected but replace an infinite loop by an > + * assertion in case of missing break. I don't understand this sentence. @@ +6479,3 @@ > > + if (cs->format & JOF_DECOMPOSE) { > + if (pc >= target) When is there going to be a decomposing opcode not followed by anything? Assert it. @@ +6505,4 @@ > */ > + if (op == JSOP_THROWING || op == JSOP_THROW) { > + /* > + * The current catch block is conditional, we compensate the "compensate for" @@ +6507,5 @@ > + /* > + * The current catch block is conditional, we compensate the > + * "pop" added after the "ifeq" by adding 1 to the catch > + * pcdepth. This fake slot might not contain the right > + * value, but it would be eliminated with the next throw or "it will" @@ +6520,4 @@ > > + breakFlow = false; > + if (op == JSOP_RETRVAL || op == JSOP_GOTO || op == JSOP_GOSUB || > + op == JSOP_NOP || op == JSOP_THROW) Do you need JSOP_RETSUB here? @@ +6544,5 @@ > + hpcdepth = pcdepth; > + } else if (sn && SN_TYPE(sn) == SRC_CATCH) { > + /* > + * Continue after hidden instructions. Discard saved depth. Do not > + * assert to handle the case of a second catch statements which is assert what? @@ +6552,5 @@ > hpcdepth = unsigned(-1); > > + /* > + * Save catch depth to restore the stack for the next catch. This > + * path would be used by enterblock and leaveblock to add/remove let "is used by" @@ +6582,3 @@ > > + /* Continue until we reach the target. */ > + if (pc >= target) How can pc > target happen? @@ +6601,5 @@ > */ > + if (sn && SN_TYPE(sn) == SRC_COND) { > + ptrdiff_t jmplen = GET_JUMP_OFFSET(pc); > + if (pc + jmplen <= target) { > + /* Target does not lies in T. */ "lies" -> "lie"
Attachment #673099 - Flags: feedback+
Comment on attachment 673099 [details] [diff] [review] Rewrite ReconstructPCStack. Terrence, can you review the AllocateInDebugMode class of this patch, which is needed for calling debug functions from gdb. Is there a way to prevent GC as well ?
Attachment #673099 - Flags: review?(terrence)
Comment on attachment 673099 [details] [diff] [review] Rewrite ReconstructPCStack. Review of attachment 673099 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/Root.h @@ +766,5 @@ > { > JS_ASSERT(!InNoGCScope()); > } > + > +class AllocateInDebugMode { (1) Wow, that is just incredibly poorly named. I thought that I was going to be reviewing a new allocation function, not a scoped assertion. (2) What is this for and how can it possibly be valid to add this?
(In reply to Terrence Cole [:terrence] from comment #14) > Comment on attachment 673099 [details] [diff] [review] > Rewrite ReconstructPCStack. > > Review of attachment 673099 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/gc/Root.h > @@ +766,5 @@ > > { > > JS_ASSERT(!InNoGCScope()); > > } > > + > > +class AllocateInDebugMode { > > (1) Wow, that is just incredibly poorly named. Thanks, a constructive remark would have been better. > I thought that I was going > to be reviewing a new allocation function, not a scoped assertion. This is not a scoped assertion, this is a MuteAllGCAssertionsInDebugFunction. > (2) What is this for and how can it possibly be valid to add this? because it is surrounded by a #if DEBUG, and because it should only be used by debug (gdb) functions. I don't care if it is occasionally crashing in debug mode, because at this point I am desperate and an insane work-around is enough to get a damn debug function working under gdb.
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #15) > > (1) Wow, that is just incredibly poorly named. > > Thanks, a constructive remark would have been better. Yeah, sorry. I tried to think of something better, but I have no idea what the purpose is. > > I thought that I was going > > to be reviewing a new allocation function, not a scoped assertion. > > This is not a scoped assertion, this is a MuteAllGCAssertionsInDebugFunction. It modifies a scoped assertion. How about |AutoMuteAllGCAssertionsAndMaybeCrash|. > > (2) What is this for and how can it possibly be valid to add this? > > because it is surrounded by a #if DEBUG, and because it should only be used > by debug (gdb) functions. I don't care if it is occasionally crashing in > debug mode, because at this point I am desperate and an insane work-around > is enough to get a damn debug function working under gdb. There are two levels of #ifdef DEBUG here then: normally, we don't want to corrupt the heap, even in #ifdef DEBUG blocks. I'm not entirely clear on why we need to check this in rather than just reimplementing it when we need it to do crazy things in gdb?
(In reply to Terrence Cole [:terrence] from comment #16) > (In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #15) > > > I thought that I was going > > > to be reviewing a new allocation function, not a scoped assertion. > > > > This is not a scoped assertion, this is a MuteAllGCAssertionsInDebugFunction. > > It modifies a scoped assertion. How about > |AutoMuteAllGCAssertionsAndMaybeCrash|. Sounds fine to me. > > > (2) What is this for and how can it possibly be valid to add this? > > > > because it is surrounded by a #if DEBUG, and because it should only be used > > by debug (gdb) functions. I don't care if it is occasionally crashing in > > debug mode, because at this point I am desperate and an insane work-around > > is enough to get a damn debug function working under gdb. > > There are two levels of #ifdef DEBUG here then: normally, we don't want to > corrupt the heap, even in #ifdef DEBUG blocks. I'm not entirely clear on > why we need to check this in rather than just reimplementing it when we need > it to do crazy things in gdb? Because when this bug appear only in a mochi-test, it is way more practical to have this enabled in firefox and usable in gdb. The fact that the class only exists in Debug mode implies that it will cause a compilation failure in optimized builds. Is there a way to prevent any GC within a scope?
(In reply to Benjamin Peterson [:benjamin] from comment #12) > Comment on attachment 673099 [details] [diff] [review] > Rewrite ReconstructPCStack. > > Review of attachment 673099 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm not sure how I feel about the AllocateInDebugMode construct. It should > be put past the GC people. It's orthogonal to fixing ReconstructPCStack at > least, right? Right, but it is a needed trick to debug ReconstructPCStack. > ::: js/src/jsopcode.cpp > @@ +324,5 @@ > > * If counts != NULL, include a counter of the number of times each op was executed. > > */ > > JS_FRIEND_API(JSBool) > > +js_DisassembleAtPC(JSContext *cx, JSScript *script_, JSBool lines, > > + jsbytecode *pc, bool showAll, Sprinter *sp) > > Is there a problem with having showAll on all the time? It seems strictly > more useful. Simple, I don't want to change the output expected by other developers which are using other wrapper of this function for debugging. (DumpScript, DumpPC, …) > @@ +6469,5 @@ > > ptrdiff_t oplen; > > + > > + /* > > + * The condition stop later than expected but replace an infinite loop by an > > + * assertion in case of missing break. > > I don't understand this sentence. /* * Stop beyond the target, this replace an infinite loop by an assertion in * case of missing break. */ better? > @@ +6479,3 @@ > > > > + if (cs->format & JOF_DECOMPOSE) { > > + if (pc >= target) > > When is there going to be a decomposing opcode not followed by anything? > Assert it. The problem is not that it is followed by something or not, just that this is the targeted pc and we should not start it's emulation. IonMonkey assertion use this pc for encoding snapshots. > @@ +6520,4 @@ > > > > + breakFlow = false; > > + if (op == JSOP_RETRVAL || op == JSOP_GOTO || op == JSOP_GOSUB || > > + op == JSOP_NOP || op == JSOP_THROW) > > Do you need JSOP_RETSUB here? No because JSOP_RETSUB can safely continue after (AFAIU), try { if (…) return; // RETSUB will return after the finally. …; // RETSUB will continue after the finally. } finally { … // RETSUB emitted at the end of the finally. } > @@ +6544,5 @@ > > + hpcdepth = pcdepth; > > + } else if (sn && SN_TYPE(sn) == SRC_CATCH) { > > + /* > > + * Continue after hidden instructions. Discard saved depth. Do not > > + * assert to handle the case of a second catch statements which is > > assert what? Do not assert the hidden depth is unset to handle … 20 00001 00039: 5 leaveblock 1 11 00000 00042: 5 goto 82 (+40) 11 00002 00047: 5 throwing 11 00001 00048: 5 leaveblock 1 20 00000 00051: 6 enterblock depth 0 {d: 0} <-- reset hpc before simulating this instruction. Otherwise, we could assert that hpcdepth is unset. The tricky part is that leaveblock is not really an expression which is breaking any flow. > @@ +6582,3 @@ > > > > + /* Continue until we reach the target. */ > > + if (pc >= target) > > How can pc > target happen? It should not happen, but I prefer having the assertion at the end of the loop than an infinite loop.
Comment on attachment 673099 [details] [diff] [review] Rewrite ReconstructPCStack. Review of attachment 673099 [details] [diff] [review]: ----------------------------------------------------------------- Okay, more constructive comments inline. I'd like to have another look once you apply them. ::: js/src/gc/Root.h @@ +766,5 @@ > { > JS_ASSERT(!InNoGCScope()); > } > + > +class AllocateInDebugMode { Okay, I talked it over with Bill and |AutoMuteGCRegionAssertionsAndMaybeCrash| is an okay name for this. (1) We also need an appropriately dire comment here. (2) I also would like these to be SpiderMonkey only -- i.e. try moving these definitions to jsgc.h. (3) Bill also wants to add an #ifndef DEBUG #error "..." #endif block to the constructor so that this never gets added to opt builds. (4) We would also like a function - |MuteGCRegionAssertionsForInteractiveDebug| - which is available in DEBUG and just sets gcAssertNoGCDepth to 0.
Attachment #673099 - Flags: review?(terrence)
Nicolas, what rating would you give this bug? From comment 6 it maybe doesn't sound too bad, but maybe I am not understanding? https://wiki.mozilla.org/Security_Severity_Ratings
Flags: needinfo?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #18) > (In reply to Benjamin Peterson [:benjamin] from comment #12) > > ::: js/src/jsopcode.cpp > > @@ +324,5 @@ > > > * If counts != NULL, include a counter of the number of times each op was executed. > > > */ > > > JS_FRIEND_API(JSBool) > > > +js_DisassembleAtPC(JSContext *cx, JSScript *script_, JSBool lines, > > > + jsbytecode *pc, bool showAll, Sprinter *sp) > > > > Is there a problem with having showAll on all the time? It seems strictly > > more useful. > > Simple, I don't want to change the output expected by other developers which > are using other wrapper of this function for debugging. (DumpScript, DumpPC, > …) I think it should be called something more specific than |showAll| then. > > > @@ +6469,5 @@ > > > ptrdiff_t oplen; > > > + > > > + /* > > > + * The condition stop later than expected but replace an infinite loop by an > > > + * assertion in case of missing break. > > > > I don't understand this sentence. > > /* > * Stop beyond the target, this replace an infinite loop by an assertion > in > * case of missing break. > */ > > better? "This means a missing break will result in an assertion instead of an infinite loop." > > > @@ +6479,3 @@ > > > > > > + if (cs->format & JOF_DECOMPOSE) { > > > + if (pc >= target) > > > > When is there going to be a decomposing opcode not followed by anything? > > Assert it. > > The problem is not that it is followed by something or not, just that this > is the targeted pc and we should not start it's emulation. IonMonkey > assertion use this pc for encoding snapshots. Okay. > > > @@ +6544,5 @@ > > > + hpcdepth = pcdepth; > > > + } else if (sn && SN_TYPE(sn) == SRC_CATCH) { > > > + /* > > > + * Continue after hidden instructions. Discard saved depth. Do not > > > + * assert to handle the case of a second catch statements which is > > > > assert what? > > Do not assert the hidden depth is unset to handle … > > 20 00001 00039: 5 leaveblock 1 > 11 00000 00042: 5 goto 82 (+40) > 11 00002 00047: 5 throwing > 11 00001 00048: 5 leaveblock 1 > 20 00000 00051: 6 enterblock depth 0 {d: 0} <-- reset hpc before > simulating this instruction. > > Otherwise, we could assert that hpcdepth is unset. The tricky part is that > leaveblock is not really an expression which is breaking any flow. You could put this nice explanation in the comment there. :) > > > @@ +6582,3 @@ > > > > > > + /* Continue until we reach the target. */ > > > + if (pc >= target) > > > > How can pc > target happen? > > It should not happen, but I prefer having the assertion at the end of the > loop than an infinite loop. Okay.
Attached patch Rewrite ReconstructPCStack. (obsolete) — Splinter Review
Update the previous patch (gary r+, benjamin f+) with benjamin and terrence nits.
Attachment #673099 - Attachment is obsolete: true
Attachment #673099 - Flags: review?(jorendorff)
Attachment #677132 - Flags: review?(terrence)
Attachment #677132 - Flags: review?(jorendorff)
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 677132 [details] [diff] [review] Rewrite ReconstructPCStack. Review of attachment 677132 [details] [diff] [review]: ----------------------------------------------------------------- Perfect.
Attachment #677132 - Flags: review?(terrence) → review+
(In reply to Andrew McCreight [:mccr8] from comment #20) > Nicolas, what rating would you give this bug? From comment 6 it maybe > doesn't sound too bad, but maybe I am not understanding? > https://wiki.mozilla.org/Security_Severity_Ratings In a release build, this path is only used by the expression decompiler which is used for producing JS error message (so predictable) and allow somebody to read some unknown memory location. In practice, it seems difficult to exploit this piece of code to accumulate shift errors reported by the current assertion in order to read a precise memory location. If this *ever* happen in the wild, the most likely effect would be either a wrong error message or a crash. “Any crash where random memory is accessed” => sec-critical
Keywords: sec-critical
Comment on attachment 677132 [details] [diff] [review] Rewrite ReconstructPCStack. Review of attachment 677132 [details] [diff] [review]: ----------------------------------------------------------------- Phew. Good patch; I'm not sure if I've found any bugs or not, but there are parts I'm worried about. I only reviewed the parts in jsopcode.cpp. Clearing review flag for now; ping me again on a revision. ::: js/src/jsopcode.cpp @@ +320,5 @@ > } > > /* > * If pc != NULL, include a prefix indicating whether the PC is at the current line. > * If counts != NULL, include a counter of the number of times each op was executed. While you're here, please remove the comment about counts. @@ +6449,1 @@ > * operand-generating opcode PCs in pcstack. The line just before this one says "Walk forward from script->main", but it starts from script->code actually. Please fix the comment. @@ +6473,5 @@ > + /* > + * Stop beyond the target, this replace an infinite loop by an assertion in > + * case of missing break. > + */ > + for (; pc <= target; pc += oplen) { I still don't understand this comment. :) I think it should say <, not <=. I don't understand about the missing break. @@ +6489,5 @@ > + > + if (breakFlow) { > + /* > + * Restore the hidden depth for a non-hidden instruction comming > + * after we broke the flow. spelling: "coming". @@ +6496,4 @@ > if (hpcdepth != unsigned(-1)) { > pcdepth = hpcdepth; > hpcdepth = unsigned(-1); > } Why *not* set pcdepth in the case where a hidden instruction immediately follows a break? If you set hpcdepth to unsigned(-1) here and the current instruction is hidden, hpcdepth will be reset correctly below, I think. I don't understand the reason for the conditional; doesn't it cause pcdepth to be very wrong, just for a few instructions, in cases like while (a) { let x; if (b) { if (c) { d(); break; // hidden LEAVEBLOCK, then GOTO } break; // another hidden LEAVEBLOCK, then GOTO } } I think you could cause pcdepth to go negative by adding more let-variables. @@ +6507,1 @@ > */ Clearer, I think: /* * JSOP_THROWING and JSOP_THROW appear immediately after a break only * after a conditional catch-block, in the path where the condition is * false. */ That's what is going on here, right? I am a little worried about correctness here (see below). @@ +6512,5 @@ > + * pcdepth. This fake slot might not contain the right value, > + * but it will be eliminated with the next throw or throwing > + * instruction. > + */ > + if (cpcdepth != unsigned(-1)) { Instead of using (cpcdepth != unsigned(-1)) as an if-condition, can't we assert it? @@ +6514,5 @@ > + * instruction. > + */ > + if (cpcdepth != unsigned(-1)) { > + pcdepth = cpcdepth + 1; > + cpcdepth = unsigned(-1); This is what I'm worried about. What if several catch-blocks are nested? It seems like the inner try/catch will clobber the cpcdepth for the outer try/catch. @@ +6522,5 @@ > > + breakFlow = false; > + if (op == JSOP_RETRVAL || op == JSOP_GOTO || op == JSOP_GOSUB || > + op == JSOP_NOP || op == JSOP_THROW) > + { A suggestion: breakFlow = (op == JSOP_RETRVAL || op == JSOP_GOTO || op == JSOP_GOSUB || op == JSOP_NOP || op == JSOP_THROW); @@ +6527,2 @@ > /* > + * We are continuing after an unconditionnal jump, potentially out spelling: "unconditional" @@ +6540,5 @@ > + * Hidden instructions annotate early-exit paths and do not affect > + * the stack depth when not taken. However, when the target is in an > + * early-exit path, hidden instructions need to be taken into > + * account. Save the depth before hidden instruction to restore it > + * if the early-exit path is not taken. Change the last line to read: "if target is not in the early-exit path." @@ +6555,1 @@ > hpcdepth = unsigned(-1); SRC_CATCH appears on JSOP_ENTERBLOCK and JSOP_LEAVEBLOCK opcodes. When SRC_CATCH appears on a LEAVEBLOCK op, js_GetSrcNoteOffset(sn, 0) is conveniently the stack depth after leaving the block. Can we use that instead of cpcdepth? Here are the three cases I know about: 1 leaveblock +SRC_CATCH ;; end of standard catch-block goto next_statement +SRC_HIDDEN nop +SRC_ENDBRACE 2 leaveblock +SRC_CATCH ;; end of a conditional catch-block goto next_statement +SRC_HIDDEN throwing +SRC_HIDDEN ;; followed by another catch-block leaveblock +SRC_HIDDEN 3 leaveblock +SRC_CATCH ;; end of conditional catch-block goto next_statement +SRC_HIDDEN throw +SRC_HIDDEN ;; with no more catch-blocks nop +SRC_ENDBRACE It looks like in all three cases we can just set hpcdepth from the source note offset and forget about it, so this way may be short on code (and long on comments). @@ +6582,3 @@ > */ > + JS_ASSERT_IF(script->hasAnalysis() && script->analysis()->maybeCode(pc), > + script->analysis()->getCode(pc).stackDepth == pcdepth); If using <= in the loop condition is meant to preserve this assertion, you could instead just factor it out into a function and call it again after the loop. static void AssertPCDepth(JSScript *script, jsbytecode *pc, unsigned pcdepth) { JS_ASSERT_IF(...); } But I think the <= also causes pcdepth to be modified in certain cases, which frightens me a little bit. If you'd like to explain that a little better, I'd be happy to listen. :) @@ +6599,5 @@ > + * 09 001 pc : ifeq +11 > + * 000 pc+ 5: null > + * 001 pc+ 6: goto +... <-- The stack after the goto > + * 000 pc+11: ... <-- does not have the same depth. > + * Greatly improved comment and code here. I like it.
Attachment #677132 - Flags: review?(jorendorff)
(In reply to Jason Orendorff [:jorendorff] from comment #25) > ::: js/src/jsopcode.cpp > @@ +6473,5 @@ > > + /* > > + * Stop beyond the target, this replace an infinite loop by an assertion in > > + * case of missing break. > > + */ > > + for (; pc <= target; pc += oplen) { > > I still don't understand this comment. :) I think it should say <, not <=. > I don't understand about the missing break. I removed the comment because it seems to confuse everybody, and replace the condition to be '<' instead of '<='. > @@ +6496,4 @@ > > if (hpcdepth != unsigned(-1)) { > > pcdepth = hpcdepth; > > hpcdepth = unsigned(-1); > > } > > Why *not* set pcdepth in the case where a hidden instruction immediately > follows a break? > > If you set hpcdepth to unsigned(-1) here and the current instruction is > hidden, hpcdepth will be reset correctly below, I think. No because we have one hidden instruction for each let-block that we exit, so we cannot restore it after each hidden instruction because we have to keep it as it was before the first hidden instruction while simulating the pop done by previous hidden instructions. sn stack loc line op -- ----- ----- ---- -- 01 00007 00330: 29 ifeq 351 (+21) // if … 11 00006 00335: 29 leaveblock 4 // catch ({c,d,e,f}) finally 11 00002 00338: 29 leaveblock 2 // catch ({a,b}) {} 11 00000 00341: 29 gosub 399 (+58) // --> jump to the finally 01 00000 00346: 29 goto 409 (+63) 00006 00351: 30 getlocal 4 > I don't understand the reason for the conditional; doesn't it cause pcdepth > to be very wrong, just for a few instructions, in cases like > > while (a) { > let x; > if (b) { > if (c) { > d(); > break; // hidden LEAVEBLOCK, then GOTO > } > break; // another hidden LEAVEBLOCK, then GOTO > } > } > > I think you could cause pcdepth to go negative by adding more let-variables. Good catch ! (gdb) call js_DumpScriptDepth(cx, script, pc) _build/bug799185-6.js:1 sn stack loc line op -- ----- ----- ---- -- main: … 11 00001 00042: 7 leaveblock 1 01 00000 00045: 7 goto 70 (+25) --> 11 00001 00050: 9 leaveblock 1 01 00000 00053: 9 goto 70 (+17) 00001 00058: 9 leaveblock 1 … > @@ +6507,1 @@ > > */ > > Clearer, I think: > > /* > * JSOP_THROWING and JSOP_THROW appear immediately after a break only > * after a conditional catch-block, in the path where the condition > is > * false. > */ > > That's what is going on here, right? > > I am a little worried about correctness here (see below). > > @@ +6512,5 @@ > > + * pcdepth. This fake slot might not contain the right value, > > + * but it will be eliminated with the next throw or throwing > > + * instruction. > > + */ > > + if (cpcdepth != unsigned(-1)) { > > Instead of using (cpcdepth != unsigned(-1)) as an if-condition, can't we > assert it? > > @@ +6514,5 @@ > > + * instruction. > > + */ > > + if (cpcdepth != unsigned(-1)) { > > + pcdepth = cpcdepth + 1; > > + cpcdepth = unsigned(-1); > > This is what I'm worried about. > > What if several catch-blocks are nested? It seems like the inner try/catch > will clobber the cpcdepth for the outer try/catch. cpcdepth save the input depth of the leaveblock instruction before any hidden instructions and we need to restore it before emulating JSOP_THROW or JSOP_THROWING. > @@ +6555,1 @@ > > hpcdepth = unsigned(-1); > > SRC_CATCH appears on JSOP_ENTERBLOCK and JSOP_LEAVEBLOCK opcodes. > > When SRC_CATCH appears on a LEAVEBLOCK op, js_GetSrcNoteOffset(sn, 0) is > conveniently the stack depth after leaving the block. Can we use that > instead of cpcdepth? Here are the three cases I know about: > > 1 > leaveblock +SRC_CATCH ;; end of standard catch-block > goto next_statement +SRC_HIDDEN > nop +SRC_ENDBRACE > > 2 > leaveblock +SRC_CATCH ;; end of a conditional catch-block > goto next_statement +SRC_HIDDEN > throwing +SRC_HIDDEN ;; followed by another catch-block > leaveblock +SRC_HIDDEN > > 3 > leaveblock +SRC_CATCH ;; end of conditional catch-block > goto next_statement +SRC_HIDDEN > throw +SRC_HIDDEN ;; with no more catch-blocks > nop +SRC_ENDBRACE > > It looks like in all three cases we can just set hpcdepth from the source > note offset and forget about it, so this way may be short on code (and long > on comments). If I understand well, you suggest to use the hpcdepth for the leaveblock annotated with a SRC_CATCH, and restore it for JSOP_THROW and JSOP_THROWING. If we do so, 1. We might have to check for the SRC_ENDBRACE to avoid restoring it. 2. Do not consider the hidden leaveblock after a throw. 3. (nothing). I prefer having more code and having one variable dedicated to each case. This sounds easier to understand, and I think this function is the root of all evil[1]. Especially since this function is already hard to understand and it does not need performance improvement. > @@ +6582,3 @@ > > */ > > + JS_ASSERT_IF(script->hasAnalysis() && script->analysis()->maybeCode(pc), > > + script->analysis()->getCode(pc).stackDepth == pcdepth); > > If using <= in the loop condition is meant to preserve this assertion, you > could instead just factor it out into a function and call it again after the > loop. > > static void AssertPCDepth(JSScript *script, jsbytecode *pc, unsigned > pcdepth) { > JS_ASSERT_IF(...); > } > > But I think the <= also causes pcdepth to be modified in certain cases, > which frightens me a little bit. If you'd like to explain that a little > better, I'd be happy to listen. :) This is some legacy, where I forgot a continue in the middle of the function when the breakFlow flag was set, which caused an infinite loop. But I guess, that based on the number of break/continue, we can safely infer that we won't have any more infinite loop. So I replaced the '<=' condition by a '<'. [1] http://en.wikiquote.org/wiki/Donald_Knuth I will split the patch to have one for the debug functions and one for the ReconstructPCStack, and I will add an additional patch containing all test cases, which would hopefully land soon after.
I didn't understand how cpcdepth was working. It makes sense to me now.
Attached patch Rewrite ReconstructPCStack. (obsolete) — Splinter Review
This patch only include changes related to ReconstructPCStack. - It now handles correctly break and continue as reported in the previous comment. - Applied nits. Decoder & gkw, I am looking for some hard fuzzing on this patch, One way to trigger *some* issues of ReconstructPCStack is by (1) using the expression decompiler: for (var a = 0; a < 0; a++) { … anything your fuzzer can produce … } // Trigger the expression decompiler which iterate linearly // on the bytecode, including the unused for loop. try { null.x; } catch (x) {} This technique does not depend on any shell flag (should reproduce with any shell flag), but it needs a *non-deterministic* build to continue inside the expression decompiler. or by (2) using ion-eager on function without try blocks, such as ion can generate snapshots and check them by using reconstructPCStack. AFAIK, these are to 2 known way to reach reconstructPCStack. Some issue reported by (2) might not be visible at all with (1), because IonMonkey is checking with any kind of PC where the expression decompiler only target a subset.
Attachment #677132 - Attachment is obsolete: true
Attachment #678506 - Flags: review?(jorendorff)
Attachment #678506 - Flags: review?(gary)
Attachment #678506 - Flags: review?(choller)
This patch is extracted from the previous ReconstructPCStack patch. It change the paradigm of the debug function such as it does not introduce a security risk which might cause a crash if it is badly used (only prevent GC within a scope). This patch does not need to be back-ported and does not represent any security issue.
Attachment #678507 - Flags: review?(terrence)
Attached patch Test cases. (obsolete) — Splinter Review
This patch contains all issues encountered during the re-development of ReconstructPCStack. Some test case might work with the previous version of reconstructPCStack, but not all of them. These tests highlight potential security issues which might be exploitable on fx17.
Attached patch Test cases. (obsolete) — Splinter Review
Add missing test header.
Attachment #678509 - Attachment is obsolete: true
Comment on attachment 678507 [details] [diff] [review] Improve debug function for ReconstructPCStack. Review of attachment 678507 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for re-requesting review. I'd like a few names and comments changed, r+ with those applied. ::: js/src/jscntxt.cpp @@ +655,5 @@ > * out-of-memory error by a script-catchable exception. > */ > cx->clearPendingException(); > if (onError) { > + AutoAtomicIncrement incr(&cx->runtime->mayAllocInNoGCScope); I conflicted you today in my atomic removal patch. Sorry. ::: js/src/jscntxt.h @@ +926,5 @@ > size_t noGCOrAllocationCheck; > #endif > > /* > + * To ensure that cx->malloc does not cause a GC. If a GC is requested, we Hmm. This comment is misleading (as it existed already): cx->malloc cannot cause GC directly in any case. Really this keeps us from entering a GC for whatever reason. Can we change this to: "When this flag is non-zero, any attempt to GC will be skipped. It is used to suppress GC when reporting an OOM (see js_ReportOutOfMemory) and in debugging facilities that cannot tolerate a GC and would rather OOM immediately, such as utilities exposed to GDB. Setting this flag is extremely dangerous and should only be used when in an OOM situation or in non-exposed debugging facilities." @@ +935,2 @@ > */ > + int32_t mayAllocInNoGCScope; Writing the above comment suggested a better name to me. Please change this to |suppressGC| to accurately reflect its function. ::: js/src/jsgc.cpp @@ +5636,5 @@ > +#ifdef DEBUG > + > +/* Should only be called manually under gdb */ > +static > +void PreventGCOnAllocationDuringInteractiveDebug() Hmmm, the on-allocation is probably overkill. I think |PreventGCDuringInteractiveDebug| will probably be adequate. ::: js/src/jsgc.h @@ +1168,5 @@ > #endif > > +/* > + * Should only be instanciated in function only called via gdb and which are not > + * reachable through any ABI call. Can we change this comment to: "Instances of this class set the |JSRuntime::suppressGC| flag for the duration that they are live. Use of this class is highly discouraged. Please carefully read the comment in jscntxt.h above |suppressGC| and take all appropriate precautions before instantiating this class." @@ +1170,5 @@ > +/* > + * Should only be instanciated in function only called via gdb and which are not > + * reachable through any ABI call. > + */ > +class AutoMuteNoGCAssertions Now that jsgc.h is no longer public API, we can add crazy stuff to it without keeping me up at night. Please call this AutoSuppressGC, and use it in js_ReportOutOfMemory too. ::: js/src/jsopcode.cpp @@ +325,3 @@ > */ > JS_FRIEND_API(JSBool) > +js_DisassembleAtPC(JSContext *cx, JSScript *script_, JSBool lines, Please change script_ to scriptArg to match our current style. @@ +329,1 @@ > { Please add an |AssertCanGC();| here to ensure this is not called in an AutoAssertNoGC scope in places that don't suppress GC.
Attachment #678507 - Flags: review?(terrence) → review+
function testBitOrInconvertibleObjectInconvertibleObject() { var o1 = {}; var count2 = 0; function toString2() { ++count2; if (count2 == 95) return {}; } var o2 = { toString: toString2 }; try { for (var i = 0; i < 100; i++) var q = o1 | o2; } catch (e) { if (i !== 94) return gc(); this.bar.foo ^ this } } testBitOrInconvertibleObjectInconvertibleObject() Assertion failure: script->analysis()->getCode(pc).stackDepth == pcdepth, at /srv/repos/mozilla-central/js/src/jsopcode.cpp:6423
Attached patch Rewrite ReconstructPCStack. (obsolete) — Splinter Review
Delta: - Handle return and throw in the same way as break & continue.
Attachment #678506 - Attachment is obsolete: true
Attachment #678506 - Flags: review?(jorendorff)
Attachment #678506 - Flags: review?(gary)
Attachment #678506 - Flags: review?(choller)
Attachment #678980 - Flags: review?(jorendorff)
Attachment #678980 - Flags: review?(gary)
Attachment #678980 - Flags: review?(choller)
Delta: - Add "areGCEnabled" and use it inside AssertCanGC() and in AutoAssertNoGC. - (no merge conflict on AutoAtomicIncrement yet)
Attachment #678983 - Flags: review?(terrence)
Comment on attachment 678983 [details] [diff] [review] Improve debug function for ReconstructPCStack. Review of attachment 678983 [details] [diff] [review]: ----------------------------------------------------------------- This should be fine with comments addressed. ::: js/src/gc/Root.h @@ +81,5 @@ > > JS_FRIEND_API(void) EnterAssertNoGCScope(); > JS_FRIEND_API(void) LeaveAssertNoGCScope(); > JS_FRIEND_API(bool) InNoGCScope(); > +JS_FRIEND_API(bool) areGCEnabled(); This should be |isGCEnabled|. The verb "are" is plural, so it needs a plural target, which would make it areGCsEnabled. On one hand, this is fine: it is viewing GC as a process that can be skipped. However, this reads quite oddly since the standard convention is to use the singular "is" for boolean-style checks. I think it reads better with "GC" as a module of code that is enabled or disabled: e.g. isGCEnabled. Besides, even in the prior case, we effectively only care about the first GC, so isGCEnabled makes sense for that as well. Secondly, I'd prefer if we didn't have to expose this as friend API. Does it still work if we remove the FRIEND annotation since it's not called externally? Probably not, since we're inlining the users. Alternatively, it should be possible to forward declare it /inside/ the methods that use it. If neither of those work please add a comment like: "This is an internal state of the GC that is exposed for inlining purposes." ::: js/src/jsgc.cpp @@ +4421,5 @@ > for (CompartmentsIter c(rt); !c.done(); c.next()) > JS_ASSERT_IF(rt->gcMode == JSGC_MODE_GLOBAL, c->isGCScheduled()); > #endif > > /* Don't GC if we are reporting an OOM. */ Please update this comment to include "or in an interactive debugging session." ::: js/src/jsgc.h @@ +1174,5 @@ > + * precautions before instantiating this class. > + */ > +class AutoSuppressGC > +{ > + AutoAtomicIncrement incr_; This does not need to be atomic, does it? I think normal arithmetic should be fine here. ::: js/src/jsopcode.cpp @@ +419,5 @@ > +/* > + * Useful to debug ReconstructPCStack. > + */ > +JS_FRIEND_API(JSBool) > +js_DumpScriptDepth(JSContext *cx, JSScript *script_, jsbytecode *pc) s/script_/scriptArg/
Attachment #678983 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #36) > Comment on attachment 678983 [details] [diff] [review] > Improve debug function for ReconstructPCStack. > > Review of attachment 678983 [details] [diff] [review]: > ----------------------------------------------------------------- > > This should be fine with comments addressed. > > ::: js/src/gc/Root.h > @@ +81,5 @@ > > > > JS_FRIEND_API(void) EnterAssertNoGCScope(); > > JS_FRIEND_API(void) LeaveAssertNoGCScope(); > > JS_FRIEND_API(bool) InNoGCScope(); > > +JS_FRIEND_API(bool) areGCEnabled(); > > Secondly, I'd prefer if we didn't have to expose this as friend API. Does > it still work if we remove the FRIEND annotation since it's not called > externally? Probably not, since we're inlining the users. I don't think we can, because we at least need the same visibility as InNoGCScope function because this function is used in “Return<T>::operator const T&()”. The only work around would be to pre-instantiate every Return<T> in jsgc.cpp, such as we can link against “Return<T>::operator const T&()” instead of inlining it, but I guess we don't want that for optimized builds. > Alternatively, > it should be possible to forward declare it /inside/ the methods that use > it. I am not sure to understand what you mean by “forward declare it /inside/ the methods”? And I don't understand the risk of exposure of this function, knowing that it is just reading a state variable before casting it to a bool. I am fine with adding the comment above the function, but I am not sure to fully understand why InNoGCScope should not be covered by this comment too.
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #37) > (In reply to Terrence Cole [:terrence] from comment #36) > > Comment on attachment 678983 [details] [diff] [review] > > Improve debug function for ReconstructPCStack. > > > > Review of attachment 678983 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This should be fine with comments addressed. > > > > ::: js/src/gc/Root.h > > @@ +81,5 @@ > > > > > > JS_FRIEND_API(void) EnterAssertNoGCScope(); > > > JS_FRIEND_API(void) LeaveAssertNoGCScope(); > > > JS_FRIEND_API(bool) InNoGCScope(); > > > +JS_FRIEND_API(bool) areGCEnabled(); > > > > Secondly, I'd prefer if we didn't have to expose this as friend API. Does > > it still work if we remove the FRIEND annotation since it's not called > > externally? Probably not, since we're inlining the users. > > I don't think we can, because we at least need the same visibility as > InNoGCScope function because this function is used in “Return<T>::operator > const T&()”. The only work around would be to pre-instantiate every > Return<T> in jsgc.cpp, such as we can link against “Return<T>::operator > const T&()” instead of inlining it, but I guess we don't want that for > optimized builds. > > > Alternatively, > > it should be possible to forward declare it /inside/ the methods that use > > it. > > I am not sure to understand what you mean by “forward declare it /inside/ > the methods”? http://stackoverflow.com/questions/9186996/function-forward-declaration-inside-another-function > And I don't understand the risk of exposure of this function, knowing that > it is just reading a state variable before casting it to a bool. > > I am fine with adding the comment above the function, but I am not sure to > fully understand why InNoGCScope should not be covered by this comment too. {Enter|Leave|In}NoGCScope is an assertion about the SpiderMonkey user: whether the code sequence in question can trigger a GC. IsGCEnabled is a property of the GC, independent of the code calling it. It's not dangerous, as such, it's just extremely confusing. It's very existence in the API implies that it must be possible to disable the GC from the API. This is not true. It is doubly confusing because it is placed next to functionality that is for a different purpose. Thus, we should have some separation and a comment explaining that its presence in the API is explicitly not because it is possible to disable the GC from API.
Comment on attachment 678980 [details] [diff] [review] Rewrite ReconstructPCStack. Review of attachment 678980 [details] [diff] [review]: ----------------------------------------------------------------- Better. Let's do another round though. I have a crazy crazy suggestion that might simplify this. Suppose we just follow these two rules about hidden instructions: - set pcdepth = hpcdepth just before every non-hidden instruction - set hpcdepth = pcdepth just after every non-hidden instruction That way hpcdepth never has to be -1. Would it work? The only cases I'm not sure about involve hidden JSOP_THROWING and JSOP_THROW instructions, and we can special-case those if needed. ::: js/src/jsopcode.cpp @@ +6459,4 @@ > > + if (cs->format & JOF_DECOMPOSE) { > + if (pc >= target) > + break; pc can't be >= target here. @@ +6472,5 @@ > + * annotated with another source note such as Break or Continue, and > + * thus it might also be considered as hidden for the time of the > + * emulation of this goto. > + */ > + if (!(sn && (SN_TYPE(sn) == SRC_HIDDEN || op == JSOP_GOTO))) { I am having trouble understanding what's going on here. Note that all this happens in an "if (breakFlow)" block. So to get here, we would have to have a hidden unconditional jump *during* an early exit path for a break or continue. That would mean this special case is for breaking or continuing out of a try-finally block; but why would that be special? I can see the comment is intended to help me understand this, but I still don't get it. I don't understand why the JSOP_GOTO exception here is needed at all, but if it is needed, it should be restricted to the specific cases that are going to be handled below. (Maybe I have completely misunderstood the code, so grab me on IRC if this review comment makes no sense to you.) @@ +6488,2 @@ > */ > + if (op == JSOP_THROWING || op == JSOP_THROW) { Note that breakFlow is not needed to detect this case. - JSOP_THROWING is emitted only in this case (that is, when a conditional catch block is followed by another catch block). - JSOP_THROW is only SRC_HIDDEN in this specific case. I point this out because I think breakFlow is unnecessary and confusing, and I hope you can remove it! @@ +6502,5 @@ > + > + /* > + * We are continuing after an unconditional jump, potentially out of > + * the current scope. Such instructions might be prefixed by > + * hidden/catch instructions to unwind let variables. Suggest adding "breakFlow is true if" at the beginning of this comment. Otherwise it appears to claim that it's always true. I'm not usually so picky about comments, but this code needs really good comments. :-( @@ +6508,5 @@ > + * Catch depth must stay unchanged across this instructions in order to > + * restore the stack depth needed by throw. > + */ > + breakFlow = (op == JSOP_RETRVAL || op == JSOP_GOTO || op == JSOP_GOSUB || > + op == JSOP_NOP || op == JSOP_THROW); Now that the code has changed I can't tell what breakFlow is trying to accomplish. I don't think JSOP_GOSUB belongs on this list. It's an unconditional jump at runtime, but not for the purposes of this function, which is about static properties of the operand stack. JSOP_GOSUB is more like JSOP_NOP in that regard; the next opcode necessarily has the same pcdepth, because control flow will continue there. @@ +6516,5 @@ > + * Hidden instructions annotate early-exit paths and do not affect > + * the stack depth when not taken. However, when the target is in an > + * early-exit path, hidden instructions need to be taken into > + * account. Save the depth before hidden instruction to restore it > + * if target is not in the early-exit path. "Hidden instructions annotate" isn't really correct. (Not your fault--the word "annotate" was already used incorrectly before your patch.) How about "Hidden instructions are used in early-exit paths..." As mentioned above, they are also used in two other cases; the comment should probably say so. @@ +6531,1 @@ > hpcdepth = unsigned(-1); SRC_CATCH is emitted on JSOP_ENTERBLOCK and JSOP_LEAVEBLOCK instructions, and no others, so breakFlow will always be false here. @@ +6533,5 @@ > + /* > + * Save catch depth to restore the stack for the next catch. This > + * path is used by enterblock and leaveblock to add/remove let > + * variables. It is only useful for continuing after non-taken > + * instructions breaking the normal flow (return, throw, goto). "let variables" is slightly misleading here, since we emit JSOP_ENTERBLOCK/LEAVEBLOCK even when the "let" keyword is not used at all: 00000: try 12 (+12) try { 00001: getgname "X" X; 00006: popv 00007: goto 31 (+24) } 00012: enterblock depth 0 {e: 0} catch (e) { 00017: exception 00018: setlocal 0 00021: pop 00022: leaveblock 1 } 00025: goto 31 (+6) 00030: nop @@ +6536,5 @@ > + * variables. It is only useful for continuing after non-taken > + * instructions breaking the normal flow (return, throw, goto). > + */ > + if (cpcdepth == unsigned(-1)) > + cpcdepth = pcdepth; Please explicitly check and only do this if op is JSOP_LEAVEBLOCK. It would be really strange to be doing this on the JSOP_ENTERBLOCK+SRC_CATCH instruction. @@ +6560,4 @@ > > + /* Continue until we reach the target. */ > + if (pc >= target) > + break; pc still can't be >= target! @@ +6575,5 @@ > + if ((op == JSOP_GOTO && sn && ( > + SN_TYPE(sn) == SRC_BREAK || SN_TYPE(sn) == SRC_BREAK2LABEL || > + SN_TYPE(sn) == SRC_CONTINUE || SN_TYPE(sn) == SRC_CONT2LABEL || > + SN_TYPE(sn) == SRC_SWITCHBREAK)) || > + op == JSOP_RETRVAL || op == JSOP_THROW) This condition is all right, but JSOP_THROW doesn't seem to belong.
Attachment #678980 - Flags: review?(jorendorff)
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #34) > Created attachment 678980 [details] [diff] [review] > Rewrite ReconstructPCStack. > > Delta: > - Handle return and throw in the same way as break & continue. With this patch: (function() { try {} catch (x) { return } finally { this.z.z } })() Assertion failure: script->analysis()->getCode(pc).stackDepth == pcdepth, (function() { try {} catch (x) { return } finally { this.t() } })() Assertion failure: script->analysis()->getCode(pc).stackDepth == pcdepth,
(In reply to Jason Orendorff [:jorendorff] from comment #39) > Comment on attachment 678980 [details] [diff] [review] > Rewrite ReconstructPCStack. > > Review of attachment 678980 [details] [diff] [review]: > ----------------------------------------------------------------- > > Better. Let's do another round though. > > I have a crazy crazy suggestion that might simplify this. > > Suppose we just follow these two rules about hidden instructions: > - set pcdepth = hpcdepth just before every non-hidden instruction > - set hpcdepth = pcdepth just after every non-hidden instruction > > That way hpcdepth never has to be -1. Would it work? The only cases I'm not > sure about involve hidden JSOP_THROWING and JSOP_THROW instructions, and we > can special-case those if needed. I think this is equivalent to the current implementation which is using -1. In both cases we have to specialized for JSOP_THROW. At the moment I feel more comfortable with the -1 implementation. > @@ +6488,2 @@ > > */ > > + if (op == JSOP_THROWING || op == JSOP_THROW) { > > Note that breakFlow is not needed to detect this case. > - JSOP_THROWING is emitted only in this case (that is, when a conditional > catch block is followed by another catch block). > - JSOP_THROW is only SRC_HIDDEN in this specific case. > > I point this out because I think breakFlow is unnecessary and confusing, and > I hope you can remove it! I cannot get rid of the breakflow notion but this should be easier to see on the next patch. I can remove the variable, but it would create some redundant code. > @@ +6502,5 @@ > > + > > + /* > > + * We are continuing after an unconditional jump, potentially out of > > + * the current scope. Such instructions might be prefixed by > > + * hidden/catch instructions to unwind let variables. > > Suggest adding "breakFlow is true if" at the beginning of this comment. > Otherwise it appears to claim that it's always true. > > I'm not usually so picky about comments, but this code needs really good > comments. :-( True, see the next patch. > @@ +6533,5 @@ > > + /* > > + * Save catch depth to restore the stack for the next catch. This > > + * path is used by enterblock and leaveblock to add/remove let > > + * variables. It is only useful for continuing after non-taken > > + * instructions breaking the normal flow (return, throw, goto). > > "let variables" is slightly misleading here, since we emit > JSOP_ENTERBLOCK/LEAVEBLOCK even when the "let" keyword is not used at all: I replaced them by "scoped" variable, which should be easier to understand. > @@ +6575,5 @@ > > + if ((op == JSOP_GOTO && sn && ( > > + SN_TYPE(sn) == SRC_BREAK || SN_TYPE(sn) == SRC_BREAK2LABEL || > > + SN_TYPE(sn) == SRC_CONTINUE || SN_TYPE(sn) == SRC_CONT2LABEL || > > + SN_TYPE(sn) == SRC_SWITCHBREAK)) || > > + op == JSOP_RETRVAL || op == JSOP_THROW) > > This condition is all right, but JSOP_THROW doesn't seem to belong. It does because the rethrow is breaking the flow which does to the next instruction, and the depth taken at the goto should be restored again: catch-end: --> 20 00001 00065: 8 leaveblock 1 // d = 1 / h = · / c = 1 00000 00068: 8 gosub 79 (+11) // d = 0 / h = · / c = 1 11 00000 00073: 8 goto 82 (+9) // d = 0 / h = 0 / c = 1 // d = c + 1 catch-rethrow: 11 00002 00078: 8 throw // d = 2 / h = 0 / c = · // d == 1 (after throw) // d = 0 00000 00079: 9 finally // d = 0 / h = · / c = · This is really a weird case, because we have 2 different paths, and only one linear entry for the hidden sequence of instruction.
Delta: - Untangle hidden depth and catch depth. - Improve comments & Remove code. - Add strong assertions for expected sequences of bytecode.
Attachment #678980 - Attachment is obsolete: true
Attachment #678980 - Flags: review?(gary)
Attachment #678980 - Flags: review?(choller)
Attachment #680425 - Flags: review?(jorendorff)
Attachment #680425 - Flags: review?(gary)
Attachment #680425 - Flags: review?(choller)
Attached patch Test cases.Splinter Review
Delta: - Add fuzzer's test cases.
Attachment #678520 - Attachment is obsolete: true
Comment on attachment 680425 [details] [diff] [review] Rewrite ReconstructPCStack. Review of attachment 680425 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsopcode.cpp @@ +6454,1 @@ > for (; pc < target; pc += oplen) { The condition of the loop should be “pc <= target” or “true” such as if the target is JSOP_THROW, we correctly restore the depth counter before exiting the loop. Otherwise AssertPCDepth will raise at the end of the loop.
Comment on attachment 680425 [details] [diff] [review] Rewrite ReconstructPCStack. I didn't find anything terribly wrong after a few hours' of fuzzing this time. r=me
Attachment #680425 - Flags: review?(gary) → review+
Comment on attachment 680425 [details] [diff] [review] Rewrite ReconstructPCStack. Review of attachment 680425 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for bearing with me. r=me with or without the proposed tweaks. ::: js/src/jsopcode.cpp @@ +6431,4 @@ > * operand-generating opcode PCs in pcstack. > * > * FIXME: Code to compute oplen copied from js_Disassemble1 and reduced. > * FIXME: Optimize to use last empty-stack sequence point. Remove both FIXME comments. The first is obsolete. The second is an optimization we will never do (we are actually de-optimizing this right now). Here's the comment I'd like to see at the top: /* * Walk forward from script->code and compute the stack depth and stack of * operand-generating opcode PCs in pcstack. * * Every instruction has a statically computable stack depth before and * after. This function returns the stack depth before the target * instruction. * * The stack depth can be computed from these three common-sense rules... * * - The stack depth before the first instruction of the script is 0. * * - The stack depth after an instruction is always simply the stack * depth before, plus whatever effect of the instruction has. * SimulateOp computes this. * * - Except for instructions following JSOP_GOTO, JSOP_RETRVAL, and * JSOP_THROW opcodes in the three specific cases listed below, the * stack depth before an instruction is simply the stack depth after * the preceding one. * * ...and these three less obvious rules. * * - Special case 1: break, continue, or return statements that exit * certain kinds of blocks. * * Rule: For these statements, we emit some instructions annotated with * SRC_HIDDEN, followed by a JSOP_GOTO or JSOP_RETRVAL. The stack depth * before the next instruction (following the GOTO/RETRVAL) equals the * stack depth before the first SRC_HIDDEN instruction. * * - Special case 2: At the end of conditional catch blocks. * * Rule: At the end of every conditional catch block there is a * JSOP_LEAVEBLOCK annotated with SRC_CATCH, then some other stuff, * then a JSOP_GOTO. The stack depth before the following instruction * (after the GOTO) is one more than the stack depth before the * LEAVEBLOCK. * * (The bytecode and source notes do not encode this straightforwardly, * so the code for this rule is complicated.) * * - Special case 3: Conditional expressions (A ? B : C). * * Rule: Between the code for B and the code for C, we emit a * JSOP_GOTO; the stack depth before the insn following the GOTO is one * less than the stack depth after the GOTO. */ If you just want to stop wasting time on this function and check in, go for it. If you want to take this comment and trim down some of the other comments, great. I think listing the special cases up front helps make sense of the code below, because you can just label each huge chunk of messy code at the top of the comment: "Special case 1/2/3". @@ +6454,1 @@ > for (; pc < target; pc += oplen) { Yes, agreed. Actually, the condition can just be removed. By the way, 'jsbytecode *pc = script->code;' could be moved to the line right before this loop, so: for (jsbytecode *pc = script->code; ; pc += oplen) { @@ +6462,2 @@ > continue; > + } This should be moved to the top of SimulateOp. That's where it belongs, and if we do it there, it's two lines instead of five: if (cs->format & JOF_DECOMPOSE) return pcdepth; Not a big deal either way. @@ +6472,5 @@ > + * > + * As we are not following any branch, we need to restore the stack > + * depth after any instruction which is breaking the flow of the > + * program. Sequences of hidden instructions are forming the following > + * grammar: "Special case 1". Good comment. I like the grammar; it is necessary to convince me that the code is sane. The grammar is incomplete; here's what it really looks like for exit paths: EarlyExit: setrval; ExitSegment* retrval; | (return stmt) ExitSegment* goto; (break/continue stmt) ExitSegment: hidden leaveblock; | (leave let-block or catch-block) hidden leavewith; | (leave with-block) hidden gosub; | (leave try or catch-block with finally) hidden enditer; | (leave for-in/of block) hidden leaveforletin; hidden enditer; hidden popn; (leave for-let-in/of block) Of course the hidden instructions that *aren't* actually involved in "Special case 1" are more important, as those are the scary X-factor in this code. @@ +6535,5 @@ > + /* > + * The catch depth is used to restore the depth expected in case of > + * rethrow or in case of guard failure. The depth expected by throw and > + * throwing instructions correspond to the result of enterblock and > + * exception instructions. "Special case 2." @@ +6561,5 @@ > + * If there is no rethrow, then we should expect an end-brace nop or a > + * finally. > + */ > + if (op == JSOP_LEAVEBLOCK && sn && SN_TYPE(sn) == SRC_CATCH) { > + JS_ASSERT(cpcdepth == unsigned(-1)); Since we are worried about the correctness of this code, use LOCAL_ASSERT rather than JS_ASSERT throughout. (In a debug build, the two are the same. In a release build, JS_ASSERT does nothing, but LOCAL_ASSERT quietly returns -1 if we flunk the assertion.) @@ +6610,2 @@ > */ > + AssertPCDepth(script, pc, pcdepth); Please move the comment into AssertPCDepth. Please move the AssertPCDepth down a bit: @@ +6612,5 @@ > > + /* Continue until we reach the target. */ > + if (pc == target) > + break; > + Ideally it looks like this: /* At this point, pcdepth is the stack depth before the insn at pc. */ AssertPCDepth(script, pc, pcdepth); if (pc >= target) break; /* Simulate the instruction at pc. */ if (SimulateOp(script, op, cs, pc, pcstack, pcdepth) < 0) return -1; /* At this point, pcdepth is the stack depth after the instruction at pc. */ @@ +6632,5 @@ > > + /* > + * A (C ? T : E) expression requires skipping T if target is in E or > + * after the whole expression, because this expression is pushing a > + * result on the stack and the goto cannot be skipped. "Special case 3."
Attachment #680425 - Flags: review?(jorendorff) → review+
Attachment #678507 - Attachment is obsolete: true
This patch takes the patch reviewed by jorendorff (with nits applied) and fuzzed by gary and strip all comments from it (even restore some previous one). And rename one variable to a non-useful name as it might indicate how to build a potential attack. [Security approval request comment] How easily can the security issue be deduced from the patch? Without comments it becomes a bit more difficult, but with all the constants such as "op == JSOP_LEAVEBLOCK && sn && SN_TYPE(sn) == SRC_CATCH" and "op == JSOP_RETRVAL", one can start experimenting with return and catch statements and can easily find a bug and understand how to create the desired relative offset, which can easily cause a crash. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? They did, I removed both the test and the new comments from the patch. Which older supported branches are affected by this flaw? At least fx18, after looking up the patches which caused such failure. If not all supported branches, which bug introduced the flaw? - Bug 687901 Introduced the bug (probably latent at the time) by using ReconstructPCStack out of it domain of definition. - Bug 767349 Emphasized the assertion which used ReconstructPCStack. - Bug 780451 Added the failing assertion, showing that both the previous implementation was not iterating correctly at the end of non-hidden instructions (which should be fine for the expression decompiler use case made in fx17.) Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The backport is copy and paste of the new ReconstructPCStack version. How likely is this patch to cause regressions; how much testing does it need? A Lot of testing before the happy ending …!
Attachment #681634 - Flags: sec-approval?
After testing with the test cases attached to this bug, fx17 is not affected. As described in comment 47, the use case changed with the addition of IonMonkey assertions, and the approximation that we had before was enough to emulate the stack behavior expected by the expression decompiler. Ask tracking flag for fx18, fx19 and later. Sorry for pushing before, I thought I made modification to fx17.
sec-approval should be a quick decision, given the fact that our release versions are unaffected.
(In reply to Alex Keybl [:akeybl] from comment #49) > sec-approval should be a quick decision, given the fact that our release > versions are unaffected. Do we know that 17, for example, is unaffected?
Doh, and I just saw the comment saying 17 is unaffected. my bad.
Attachment #681634 - Flags: sec-approval? → sec-approval+
Flag as [leave open] and in-testsuite? such as it stay open to land the comment, debug functions and the test cases later. https://hg.mozilla.org/integration/mozilla-inbound/rev/ae672aa46c28
Flags: in-testsuite?
Whiteboard: [jsbugmon:update,ignore][ion:p1:fx19] → [jsbugmon:update,ignore][ion:p1:fx19][leave open]
part 1 - Rewrite ReconstructPCStack: https://hg.mozilla.org/mozilla-central/rev/ae672aa46c28
Target Milestone: --- → mozilla19
Comment on attachment 681634 [details] [diff] [review] [Final] Rewrite ReconstructPCStack (see comment 47 for detail) [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 687901, Bug 767349, Bug 780451. User impact if declined: Potential crashes in JS code. Testing completed (on m-c, etc.): Fuzzed by Gary (comment 45), and check-in on m-c (comment 53). Risk to taking this patch (and alternatives if risky): Still a little risk of crash [we never know], but less important as mentioned previously (comment 8), as for the same amount of testing this code would be more secure than the previous one. String or UUID changes made by this patch: N/A
Attachment #681634 - Flags: approval-mozilla-aurora?
Comment on attachment 681634 [details] [diff] [review] [Final] Rewrite ReconstructPCStack [Triage Comment] sec-critical, has sec-approval, let's land on Aurora. Please land before Monday morning PT to make it in before the merge.
Attachment #681634 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/200d85e46699 The bug is left open, to wait until all changes are green on aurora before landing remaining patches.
Target Milestone: mozilla19 → ---
Oops, revert last unintended flag changes, set firefox18 as fixed.
Target Milestone: --- → mozilla19
Flags: in-testsuite? → in-testsuite+
Whiteboard: [jsbugmon:update,ignore][ion:p1:fx19][leave open] → [jsbugmon:update,ignore][ion:p1:fx19]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 680425 [details] [diff] [review] Rewrite ReconstructPCStack. Canceling r? here, since the patch is already live and being tested :)
Attachment #680425 - Flags: review?(choller)
Whiteboard: [jsbugmon:update,ignore][ion:p1:fx19] → [jsbugmon:update,ignore][ion:p1:fx19][adv-main18-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: