Last Comment Bug 457521 - Crash [@ js_DecompileValueGenerator]
: Crash [@ js_DecompileValueGenerator]
Status: VERIFIED FIXED
: [sg:critical?] post 1.8-branch
: assertion, crash, fixed1.9.0.7, regression, testcase, verified1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
: general
:
:
: jsfunfuzz 420399
  Show dependency treegraph
 
Reported: 2008-09-28 02:57 PDT by Gary Kwong [:nth10sd]
Modified: 2009-03-05 17:48 PST (History)
12 users (show)
sayrer: blocking1.9.1+
dveditz: blocking1.9.0.7+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
bclary: in‑testsuite+
bclary: in‑litmus-
See Also:
Crash Signature:
[@ js_DecompileValueGenerator]


Attachments
crash log (2.94 KB, text/plain)
2008-09-28 02:57 PDT, Gary Kwong [:nth10sd]
no flags Details
fix v1 (821 bytes, patch)
2008-11-18 07:56 PST, Igor Bukanov
crowderbt: review+
mbeltzner: approval1.9.1b2-
Details | Diff | Splinter Review
updated fix (477 bytes, patch)
2008-11-26 07:37 PST, Igor Bukanov
igor: review+
Details | Diff | Splinter Review
js1_6/extensions/regress-457521.js (2.19 KB, text/plain)
2008-12-04 05:33 PST, Bob Clary [:bc:]
no flags Details
fox for 190 branch (884 bytes, patch)
2009-01-23 06:59 PST, Igor Bukanov
igor: review+
dveditz: approval1.9.0.7+
Details | Diff | Splinter Review

Summon comment box

Description Gary Kwong [:nth10sd] 2008-09-28 02:57:00 PDT
Created attachment 340781 [details]
crash log

this.__defineSetter__("x", [,,,].map)
this.watch("x", (new Function("var y, eval")))
x = true

crashes at 0x00000000fffffffc for opt and asserts debug shell on Mac 10.5.5. This occurs for both trunk and tracemonkey shells.

Console output for tracemonkey shell:

$ ./js-dbg-tm-intelmac 
js> this.__defineSetter__("x", [,,,].map)
js> this.watch("x", (new Function("var y, eval")))
js> x = true
Assertion failure: (size_t) (regs->sp - stackBase) <= StackDepth(script), at jsopcode.cpp:4988
Trace/BPT trap

$ ./js-opt-tm-intelmac 
js> this.__defineSetter__("x", [,,,].map)
js> this.watch("x", (new Function("var y, eval")))
js> x = true
Segmentation fault

===

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000fffffffc
Crashed Thread:  0

Thread 0 Crashed:
0   js-opt-tm-intelmac            	0x00059139 js_DecompileValueGenerator + 229
1   js-opt-tm-intelmac            	0x0001353f js_ReportValueErrorFlags + 55
2   js-opt-tm-intelmac            	0x0002cfca js_ReportIsNotFunction + 350
3   js-opt-tm-intelmac            	0x0002d06b js_ValueToFunction + 143
4   js-opt-tm-intelmac            	0x0002d0d6 js_ValueToFunctionObject + 84

Nominating security sensitive due to scary address of 0x00000000fffffffc.
Comment 1 Jesse Ruderman 2008-10-05 22:09:05 PDT
Related to bug 421623?
Comment 2 Igor Bukanov 2008-11-18 07:55:11 PST
this is my fault - the changes for the bug 420399 removed the detection of watch-pseudo-frames.
Comment 3 Igor Bukanov 2008-11-18 07:56:28 PST
Created attachment 348766 [details] [review]
fix v1

The patch adds the check for NULL fp->regs->sp - this is exactly what happens with watch-pseudo-frames.
Comment 4 Igor Bukanov 2008-11-18 08:18:00 PST
The bug exists on the 1.9.0 branch or later.
Comment 5 Igor Bukanov 2008-11-18 10:37:09 PST
Comment on attachment 348766 [details] [review]
fix v1

Asking for b2 approval to increase test coverage for the security fix as it should go to 1.9.0 branch as well.
Comment 6 Igor Bukanov 2008-11-18 12:21:14 PST
Comment on attachment 348766 [details] [review]
fix v1

The patch applies to 1.9.0 as-is so asking for approval for 1.9.0.6.
Comment 7 Mike Beltzner [:beltzner] 2008-11-20 08:30:35 PST
Comment on attachment 348766 [details] [review]
fix v1

1.9.0.6 is a ways away, so we can wait until after we branch - trying to close things down and limit risk on the b2 stuff right now.
Comment 8 Igor Bukanov 2008-11-26 07:37:33 PST
Created attachment 350153 [details] [review]
updated fix

