Last Comment Bug 635389 - Crash with too much recursion in array_toString
: Crash with too much recursion in array_toString
Status: RESOLVED FIXED
[sg:dos][fixed-in-tracemonkey]
: crash, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla6
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
: 646052 (view as bug list)
Depends on:
Blocks: 326633 562446
  Show dependency treegraph
 
Reported: 2011-02-18 14:22 PST by Jesse Ruderman
Modified: 2013-01-19 14:10 PST (History)
11 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+
-


Attachments
crash report (73.91 KB, text/plain)
2011-02-18 14:22 PST, Jesse Ruderman
no flags Details
Push the recursion check upward into all callers (1.34 KB, patch)
2011-04-15 13:18 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-02-18 14:22:12 PST
Created attachment 513597 [details]
crash report

var x = [];
x.join = x.toString;
"" + x;

Crash with repeating stack portion:
  386 __ZL14array_toStringP9JSContextjPN2js5ValueE
  387 js::CallJSNative
  388 js::Invoke
  389 __ZL14array_toStringP9JSContextjPN2js5ValueE

The first bad revision is:
changeset:   a3946d490610
user:        Jeff Walden
date:        Thu Jul 15 12:33:33 2010 -0500
summary:     Bug 562446 - ES5: Array.prototype.toString and Array.prototype.toLocaleString should be generic.  r=igor
Comment 1 Andreas Gal :gal 2011-02-18 14:25:34 PST
Unbounded stack use should not be filed as an open bug IMO.

js> var x = [];
js> x.join = x.toString;
function toString() {[native code]}
js> "" + x;

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00007fff5f3ffff8
0x00000001000ff03e in JSID_IS_ATOM (id={asBits = 4308603328}) at jsatom.h:87
87	    return JSID_IS_STRING(id);
(gdb)
Comment 2 Andreas Gal :gal 2011-02-18 14:26:52 PST
I would take a reviewed patch even this late in the cycle, but its probably not necessary to block on this. This is not exploitable on most platforms (any platforms that are bad about keeping stack and heap apart this might be a problem though).
Comment 3 Jesse Ruderman 2011-02-18 14:33:22 PST
Platforms that are bad about keeping the stack and heap apart are hopeless.  Too-much-recursion crash bugs are almost as common as null deref crash bugs, and they're even harder to prevent.  I think this bug report should be public.
Comment 4 Brendan Eich [:brendan] 2011-02-18 14:42:21 PST
Hitting the redzone is not exploitable.

/be
Comment 7 Brendan Eich [:brendan] 2011-02-18 15:36:04 PST
I don't think we have ever treated bugs like this as s-s.

A big local array, or alloca abusage, in a script-reachable part of the codebase, would be its own bug. Not this bug.

/be
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-18 21:46:00 PST
Agree with comment 3, comment 4, and comment 7, stack overflow can't be treated as exploitable, and we should open.

Should array_toString have a JS_CHECK_RECURSION(cx, return false) in it?  Per comment 0 it has to be there, CallJSNative, or Invoke to fix this.  CJN seems not right, although I'm not sure I could articulate *why* it feels not right to me.  Is there a reason Invoke doesn't have one, or shouldn't have one?  That seems the general chokepoint to be filled to me, but I could well be missing a subtilty of infinite recursion.
Comment 9 Brendan Eich [:brendan] 2011-02-18 21:57:17 PST
In 1.9.0 (Fx3) array_join_sub was the common subroutine of array_toString and variations on that theme, and it had JS_CHECK_RECURSION. array_join_sub seems to have been renamed array_toString_sub, and it still does JS_CHECK_RECURSION first thing, but now array_toString no longer calls it.

The patch for bug 562446 changed this as noted in comment 0 and the dependency.

The reason Invoke does not check recursion is that Interpret does, and natives in general don't need the check (many are control flow leaves). Thus the very old JS_CHECK_RECURSION calls in certain *_toString* helpers and functions.

/be
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-18 23:28:05 PST
Oh, I also don't think this should block.  Although if someone posted a patch (who?  technically we're all-blockers, all the time these days) I'd definitely rubber-stamp an approval.
Comment 11 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-19 15:53:32 PST
As per comment 10
Comment 12 Brendan Eich [:brendan] 2011-02-19 16:41:17 PST
Someone should take assignment -- Waldo (your patch regressed), Igor (you are concerned about exploitability)? This bug sitting around unassigned yet s-s is no good.

/be
Comment 13 Gary Kwong [:gkw] [:nth10sd] 2011-02-19 21:00:53 PST
(In reply to comment #11)
> As per comment 10

".x" would be nice. :)
Comment 15 Jason Orendorff [:jorendorff] 2011-03-29 10:06:48 PDT
*** Bug 646052 has been marked as a duplicate of this bug. ***
Comment 16 Jason Orendorff [:jorendorff] 2011-03-29 10:08:21 PDT
The description of bug 646052 (a dup of this) says that this is now a #16 topcrasher.
Comment 17 Jeff Walden [:Waldo] (remove +bmo to email) 2011-03-29 10:49:40 PDT
The topcrash call seems to have been because it was a crash in PropertyTable::search, and that signature has a lot of crashes.  This correlation between this mode of crashing and that signature is suspect.  I doubt most of those crashes have anything like the infinite-looking stack we have here, and I bet they're different bugs, but I have neither the time nor a sufficiently powerful net connection to scan through the reports and find out for sure.
Comment 18 Jason Orendorff [:jorendorff] 2011-03-30 07:07:05 PDT
Jeff, not to put too fine a point on it, would you please take and fix this straightforward browser-crashing regression you introduced? We're all busy, I get it -- but if we're too busy to clean up our own spills even now, something's wrong, and plowing ahead isn't going to fix it.
Comment 19 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-15 11:49:01 PDT
Okay.  (I missed bugmail for comment 18 when it was sent.)
Comment 20 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-15 13:18:07 PDT
Created attachment 526353 [details] [diff] [review]
Push the recursion check upward into all callers
Comment 21 Jason Orendorff [:jorendorff] 2011-04-25 12:35:04 PDT
Comment on attachment 526353 [details] [diff] [review]
Push the recursion check upward into all callers

Review of attachment 526353 [details] [diff] [review]:

Don't forget to add the test. r=me with that.
Comment 22 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-26 23:20:34 PDT
http://hg.mozilla.org/tracemonkey/rev/a7b220e7425a
Comment 23 Chris Leary [:cdleary] (not checking bugmail) 2011-05-02 16:01:13 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/a7b220e7425a
Comment 24 Christian Holler (:decoder) 2013-01-19 14:10:30 PST
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929

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