Last Comment Bug 385133 - Crash due to too much recursion in js_DecompileValueGenerator with watch, setter, delete, generator
: Crash due to too much recursion in js_DecompileValueGenerator with watch, set...
Status: VERIFIED FIXED
: crash, testcase, verified1.8.1.17
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: P4 critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: jsfunfuzz 416353
  Show dependency treegraph
 
Reported: 2007-06-19 20:51 PDT by Jesse Ruderman
Modified: 2008-08-22 04:42 PDT (History)
7 users (show)
dsicore: blocking1.9+
dveditz: wanted1.8.1.x+
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (1.93 KB, patch)
2008-02-03 17:43 PST, Igor Bukanov
brendan: review+
brendan: approval1.9+
Details | Diff | Splinter Review
first stab at a MOZILLA_1_8_BRANCH patch (1.95 KB, patch)
2008-08-16 21:34 PDT, Gary Kwong [:gkw] [:nth10sd]
igor: review+
dveditz: approval1.8.1.17+
Details | Diff | Splinter Review
diff between trunk and 1.8 patches (701 bytes, text/plain)
2008-08-17 02:34 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details

Description Jesse Ruderman 2007-06-19 20:51:31 PDT
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 2007-06-19 20:52:22 PDT
Be sure to test with -v 170, since the script uses "yield;".
Comment 2 Jesse Ruderman 2007-09-24 15:57:02 PDT
jsfunfuzz has been hitting this crash a lot lately.
Comment 3 Igor Bukanov 2007-09-24 16:01:21 PDT
I will look at it.
Comment 4 Damon Sicore (:damons) 2007-12-10 15:50:53 PST
+'ing w/ P4.
Comment 5 Igor Bukanov 2008-02-01 16:45:16 PST
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 2008-02-03 17:43:53 PST
Created attachment 301196 [details] [diff] [review]
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] 2008-02-07 18:31:42 PST
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 2008-02-08 07:18:56 PST
(In reply to comment #7)
> Is there a bug on file for the decompiler prob?

See bug 416353.
Comment 10 Bob Clary [:bc:] 2008-02-21 17:32:30 PST
/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:] 2008-02-26 07:19:40 PST
v
Comment 12 Gary Kwong [:gkw] [:nth10sd] 2008-06-04 14:01:39 PDT
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] 2008-08-16 21:34:37 PDT
Created attachment 334140 [details] [diff] [review]
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] 2008-08-16 21:38:56 PDT
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 2008-08-17 02:30:25 PDT
(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] 2008-08-17 02:34:46 PDT
Created attachment 334154 [details]
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] 2008-08-18 11:40:10 PDT
Not blocking 1.8 but we'll look at the patch.
Comment 18 Daniel Veditz [:dveditz] 2008-08-18 11:40:28 PDT
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] 2008-08-18 21:36:51 PDT
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 2008-08-20 01:04:45 PDT
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:] 2008-08-22 04:42:21 PDT
verified fixed 1.8.1 browser, shell linux/mac/win

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