836601 - Crash [@ ScriptedIndirectProxyHandler::get] or [@ js::Proxy::get]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

let k
Proxy.createFunction(function() {
    return {
        get: Uint32Array
    }
}(), decodeURIComponent) & k

crashes js debug and opt shell on m-c changeset 20bbf73921f4 without any CLI arguments at ScriptedIndirectProxyHandler::get with js::Proxy::get on the stack.

Does not seem to affect 32-bit Windows js shells, and this seems to be a recursive stack overflow, so likely not s-s.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   119340:9dd844b7152e
user:        Terrence Cole
date:        Mon Oct 29 13:36:41 2012 -0700
summary:     Bug 803182 - Make the js shell stack limit match the browser's; r=d
mandelin

I'm not sure if this is entirely correct. Terrence, what do you think?

Comment 2

[Terrence Cole [:terrence]]

This doesn't crash for me on linux64 -- the patch in question only changed the js shell in windows, so the browser should be unaffected by this. I don't have a 64bit windows machine to test with.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Putting this on my plate to test when I have time to retest on my Win64 machine.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I can still reproduce this. Terrence has taken a look and may have more information to provide.

Comment 5

[Terrence Cole [:terrence]]

Verified on Gary's machine that this is indeed running out of native stack. It appears that the stack depth required to go from Proxy to Proxy in this case is larger than the difference from the artificial to the native stack top and it just happens to line up wrong.

The right solution for this is to fix how we allocate stacks and then allocate the same amount of space on all platforms. We should fix this, but given that it is not security sensitive or a fuzz blocker it's not going to be a high priority.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> The right solution for this is to fix how we allocate stacks and then
> allocate the same amount of space on all platforms. We should fix this, but
> given that it is not security sensitive or a fuzz blocker it's not going to
> be a high priority.

Erm, it is, it is a fuzzblocker for Win64. The only workaround is not to fuzz on Win64 as this is somewhat easily triggered.

Comment 7

[Terrence Cole [:terrence]]

Oh, Dang! Well, I'll whip up something ugly and windows specific that we can use as an interim solution.

Comment 8

[Terrence Cole [:terrence]]

Attachment: v0

Please check if this fix works.

Good find, by the way: it appears to be a pre-existing browser bug in the Win64.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 711389 [details] [diff] [review]
v0

Nope, still crashes. I'll post a stack trace.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack for retest with patch v0

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   121848:f3cba77064cd
parent:      121843:f27d5d9ebef2
user:        Makoto Kato
date:        Thu Feb 14 15:22:00 2013 +0900
summary:     Bug 834645 - move -STACK parameter to config.mk. r=ted

Assuming FIXED by bug 834645. Terrence, is this likely?

Comment 12

[Terrence Cole [:terrence]]

Yes, that is exactly the right fix: much better than what's attached on this bug.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Testcase landed via bug 845569.