854788 - Crash [@ js::Proxy::objectClassIs] with [@ js::ObjectClassIs] and [@ js::DirectProxyHandler::objectClassIs] on the stack

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

s = newGlobal('');
function f(code) {
    try {
        evalcx(code, s)
    } catch (e) {}
}
for (var z = 0; z < 99; z++) {
    f("\
        s0 = '';\
        b0 = new ArrayBuffer();\
        a1 = [];\
        s1 = s2 = this;\
        function x(){};\
        eval(\"Object.preventExtensions(this)\");\
        s0 = a1 = [x,x,x];\
        options('strict_mode');\
        s1 = x;\
        s0 += s0;\
        s1.toSource = (function() {\
            for(var v of s2) {\
                b0 = wrap(b0)\
            }\
        });\
        s2 += s0;\
        s2 += s2;\
        s2 += s2;\
    ");
    f("var p;");
    f("for(var j;;){};");
    f("var r;");
    f("var _;");
    f("let b={};");
    f("function v(){};");
    f("Float32Array(b0);");
}

crashes js opt shell on m-c changeset 4d3250f3afea with --ion-eager at js::Proxy::objectClassIs with js::ObjectClassIs and js::DirectProxyHandler::objectClassIs on the stack.

Tested with 64-bit opt deterministic threadsafe shell, on Windows.

This looks like a recursive overflow and likely to be safe, but still marking s-s because it involves ArrayBuffer and Float32Array.

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=dmandelin

Bug 803182 only landed on Aurora 21, so marking flags as necessary.

Comment 1

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

Attachment: Another version of the stack

Comment 2

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

Apparently --ion-eager is not needed - this reproduces even with --no-ion and --no-jm.

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 4

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

