The default bug view has changed. See this FAQ.

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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P4
critical
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: Igor Bukanov)

Tracking

(Blocks: 2 bugs, {crash, testcase, verified1.8.1.17})

Trunk
x86
Mac OS X
crash, testcase, verified1.8.1.17
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.8.1.x +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

10 years ago
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()) { }
(Reporter)

Comment 1

10 years ago
Be sure to test with -v 170, since the script uses "yield;".
(Reporter)

Updated

10 years ago
Severity: normal → critical
(Reporter)

Comment 2

10 years ago
jsfunfuzz has been hitting this crash a lot lately.
(Assignee)

Comment 3

10 years ago
I will look at it.
Assignee: general → igor
(Reporter)

Updated

9 years ago
Flags: blocking1.9?
+'ing w/ P4.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
(Assignee)

Comment 5

9 years ago
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
(Assignee)

Comment 6

9 years ago
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)
Attachment #301196 - Flags: review?(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
Attachment #301196 - Flags: review?(brendan)
Attachment #301196 - Flags: review+
Attachment #301196 - Flags: approval1.9+
(Assignee)

Updated

9 years ago
Blocks: 416353
(Assignee)

Comment 8

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

See bug 416353.
(Assignee)

Comment 9

9 years ago
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
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 10

9 years ago
/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
Flags: in-testsuite+
Flags: in-litmus-

Comment 11

9 years ago
v
Status: RESOLVED → VERIFIED
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.
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>
Attachment #334140 - Flags: review?(igor)
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.
Flags: blocking1.8.1.17?
(Assignee)

Comment 15

9 years ago
(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?
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
> +
$
Attachment #334154 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

9 years ago
Attachment #334140 - Flags: review?(igor) → review+
(Assignee)

Updated

9 years ago
Attachment #334140 - Flags: approval1.8.1.17?
Not blocking 1.8 but we'll look at the patch.
Flags: blocking1.8.1.17? → wanted1.8.1.x+
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.
Attachment #334140 - Flags: approval1.8.1.17? → approval1.8.1.17+
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.
(Assignee)

Comment 20

9 years ago
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
Keywords: fixed1.8.1.17

Comment 21

9 years ago
verified fixed 1.8.1 browser, shell linux/mac/win
Keywords: fixed1.8.1.17 → verified1.8.1.17
You need to log in before you can comment on or make changes to this bug.