Closed Bug 355497 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: crowderbt)

Details

(4 keywords, Whiteboard: [need testcase])

Attachments

(2 files)

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)
...
OS: Mac OS X 10.4 → All
Hardware: Macintosh → PC
Talkback-Record TB24292490E 
javascript:[].filter.__defineGetter__(0, [].slice); [5].filter[0] in Firefox2
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.
Attachment #250939 - Flags: review?(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 on attachment 250939 [details] [diff] [review]
Yet another recursion check

Talking with Brendan, this is the right way to go.
Attachment #250939 - Flags: review?(crowder) → review+
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 on attachment 250939 [details] [diff] [review]
Yet another recursion check

Another good, straightforward crash fix by adding a recursion check
Attachment #250939 - Flags: approval1.8.1.2?
Attachment #250939 - Flags: approval1.8.0.10?
Please get this landed on the trunk for regression testing before we approve it for branch landing.
Assignee: general → crowder
This is on the trunk:

jsinterp.c  3.321
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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
Not sure this actually improves performance relative to the current trunk -- Brian, can you try to measure with a scripted getter function?

/be
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?
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?
I think the trunk version is fine, hopefully Brendan will chime in.
Comment on attachment 250939 [details] [diff] [review]
Yet another recursion check

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Attachment #250939 - Flags: approval1.8.1.2?
Attachment #250939 - Flags: approval1.8.1.2+
Attachment #250939 - Flags: approval1.8.0.10?
Attachment #250939 - Flags: approval1.8.0.10+
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
/cvsroot/mozilla/js/tests/js1_5/Array/regress-355497.js,v  <--  regress-355497.js
initial revision: 1.1
Flags: in-testsuite+
Whiteboard: [need testcase]
verified fixed 1.8.0, 1.8.1 1.9.0 2007-01-23 win/mac*/linux
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.