385133 - Crash due to too much recursion in js_DecompileValueGenerator with watch, setter, delete, generator

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P4)

Description

[Jesse Ruderman]

This script crashes trunk jsshell:

this.x setter= ({}.watch);
this.watch('x', 'foo'.split);
delete x;
function g(){ x = 1; yield; }
for (i in g()) { }

Comment 1

[Jesse Ruderman]

Be sure to test with -v 170, since the script uses "yield;".

Comment 2

[Jesse Ruderman]

jsfunfuzz has been hitting this crash a lot lately.

Comment 3

[Igor Bukanov]

I will look at it.

Comment 4

[Damon Sicore (:damons)]

+'ing w/ P4.

Comment 5

[Igor Bukanov]

Here is a simpler script that shows the same infinite recursion:

~/m/ff/mozilla/js/src $ cat ~/m/y.js
this.x setter = ({}.watch);
function g() { x = 1; yield; }
g().next();
~/m/ff/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/m/y.js
segmentation fault

Comment 6

[Igor Bukanov]

Attachment: v1

The reason for the bug is that 

this.x setter = ({}.watch);
function g() { x = 1; yield; }
g().next();

will invoke the watch function as a setter during x=1 like in watch.call(this, 1). Since the watch expects a function as its second argument, this throws an exception in js_ValueToFunction via:

        js_ReportIsNotFunction(cx, vp, JSV2F_SEARCH_STACK);

That will trigger decompilation of

00001:  bindname "x"
00004:  one
00005:  setname "x"

It eventually calls PopOff in
http://lxr.mozilla.org/seamonkey/source/js/src/jsopcode.c#1775 

That calls GetOff which invokes recursively js_DecompileValueGenerator at
http://lxr.mozilla.org/seamonkey/source/js/src/jsopcode.c#794 . There are comments before GetOff that such recursion means a bug in the decompiler and the recursive call is a recovery procedure. The patch does not fix that. What it fixes is the infinite recursion in the recovery procedure. 

js_DecompileValueGenerator at
  
http://lxr.mozilla.org/seamonkey/source/js/src/jsopcode.c#4869 

checks that pc stored in stack's slot is a valid one. But that check compare stack's pointer against stack's arena. That does not work with generators as their frames are not allocated using the arena. 

The patch fixes that via just checking that sp belongs to [spbase, spbase + depth)

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 301196 [details] [diff] [review]
v1

Old, old arena base/limit code. Thanks!

Is there a bug on file for the decompiler prob?

/be

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> Is there a bug on file for the decompiler prob?

See bug 416353.

Comment 9

[Igor Bukanov]

I checked in the patch from comment 6 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1202507940&maxdate=1202508086&who=igor%25mir2.org

Comment 10

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/public-failures.txt,v  <--  public-failures.txt
new revision: 1.43; previous revision: 1.42

/cvsroot/mozilla/js/tests/js1_7/regress/regress-385133-01.js,v  <--  regress-385133-01.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_7/regress/regress-385133-02.js,v  <--  regress-385133-02.js
initial revision: 1.1

Comment 11

[Bob Clary [:bc] (inactive)]

v

Comment 12

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

The testcase in comment #0 crashes 1.8 branch js shell as well, and jsfunfuzz seems to hit it quite often too.

However, the Igor's testcase in comment #5 works as expected in 1.8 branch js shell (doesn't crash). And yes, I turned on -v 170 for every test.

Not sure whether to nominate since this merely causes it to run out of stack space.

Comment 13

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

Attachment: first stab at a MOZILLA_1_8_BRANCH patch

I backported Igor's trunk patch above to the MOZILLA_1_8_BRANCH.

The patch fixes the hang if the commands in comment #0 are input to the shell. Post-patch, the shell works as expected.

Console output:

$ ./js-old -v 170
js> this.x setter= ({}.watch);
function watch() {
    [native code]
}
js> this.watch('x', 'foo'.split);
js> delete x;
true
js> function g(){ x = 1; yield; }
js> for (i in g()) { }
^C

$ ./js-new -v 170
js> this.x setter= ({}.watch);
function watch() {
    [native code]
}
js> this.watch('x', 'foo'.split);
js> delete x;
true
js> function g(){ x = 1; yield; }
js> for (i in g()) { }
typein:4: TypeError: x is not a function
js>

Comment 14

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

Adding on to comments #12 and #13, the current situation is that the branch js shell no longer *crashes* anymore, instead it *hangs*.

Nominating blocking1.8.1.17 for inclusion into 2.0.0.17, as there is a patch available.

Comment 15

[Igor Bukanov]

(In reply to comment #13)
> Created an attachment (id=334140) [details]
> first stab at a MOZILLA_1_8_BRANCH patch
> 
> I backported Igor's trunk patch above to the MOZILLA_1_8_BRANCH.

Could you attach a plain diff (i.e. diff without -u or -c options) between trunk and 1.8 patches?

Comment 16

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

Attachment: diff between trunk and 1.8 patches

$ diff rec_fix.patch branch-patch-v1.diff 
4,8c4,8
< retrieving revision 3.286
< diff -U8 -p -r3.286 jsopcode.c
< --- js/src/jsopcode.c	1 Feb 2008 06:01:17 -0000	3.286
< +++ js/src/jsopcode.c	3 Feb 2008 18:16:41 -0000
< @@ -4782,16 +4782,20 @@ js_DecompileValueGenerator(JSContext *cx
---
> retrieving revision 3.89.2.76
> diff -u -8 -p -r3.89.2.76 jsopcode.c
> --- js/src/jsopcode.c	7 Aug 2008 16:23:54 -0000	3.89.2.76
> +++ js/src/jsopcode.c	17 Aug 2008 04:09:58 -0000
> @@ -4426,16 +4426,20 @@ js_DecompileValueGenerator(JSContext *cx
15c15
<      char *name;
---
>      JSString *name;
29c29
< @@ -4879,29 +4883,25 @@ js_DecompileValueGenerator(JSContext *cx
---
> @@ -4523,29 +4527,26 @@ js_DecompileValueGenerator(JSContext *cx
46a47
> +
$

Comment 17

[Daniel Veditz [:dveditz]]

Not blocking 1.8 but we'll look at the patch.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 334140 [details] [diff] [review]
first stab at a MOZILLA_1_8_BRANCH patch

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

Comment 19

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

Someone with checkin privs, please check in the patch to the MOZILLA_1_8_BRANCH before the code freeze date on August 26 11:59pm PDT.

Comment 20

[Igor Bukanov]

landed on MOZILLA_1_8_BRANCH:

Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.89.2.77; previous revision: 3.89.2.76
done

Comment 21

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.1 browser, shell linux/mac/win