457521 - Crash [@ js_DecompileValueGenerator]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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]

Related to bug 421623?

Comment 2

[Igor Bukanov]

this is my fault - the changes for the bug 420399 removed the detection of watch-pseudo-frames.

Comment 3

[Igor Bukanov]

Attachment: 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]

The bug exists on the 1.9.0 branch or later.

Comment 5

[Igor Bukanov]

Comment on attachment 348766 [details] [diff] [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]

Comment on attachment 348766 [details] [diff] [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, not reading bugmail]]

Comment on attachment 348766 [details] [diff] [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]

Attachment: updated fix

The trunk changes required trivial merge of the patch.

Comment 9

[Igor Bukanov]

landed - http://hg.mozilla.org/mozilla-central/rev/bb54a5700bca

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: js1_6/extensions/regress-457521.js

Comment 11

[Bob Clary [:bc] (inactive)]

verified mozilla-central but not tracemonkey.

Comment 12

[Samuel Sidler (old account; do not CC)]

Igor, can you work up a 1.9.0 patch if the one in this bug doesn't apply?

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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] (inactive)]

the testcase never failed for me on 1.9.0.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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] (inactive)]

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 [:gkw] [:nth10sd] (NOT official MoCo now)]

(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] (inactive)]

v 1.9.1, 1.9.2

Comment 19

[Igor Bukanov]

Attachment: fox for 190 branch

This is the 1.9.1 patch with jsopcode.cpp renamed to jsopcode.c.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 358400 [details] [diff] [review]
fox for 190 branch

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 21

[Reed Loden [:reed]]

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 - ex-MoCo]]

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] (inactive)]

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