Crash [@ DecompileExpression] with trap

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P2
critical
VERIFIED FIXED
10 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: Brian Crowder)

Tracking

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

Trunk
x86
All
assertion, crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite ?
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [has patch][has review][has approval][firebug-compat], crash signature)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

10 years ago
let (e = ({g:x} = {a:1, b:2, c:3, d:4, e:5})) { }
function f() { with (x = x) { z; } }
trap(f, 0, "");
f();

Assertion failure: script->main <= pc && pc < script->code + script->length, at jsopcode.c:4915

Changing the number of items in the hash at the top can make it trigger different assertions, crashes, or incorrect TypeError messages.
(Assignee)

Comment 1

10 years ago
I wrote a nice, simple patch that fixes this (but doesn't really, read on):
Index: jsdbgapi.c
===================================================================
RCS file: /cvsroot/mozilla/js/src/jsdbgapi.c,v
retrieving revision 3.147
diff -u -p -8 -r3.147 jsdbgapi.c
--- jsdbgapi.c	30 Apr 2008 02:18:19 -0000	3.147
+++ jsdbgapi.c	30 Apr 2008 03:56:26 -0000
@@ -100,17 +100,18 @@ js_UntrapScriptCode(JSContext *cx, JSScr
     JSTrap *trap;
 
     code = script->code;
     rt = cx->runtime;
     DBG_LOCK(rt);
     for (trap = (JSTrap *)rt->trapList.next;
          trap != (JSTrap *)&rt->trapList;
          trap = (JSTrap *)trap->links.next) {
-        if (trap->script == script) {
+        if (trap->script == script && trap->pc >= script->code &&
+            trap->pc < script->code + script->length) {
             if (code == script->code) {
                 jssrcnote *sn, *notes;
                 size_t nbytes;
 
                 nbytes = script->length * sizeof(jsbytecode);
                 notes = SCRIPT_NOTES(script);
                 for (sn = notes; !SN_IS_TERMINATOR(sn); sn = SN_NEXT(sn))
                     continue;


The problem is that DVG uses DecompileExpression, which needs more help.  It builds a PC stack from the un-"cleansed" version of the code and then later asserts that it is referencing the code it thinks it is.  We need to do the same for DecompileExpression as we do for DecompileCode, and make it safe by adding the patch above.
(Assignee)

Comment 2

10 years ago
I should hit this as soon after 1.9 as possible.
Assignee: general → crowder
Priority: -- → P2

Comment 3

10 years ago
Note the patch in bug 430293 fixes such JSOP_TRAP bugs once and for all, but it requires performance testing.
(Assignee)

Comment 4

10 years ago
Created attachment 318648 [details] [diff] [review]
best patch yet

Will ask for review after more testing and self-review.
(Assignee)

Comment 5

10 years ago
Created attachment 318687 [details] [diff] [review]
passes js/tests suite

Something to look at.  I'll do mochis in a bit.
Attachment #318648 - Attachment is obsolete: true
Attachment #318687 - Flags: review?(shaver)

Comment 6

10 years ago
sounds like this might fix the curent #10 topcrash on trunk so we should try and get it before RC1
Flags: blocking1.9?
(Assignee)

Comment 7

10 years ago
My patch survives mochitests, how's the review going?
(Assignee)

Updated

10 years ago
Blocks: 429266
Comment on attachment 318687 [details] [diff] [review]
passes js/tests suite


>+    code = js_UntrapScriptCode(cx, script); /* reach out label after this */

We have another cliche for "From here on, control must flow through", etc. -- could
you copy it more exactly?  I wasn't sure at first if it was a typo of "reach our label".

>             LOCAL_ASSERT(nuses == 0 ||
>-                         *pcstack[pcdepth - 1] == JSOP_ENTERBLOCK);
>+                         *pcstack[pcdepth - 1] == JSOP_ENTERBLOCK ||
>+                         (*pcstack[pcdepth - 1] == JSOP_TRAP &&
>+                          JS_GetTrapOpcode(cx, script, pcstack[pcdepth - 1])));

The last bit there wants to be

&& JS_GetTrapOpcode(...) == JSOP_ENTERBLOCK.

r+a=shaver with those addressed.
Attachment #318687 - Flags: review?(shaver)
Attachment #318687 - Flags: review+
Attachment #318687 - Flags: approval1.9+
Comment on attachment 318687 [details] [diff] [review]
passes js/tests suite

>+        if (trap->script == script && trap->pc >= script->code &&
>+            trap->pc < script->code + script->length) {

Drive-by nits:

* Number-line order for relational operator operands: code <= pc && pc < code + length.
* We usually optimize this harder into (size_t)(pc - code) < length.
* If && terms spill over multiple lines, break after each && (ditto ||).

/be
(Assignee)

Comment 10

10 years ago
Created attachment 318891 [details] [diff] [review]
feedback from brendan and shaver

The patch to land.
Attachment #318687 - Attachment is obsolete: true
Attachment #318891 - Flags: review+
Comment on attachment 318891 [details] [diff] [review]
feedback from brendan and shaver

>+        if (trap->script == script &&
>+            script->code <= trap->pc &&
>+            (size_t)(trap->pc - script->code) < script->length) {

No, you don't need the code <= pc any longer. The subtraction and < are enough, since if pc < code the very large unsigned number will be much >= length.

Please remove the code <= pc line before landing.

/be
(Assignee)

Comment 12

10 years ago
Created attachment 318895 [details] [diff] [review]
one more spin
Attachment #318891 - Attachment is obsolete: true
Attachment #318895 - Flags: review+
(Assignee)

Comment 13

10 years ago
jsdbgapi.c: 3.148
jsopcode.c: 3.316
(Assignee)

Updated

10 years ago
Blocks: 431724
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Comment on attachment 318895 [details] [diff] [review]
one more spin

Nit that has come up before in other bugs:

>+    pcstack = NULL;
>+    oldcode = script->code;
>+    oldmain = script->main;
>+    /* From this point the control must flow through the label out. */

One blank line before any comment consuming one or more lines by itself is house style and a bit easier on the eyes.

/be
(Assignee)

Comment 15

10 years ago
jsopcode.c:3.317 to address the whitespace nit (also for a full-line comment below this one)
I backed this out because it was causing netwerk/test/unit/test_bug261425.js to fail (reproduced locally).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 318925 [details]
js1_7/extensions/regress-431465.js
Flags: in-testsuite+
Flags: in-litmus-
OS: Mac OS X → All
This patch also caused the test in e4x/Expressions/11.1.4-08.js and js1_7/lexical/regress-346642-03.j to terminate early without any indication of an error.
(Assignee)

Comment 19

10 years ago
Created attachment 319044 [details] [diff] [review]
oh, right.  braces.  *blush*

Passes make check; running the suite again now.
Attachment #318895 - Attachment is obsolete: true
(Assignee)

Comment 20

10 years ago
Comment on attachment 319044 [details] [diff] [review]
oh, right.  braces.  *blush*

Passes test suite, and no unit-test regressions.
Attachment #319044 - Flags: review?(shaver)
I'm not seeing this in topcrashes at all - in fact, I'm not seeing any crashes. Does this need to block?
Comment on attachment 319044 [details] [diff] [review]
oh, right.  braces.  *blush*

>+    if (pcstack)
>+        JS_free(cx, pcstack);

JS_free is null-safe, no need for if (pcstack) guard.

>-                         *pcstack[pcdepth - 1] == JSOP_ENTERBLOCK);
>+                         *pcstack[pcdepth - 1] == JSOP_ENTERBLOCK ||
>+                         (*pcstack[pcdepth - 1] == JSOP_TRAP &&
>+                          JS_GetTrapOpcode(cx, script,
>+                                           pcstack[pcdepth - 1]) ==
>+                                           JSOP_ENTERBLOCK));

Nit: the full JS_GetTrapOpcode call fits on its own line -- you just need to break before the == (also, the right operand of ==, JSOP_ENTERBLOCK, should not underhang the overflow params here; anyway, if you break before == then the == JSOP_ENTERBLOCK can go on the next line indented the same as the left operand of ==).

Shaver's traveling, but I can r+. Igor should probably have a look with triple review we should get this in, or Firebug users will suffer.

/be
Attachment #319044 - Flags: review+

Updated

10 years ago
Attachment #319044 - Flags: review?(igor)
(In reply to comment #21)
> I'm not seeing this in topcrashes at all - in fact, I'm not seeing any crashes.
> Does this need to block?

We're not blocking only for known topcrashes, but you probably won't see this in any topcrash data without a lot of Firebug users paying the price. Let's avoid doing that experiment.

/be
Status: REOPENED → ASSIGNED
(Assignee)

Comment 24

10 years ago
Created attachment 319081 [details] [diff] [review]
addressing brendan's comments
Attachment #319044 - Attachment is obsolete: true
Attachment #319081 - Flags: review?(igor)
Attachment #319044 - Flags: review?(shaver)
Attachment #319044 - Flags: review?(igor)

Updated

10 years ago
Attachment #319081 - Flags: review?(igor) → review+
(Assignee)

Comment 25

10 years ago
Comment on attachment 319081 [details] [diff] [review]
addressing brendan's comments

Re-requesting approval, since it's been a few iterations.
Attachment #319081 - Flags: approval1.9?

Comment 26

10 years ago
> re: comment 21:   I'm not seeing this in topcrashes at all - in fact, I'm not seeing any crashes.  Does this need to block?

Just to be clear.  Indeed there are crashes and it should block.  We hope that  
Bug 431724 – Firefox 3.0pre Crash [@ js_UntrapScriptCode ] - gets fixed with this and removes the #10 topcrash.

Also check brendan's comment 23

Comment 27

10 years ago
Comment on attachment 319081 [details] [diff] [review]
addressing brendan's comments

let's get this in
Attachment #319081 - Flags: approval1.9? → approval1.9+
(In reply to comment #23)
> (In reply to comment #21)
> > I'm not seeing this in topcrashes at all - in fact, I'm not seeing any crashes.
> > Does this need to block?
> 
> We're not blocking only for known topcrashes, but you probably won't see this
> in any topcrash data without a lot of Firebug users paying the price. Let's
> avoid doing that experiment.

Hokayfine. That hadn't been mentioned anywhere else in the bug, and the only other justification I'd seen for blocking status was the topcrash listing in the keywords.

Blocking.
Flags: blocking1.9? → blocking1.9+
Whiteboard: [firebug-compat]
Whiteboard: [firebug-compat] → [has patch][has review][has approval][firebug-compat]
(Reporter)

Comment 29

10 years ago
See bug 432077 for another testcase that still crashes with the patch.
(Assignee)

Comment 30

10 years ago
jsdbgapi.c: 3.150
jsopcode.c: 3.319
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Created attachment 319451 [details]
js1_7/extensions/regress-431465.js
Attachment #318925 - Attachment is obsolete: true
verified no crash 1.9.0, 1.9.1 mozilla-central/tracemonkey.
Status: RESOLVED → VERIFIED
when this bug is opened, the test should be checked in.
Flags: in-testsuite+ → in-testsuite?
Crash Signature: [@ DecompileExpression]
Group: core-security
You need to log in before you can comment on or make changes to this bug.