355497 - Stack overflow with Array.slice, getter for "0"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> [].filter.__defineGetter__(0, [].slice); [5].filter[0]
Stack overflow

...
489 js 	0x00044bf8 array_slice + 616 (crt.c:355)
490 js 	0x00033490 js_Invoke + 1548 (crt.c:355)
491 js 	0x000338ec js_InternalInvoke + 204 (crt.c:355)
492 js 	0x00033a68 js_InternalGetOrSet + 248 (crt.c:355)
493 js 	0x00022350 js_GetProperty + 1048 (crt.c:355)
494 js 	0x00043058 GetArrayElement + 292 (crt.c:355)
495 js 	0x00044bf8 array_slice + 616 (crt.c:355)
...

Comment 1

[Hermann Schwab]

Talkback-Record TB24292490E 
javascript:[].filter.__defineGetter__(0, [].slice); [5].filter[0] in Firefox2

Comment 2

[Aiko]

Short analysis of this bug:
filter has very little to do with it, its only 'fault' is that its arity is 1. Any object with a .length > 0 could be used:
  o = { length: 1 }; o.__defineGetter__(0, [].slice); o[0]

Getting o[0] invokes slice, which tries to create an array containing all of o's properties 0..(length-1). Unfortunately, to do that slice has to get o[0] again...

The functionally equivalent version
  o = { length: 1 }; o.__defineGetter__(0, function () { return Array.slice(o) }) ;o[0]
does not crash because it is protected by the recursion check in js_Interpret.

Additionally, it's quite trivial to construct a testcase where a setter exposes the same problem:
  a=[]; a.__defineSetter__(0, a.unshift); a[0]=1

My conclusion is that the right place for the recursion check would be in js_InternalGetOrSet.

Comment 3

[Aiko]

Attachment: Yet another recursion check

Comment 4

[Brian Crowder]

It seems like we can do better than a naive stack-size check here...  That said, I don't think the recursion check is a bad idea, either.  Let's see what mrbkap thinks.

Comment 5

[Brian Crowder]

Comment on attachment 250939 [details] [diff] [review]
Yet another recursion check

Talking with Brendan, this is the right way to go.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 250939 [details] [diff] [review]
Yet another recursion check

>Index: jsinterp.c
>+    }
>     /*

Nit: add a space here.

I agree with Brendan and Brian that that is the right fix.

Comment 7

[Brian Crowder]

Comment on attachment 250939 [details] [diff] [review]
Yet another recursion check

Another good, straightforward crash fix by adding a recursion check

Comment 8

[Daniel Veditz [:dveditz]]

Please get this landed on the trunk for regression testing before we approve it for branch landing.

Comment 9

[Brian Crowder]

This is on the trunk:

jsinterp.c  3.321

Comment 10

[Brendan Eich [:brendan]]

The patch (sorry I didn't look until now) is suboptimal in that js_InternalGetOrSet will call js_Interpret indirectly (via js_InternalInvoke => js_Invoke) for a scripted function, and js_Interpret does CHECK_STACK_SIZE.

It would be better if the check could be done only for callable objects other than scripted functions.  This could be worked into the cx->runtime->checkObjectAccess logic, with reordering of terms that penalizes embeddings that do not set that per-runtime callback.  Thoughts?

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: followup to avoid over-checking the stack limit

Not sure this actually improves performance relative to the current trunk -- Brian, can you try to measure with a scripted getter function?

/be

Comment 12

[Brian Crowder]

Best of 3 for trunk run:

real    0m2.323s
user    0m2.246s
sys     0m0.015s


Best of 3 for patched run:

real    0m2.320s
user    0m2.253s
sys     0m0.014s


The script I used:

tmp = { get x () { return 3 } }; for (let i = 0; i < 5000000; ++i) { tmp.x }


Badness looks about the same?

Comment 13

[Daniel Veditz [:dveditz]]

Which do we want for branches? The original one that's on trunk with the approval requests (trige team's preference all things being equal), or should we wait while you sort out the ideas in this second patch?

Comment 14

[Brian Crowder]

I think the trunk version is fine, hopefully Brendan will chime in.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 250939 [details] [diff] [review]
Yet another recursion check

approved for 1.8/1.8.0 branches, a=dveditz for drivers

Comment 16

[Brian Crowder]

1_8:
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.81; previous revision: 3.181.2.80
done


1_8_0:
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.17.2.27; previous revision: 3.181.2.17.2.26
done

Comment 17

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Array/regress-355497.js,v  <--  regress-355497.js
initial revision: 1.1

Comment 18

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0, 1.8.1 1.9.0 2007-01-23 win/mac*/linux