(In reply to Christian Holler (:decoder) from comment #3)
> JSBugMon: Cannot process bug: Unable to automatically reproduce, please
> track manually.

This might be Windows-specific..

Comment 5

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

Terrence, is bug 803182 a possible cause?

Comment 6

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

Makoto, you provided the patch in bug 834645 that fixed bug 836601, which is similar to this bug.

Terrence suggested you might be a better person to look at this, is it alright?

Comment 7

[Makoto Kato [:m_kato]]

It may need more user mode stack size although I have expanded user mode stack size to 2MB on Win64 by bug 834645.

Also, On OSX/Linux platform, default is 8MB.

Gery, is this only win64?  How about win32?

Comment 8

[Alex Keybl [:akeybl]]

What's the security rating of this bug?

Comment 9

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

> Gery, is this only win64?  How about win32?

Yes, this seems to affect only Win64. Do you mind recommending a security rating for this bug as well, if needed?

Comment 10

[Makoto Kato [:m_kato]]

for Win64 is tier 3 platform.  

Gery, Although I cannot reproduce this on my environment, can you fix this if incrementing STACKSIZE by editbin.exe??

Comment 11

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

(In reply to Makoto Kato from comment #10)
> for Win64 is tier 3 platform.  
> 
> Gery, Although I cannot reproduce this on my environment, can you fix this
> if incrementing STACKSIZE by editbin.exe??

How do I do it? Do I modify config.mk in the two locations as you did in bug 834645?

And should I also increase STACKSIZE to 8MB?

Comment 12

[Terrence Cole [:terrence]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #11)
> (In reply to Makoto Kato from comment #10)
> > for Win64 is tier 3 platform.  
> > 
> > Gery, Although I cannot reproduce this on my environment, can you fix this
> > if incrementing STACKSIZE by editbin.exe??
> 
> How do I do it? Do I modify config.mk in the two locations as you did in bug
> 834645?
> 
> And should I also increase STACKSIZE to 8MB?

Yes, I think that should also work. 8MiB would be excellent: it would be nice to standardize this number to make these bugs less platform dependent.

Comment 13

[Terrence Cole [:terrence]]

Attachment: v0

Looking at the stack, it seems that we're also missing a recursion check in Proxy::objectClassIs, so I doubt upping the stack size is going to help in this instance. Can you try this patch instead?

Comment 14

[Terrence Cole [:terrence]]

As to security rating, this is just a DOS.

Also, it occurred to me that the attached patch will probably still give the wrong error in Win64, new one coming up shortly.

Comment 15

[Terrence Cole [:terrence]]

Attachment: v1: also bump the stack size on windows

Comment 16

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

(In reply to Terrence Cole [:terrence] from comment #14)
> As to security rating, this is just a DOS.

Opening up.

Comment 17

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

Comment on attachment 730877 [details] [diff] [review]
v1: also bump the stack size on windows

fwiw, both patch v0 and v1 fix the bug, except that this v1 seemed a little faster to show the recursion error when the testcase was run.

(no crash was seen in both)

Comment 18

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

Guessing at csec-oom due to stack exhaustion / recursive stack overflow.

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 730877 [details] [diff] [review]
v1: also bump the stack size on windows

Review of attachment 730877 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/config.mk
@@ +591,5 @@
>  
>  ifdef _MSC_VER
>  ifeq ($(CPU_ARCH),x86_64)
>  # set stack to 2MB on x64 build.  See bug 582910
> +WIN32_EXE_LDFLAGS	+= -STACK:8388608

The comment here needs updating in both config.mk files, and ideally a bit more explanation for the driveby reader.

Seeing as dbaron reviewed the changes here, I'm also going to forward this to him for a review of this specific bit, too.

Oh, and be ready for the MemShrink people to fall on you like a pile of bricks, I bet.  This might only apply if the stack gets (safely) blown once, but once it's happened you're potentially hosed forever on the memory front.  (Unless kernels try to decommit if the stack size falls significantly, but that sounds heuristic-y enough that I suspect they wouldn't do it, to avoid pathological behaviors.)

::: js/src/jsproxy.cpp
@@ +2538,5 @@
>  
>  bool
>  Proxy::objectClassIs(HandleObject proxy, ESClassValue classValue, JSContext *cx)
>  {
> +    JS_CHECK_RECURSION(cx, return false);

Blugh, return false here isn't the same thing as failure!  This method, and objectClassIs in general, really should be fallible.  Something to fix at some point, not here.

Comment 20

[Terrence Cole [:terrence]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19)
> ::: config/config.mk
> @@ +591,5 @@
> >  
> >  ifdef _MSC_VER
> >  ifeq ($(CPU_ARCH),x86_64)
> >  # set stack to 2MB on x64 build.  See bug 582910
> > +WIN32_EXE_LDFLAGS	+= -STACK:8388608
> 
> The comment here needs updating in both config.mk files, and ideally a bit
> more explanation for the driveby reader.

Heh, that was indeed humorously myopic of me.

> Oh, and be ready for the MemShrink people to fall on you like a pile of
> bricks, I bet.  This might only apply if the stack gets (safely) blown once,
> but once it's happened you're potentially hosed forever on the memory front.
> (Unless kernels try to decommit if the stack size falls significantly, but
> that sounds heuristic-y enough that I suspect they wouldn't do it, to avoid
> pathological behaviors.)

Thanks for the reminder. I need to go pull up numbers for default stack sizes on different platforms.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 730877 [details] [diff] [review]
v1: also bump the stack size on windows

I'm probably not really the right reviewer for this; bumping to bsmedberg, though I'd note that if you're updating the code you should also update the corresponding comment.

Comment 22

[Alex Keybl [:akeybl]]

These signatures aren't showing up with much volume, and this isn't a very critical security bug. If a low risk fix is found, please consider nominating for uplift.

Comment 23

[Terrence Cole [:terrence]]

Attachment: v2: update comment per review feedback.

Keeping prior patch as I can't figure out how to care the existing r+ while adding an r?.

Comment 24

[Terrence Cole [:terrence]]

Comment on attachment 730877 [details] [diff] [review]
v1: also bump the stack size on windows

s/care/carry/

Comment 25

[Benjamin Smedberg]

I can think of the following reasons this is not a good idea:

* it dramatically increases the size of crash report minidumps
* it makes infinite-recursion crashes take a lot longer

I think that we *should* be crashing in this case, or at least hitting the JS recursion-limit checks and stopping the script. So please either morph this bug into making the stack limit of the shell match the actual stack size, or mark this WONTFIX.

Comment 26

[Daniel Veditz [:dveditz]]

Benjamin: what about v0 which just adds the missing recursion check, without increasing the stack size? Are you OK with that?

Comment 27

[Benjamin Smedberg]

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

That's fine with me, although I'm not a JS peer.

Comment 28

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

Terrence, perhaps v0 might be a better idea?

Comment 29

[Terrence Cole [:terrence]]

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

Revivifying v0 and carrying r+ from the review of this hunk in v1.

Comment 30

[Terrence Cole [:terrence]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #25)
> I can think of the following reasons this is not a good idea:
> 
> * it dramatically increases the size of crash report minidumps
> * it makes infinite-recursion crashes take a lot longer

True. That is also a good argument for reducing the stack size on other platforms. Perhaps we should pursue that instead.

Comment 31

[Terrence Cole [:terrence]]

Attachment: v0: with checkin comment.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4887d47d1f25

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4887d47d1f25