The trunk changes required trivial merge of the patch.
Comment 9 Igor Bukanov 2008-11-26 12:49:11 PST
landed - http://hg.mozilla.org/mozilla-central/rev/bb54a5700bca
Comment 10 Bob Clary [:bc:] 2008-12-04 05:33:19 PST
Created attachment 351364 [details]
js1_6/extensions/regress-457521.js
Comment 11 Bob Clary [:bc:] 2008-12-06 03:56:11 PST
verified mozilla-central but not tracemonkey.
Comment 12 Samuel Sidler (old account; do not CC) 2009-01-05 07:40:47 PST
Igor, can you work up a 1.9.0 patch if the one in this bug doesn't apply?
Comment 13 Gary Kwong [:nth10sd] 2009-01-05 08:19:56 PST
Strange -- the patch never landed on 1.9.0.x, but my testing shows that it no longer seems to be affected. Did any other patch on 1.9.0.x fix this issue as well?

==============

pre-patch:

$ ./js
js> this.__defineSetter__("x", [,,,].map)
js> this.watch("x", (new Function("var y, eval")))
js> x = true
typein:2: TypeError: (void 0) is not a function
js> 

$ cat ../jsopcode.c | grep -B 5 -C 5 "if (\!fp || \!fp->regs"
              spindex == JSDVG_IGNORE_STACK ||
              spindex == JSDVG_SEARCH_STACK);

    for (fp = cx->fp; fp && !fp->script; fp = fp->down)
        continue;
    if (!fp || !fp->regs)
        goto do_fallback;

    script = fp->script;
    regs = fp->regs;
    pc = regs->pc;

==============

post patch: (I de-bitrotted Igor's patch)

$ ./js
js> this.__defineSetter__("x", [,,,].map)
js> this.watch("x", (new Function("var y, eval")))
js> x = true
typein:2: TypeError: (void 0) is not a function
js> 

$ cat ../jsopcode.c | grep -B 5 -C 5 "if (\!fp || \!fp->regs"
              spindex == JSDVG_IGNORE_STACK ||
              spindex == JSDVG_SEARCH_STACK);

    for (fp = cx->fp; fp && !fp->script; fp = fp->down)
        continue;
    if (!fp || !fp->regs || !fp->regs->sp)
        goto do_fallback;

    script = fp->script;
    regs = fp->regs;
    pc = regs->pc;
Comment 14 Bob Clary [:bc:] 2009-01-05 08:45:17 PST
the testcase never failed for me on 1.9.0.
Comment 15 Gary Kwong [:nth10sd] 2009-01-05 08:50:27 PST
(In reply to comment #14)
> the testcase never failed for me on 1.9.0.

(In reply to comment #4)
> The bug exists on the 1.9.0 branch or later.

Igor seems to have thought otherwise in comment #4?
Comment 16 Bob Clary [:bc:] 2009-01-05 09:30:52 PST
I didn't say the bug didn't exist on 1.9.0. Only that the test never failed on 1.9.0.
Comment 17 Gary Kwong [:nth10sd] 2009-01-05 09:32:17 PST
(In reply to comment #16)
> I didn't say the bug didn't exist on 1.9.0. Only that the test never failed on
> 1.9.0.

Ah, thanks, bc. Sorry for my mis-interpretation.
Comment 18 Bob Clary [:bc:] 2009-01-16 00:20:03 PST
v 1.9.1, 1.9.2
Comment 19 Igor Bukanov 2009-01-23 06:59:18 PST
Created attachment 358400 [details] [review]
fox for 190 branch

This is the 1.9.1 patch with jsopcode.cpp renamed to jsopcode.c.
Comment 20 Daniel Veditz 2009-01-23 11:23:30 PST
Comment on attachment 358400 [details] [review]
fox for 190 branch

Approved for 1.9.0.7, a=dveditz for release-drivers.
Comment 21 Reed Loden [:reed] (very busy) 2009-02-03 01:26:19 PST
CVS HEAD:

Checking in js/src/jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.320; previous revision: 3.319
done
Comment 22 Al Billings [:abillings] 2009-02-12 16:08:29 PST
The unit test doesn't exist for 1.9.0 and the test case doesn't seem to fail. Is there any way to validate that this is truly fixed (or was broken in a way that affected anything) for 1.9.0.7?
Comment 23 Bob Clary [:bc:] 2009-03-05 17:48:20 PST
http://hg.mozilla.org/tracemonkey/rev/00c50dfb0e92
/cvsroot/mozilla/js/tests/js1_6/extensions/regress-457521.js,v  <--  regress-457521.js
initial revision: 1.1

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