Crash due to too much recursion through GetCustomIterator

VERIFIED FIXED in mozilla11

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: gkw, Assigned: dmandelin)

Tracking

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

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

Firefox Tracking Flags

(firefox9-, firefox10-, firefox11-)

Details

(Whiteboard: [sg:dos], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Created attachment 558528 [details]
stacks

q = {}.__proto__
function h() {
    q.p = q
}
h()
function t() {
    for each(let y in []) {
        y.__iterator__ = Iterator
        for (c in []) {}
    }
}
t()

crashes js debug shell on m-c changeset db9e99d537f2 with the patch from bug 683966 without any CLI arguments at js::PropertyTable::search and crashes js opt shell at PrimitiveToObject.

Based on the stack, this seems to be a recursive overflow, but locking s-s just to be safe.
(Reporter)

Updated

6 years ago
Summary: Crash [@ js::PropertyTable::search] or [@ PrimitiveToObject] with Iterator → Crash [@ js::PropertyTable::search] or [@ PrimitiveToObject] or [@ js::InvokeKernel] or [@ js_CheckForStringIndex] with Iterator
recursive stack exhaustion would be sg:dos, but the stacks you attached don't show that kind of error.
Crash Signature: [@ js::PropertyTable::search ] [@ PrimitiveToObject ] [@ js::InvokeKernel ] [@ js_CheckForStringIndex ]
Whiteboard: js-triage-needed → [sg:critical?] js-triage-needed
(Reporter)

Comment 2

6 years ago
(In reply to Daniel Veditz from comment #1)
> recursive stack exhaustion would be sg:dos, but the stacks you attached
> don't show that kind of error.

I should probably attach longer stacks next time, but they seem to repeatedly go on for thousands of lines.

Comment 3

6 years ago
One way to make "too much recursion" explicit is to strip out all the distractions, like I did in bug 671797 ;)

I happen to know that 0x00007fff5f3ffffc is typical "guard page at the end of stack memory" on Mac.  It would be nice if gdb gave more information about the inaccessible memory page.
Summary: Crash [@ js::PropertyTable::search] or [@ PrimitiveToObject] or [@ js::InvokeKernel] or [@ js_CheckForStringIndex] with Iterator → Crash due to too much recursion through GetCustomIterator

Comment 4

6 years ago
By the way, in fuzzing/js-known/mozilla-central/assertions.txt, you can write

# Bug 684922
[TMR] GetCustomIterator

The "TMR" is special thing for too-much-recursion crashes. It instructs detect_interesting_crashes to only consider a crash report to be a match if the offending function is seen on 9+ lines.

You have to pick a function from the repeating portion of the stack, but that's saner than picking a function from the top.

Comment 5

6 years ago
Simpler testcase:

var op = Object.prototype;
op.b = op;
op.__iterator__ = Iterator;
for (var c in {}) {}
Ok to unhide the bug then?
Whiteboard: [sg:critical?] js-triage-needed → [sg:dos] js-triage-needed

Comment 7

6 years ago
Yes.
Group: core-security

Comment 8

6 years ago
The js::PropertyTable::search crash signature is #1 top crasher on Mac OS X in 9.0b2.
(Reporter)

Comment 9

6 years ago
(In reply to Scoobidiver from comment #8)
> The js::PropertyTable::search crash signature is #1 top crasher on Mac OS X
> in 9.0b2.

Given that we have simple testcases in comment 0 and comment 5, plus it being a topcrasher (assuming the correlation), could someone please kindly take a look?

Nominating for tracking from versions 9 onwards.
tracking-firefox10: --- → ?
tracking-firefox11: --- → ?
tracking-firefox9: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 10

6 years ago
Created attachment 577397 [details] [diff] [review]
Patch

This is an infinite recursion bug. The cycle goes through GetCustomIterator, which seems like a reasonable place to put a JS_CHECK_RECURSION.

I'd be pretty surprised if this had an effect on the topcrashes, but I wouldn't mind being lucky.
Assignee: general → dmandelin
Status: NEW → ASSIGNED
Attachment #577397 - Flags: review?(luke)
(Assignee)

Updated

6 years ago
Whiteboard: [sg:dos] js-triage-needed → [sg:dos]

Updated

6 years ago
Attachment #577397 - Flags: review?(luke) → review+
(Assignee)

Comment 11

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/720247db9dc3

I don't think we need to track this bug, but we should check out the crashstats after it lands.
tracking-firefox10: ? → -
tracking-firefox11: ? → -
tracking-firefox9: ? → -
Flags: in-testsuite+
(Assignee)

Comment 12

6 years ago
Followup fix: http://hg.mozilla.org/integration/mozilla-inbound/rev/ff1c18668452
https://hg.mozilla.org/mozilla-central/rev/720247db9dc3
https://hg.mozilla.org/mozilla-central/rev/ff1c18668452
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Duplicate of this bug: 706306
Do we want to fix this on Aurora as well?

Comment 16

6 years ago
(In reply to Scoobidiver from comment #8)
> The js::PropertyTable::search crash signature is #1 top crasher on Mac OS X
> in 9.0b2.

I looked at some of the js::PropertyTable::search crash reports and none of the stacks showed deep recursion. So the topcrash is not this bug.
(Reporter)

Comment 17

6 years ago
fwiw I put the testcase in comment 5 in a <script> tag and opt Firefox crashed [@ PrimitiveToObject ] - the js::PropertyTable::search signature I got was off a debug shell.
(Reporter)

Comment 18

5 years ago
A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.