Crash with too much recursion in array_toString

RESOLVED FIXED in mozilla6

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: Waldo)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
mozilla6
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 .x+, blocking-fx -)

Details

(Whiteboard: [sg:dos][fixed-in-tracemonkey])

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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

Updated

6 years ago
Group: core-security

Comment 1

6 years ago
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)

Updated

6 years ago
Whiteboard: [sg:dos]

Comment 2

6 years ago
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).
(Reporter)

Comment 3

6 years ago
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.
Hitting the redzone is not exploitable.

/be
Group: core-security

Updated

6 years ago
Group: core-security
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
blocking2.0: --- → ?
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.
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
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.
As per comment 10
blocking2.0: ? → -
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
(In reply to comment #11)
> As per comment 10

".x" would be nice. :)
blocking2.0: - → ?

Updated

6 years ago
blocking2.0: ? → .x
Duplicate of this bug: 646052
The description of bug 646052 (a dup of this) says that this is now a #16 topcrasher.
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.
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.
blocking-fx: --- → ?

Updated

6 years ago
blocking-fx: ? → -
Okay.  (I missed bugmail for comment 18 when it was sent.)
Status: NEW → ASSIGNED
Created attachment 526353 [details] [diff] [review]
Push the recursion check upward into all callers
Assignee: general → jwalden+bmo
Attachment #526353 - Flags: review?(jorendorff)
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.
Attachment #526353 - Flags: review?(jorendorff) → review+
http://hg.mozilla.org/tracemonkey/rev/a7b220e7425a
Group: core-security
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [sg:dos] → [sg:dos][fixed-in-tracemonkey]
Target Milestone: --- → mozilla6
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/a7b220e7425a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.