595230 - Crash [@ EscapeAttributeValue] or "Assertion failure: JSVAL_TO_OBJECT(nsval)->getClass() == &js_NamespaceClass,"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

try {
    __proto__ = Proxy.createFunction((function() {}),
    function() {})
    var x
    *
} catch(e) {}
default xml namespace = x
for (let b in [0, 0]) <x/>

asserts js debug shell on TM changeset e892ea041581 without -m nor -j at Assertion failure: JSVAL_TO_OBJECT(nsval)->getClass() == &js_NamespaceClass, and crashes at EscapeAttributeValue

Seems to be null deref but s-s just to be safe.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x0013d328 in EscapeAttributeValue ()
(gdb) bt
#0  0x0013d328 in EscapeAttributeValue ()
#1  0x00140afe in ParseXMLSource ()
#2  0x00075437 in js::Execute ()
Previous frame inner to this frame (gdb could not unwind past this frame)
(gdb) x/i $eip
0x13d328 <_ZL20EscapeAttributeValueP9JSContextRN2js6VectorItLm32ENS1_18ContextAllocPolicyEEEP8JSStringi+24>:    mov    (%ecx),%ebp
(gdb) x/b $ecx
0x0:    Cannot access memory at address 0x0

Comment 1

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

Attachment: assertStack.txt

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   52709:cb719643afc5
user:        Brendan Eich
date:        Wed Aug 18 11:34:13 2010 -0700
summary:     Bug 535629 - Deleted properties' slots are not recycled (r=gal).

Comment 3

[Daniel Veditz [:dveditz]]

Might be a null deref, or maybe the 0x0 came from the testcase and can be set to something interesting. The JS gurus will have to tell us.

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix

I just recently noticed JSCLASS_GLOBAL_FLAGS reserves 3 * JSProto_LIMIT and DefineStandardSlot sets a standard proto in the "third bank" keyed by protoKey biased by 2 * JSProto_LIMIT, but nothing ever reads those slots, and they are guaranteed to duplicate the second bank. I thought I filed this, can't find it atm. Anyway, this is a fix to cope with our crazy lazy built-in init protocol.

/be

Comment 5

[Brendan Eich [:brendan]]

This can confuse a built-in prototype for the default XML namespace, which E4X code expects to be a Namespace instance.

I think this is just a DoS crasher or at worst an info leak, though, since the default XML namespace is only read from, not jumped through.

BTW, I don't think the testcase is minimal, and I do not see any evidence that Proxies are involved. Just slot recycling. Mark accordingly.

/be

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> BTW, I don't think the testcase is minimal, and I do not see any evidence that
> Proxies are involved. Just slot recycling. Mark accordingly.

Proxy.createFunction and the result stuffed in the global __proto__ seems to be involved, but I'm not sure it's more than a fuzzer-found way of ordering slot alloc and free. I'm pretty sure that's all it is doing, but I haven't found an alternative test -- and the __proto__ damage breaks the jsreftest framework (can't call reportCompare). Wahhhh.

/be

Comment 7

[Brendan Eich [:brendan]]

This is rich:

#0  js_ErrorToException (cx=0x100712a30, message=0x10071d230 "getPropertyDescriptor is not a function", reportp=0x7fff5fbfcc20, callback=0x10004366d <js_GetErrorMessage(void*, char const*, unsigned int)>, userRef=0x0) at ../jsexn.cpp:1129
#1  0x0000000100046451 in ReportError (cx=0x100712a30, message=0x10071d230 "getPropertyDescriptor is not a function", reportp=0x7fff5fbfcc20, callback=0x10004366d <js_GetErrorMessage(void*, char const*, unsigned int)>, userRef=0x0) at ../jscntxt.cpp:1335
#2  0x00000001000465ed in js_ReportErrorNumberVA (cx=0x100712a30, flags=0, callback=0x10004366d <js_GetErrorMessage(void*, char const*, unsigned int)>, userRef=0x0, errorNumber=22, charArgs=1, ap=0x7fff5fbfcce0) at ../jscntxt.cpp:1688
#3  0x0000000100015b7f in JS_ReportErrorNumber (cx=0x100712a30, errorCallback=0x10004366d <js_GetErrorMessage(void*, char const*, unsigned int)>, userRef=0x0, errorNumber=22) at ../jsapi.cpp:5312
#4  0x0000000100123cdf in js::FundamentalTrap (cx=0x100712a30, handler=0x101603410, atom=0x1016019c0, fvalp=0x7fff5fbfce48) at ../jsproxy.cpp:307

because the trap isn't callable, so we are bound to unwind through this stack:

#5  0x00000001001253ed in js::JSScriptedProxyHandler::getPropertyDescriptor (this=0x1003a8fc0, cx=0x100712a30, proxy=0x1016034b0, id={asBits = 6}, desc=0x7fff5fbfced8) at ../jsproxy.cpp:492
#6  0x0000000100122635 in js::JSProxyHandler::has (this=0x1003a8fc0, cx=0x100712a30, proxy=0x1016034b0, id={asBits = 6}, bp=0x7fff5fbfd04f) at ../jsproxy.cpp:95
#7  0x00000001001250ce in js::JSScriptedProxyHandler::has (this=0x1003a8fc0, cx=0x100712a30, proxy=0x1016034b0, id={asBits = 6}, bp=0x7fff5fbfd04f) at ../jsproxy.cpp:565
#8  0x0000000100121bc6 in js::JSProxy::has (cx=0x100712a30, proxy=0x1016034b0, id={asBits = 6}, bp=0x7fff5fbfd04f) at ../jsproxy.cpp:742
#9  0x0000000100121c0c in js::proxy_LookupProperty (cx=0x100712a30, obj=0x1016034b0, id={asBits = 6}, objp=0x7fff5fbfd188, propp=0x7fff5fbfd180) at ../jsproxy.cpp:813
#10 0x00000001001652ce in JSObject::lookupProperty (this=0x1016034b0, cx=0x100712a30, id={asBits = 6}, objp=0x7fff5fbfd188, propp=0x7fff5fbfd180) at jsobj.h:1029
#11 0x00000001000d9f05 in js_LookupPropertyWithFlags (cx=0x100712a30, obj=0x101603000, id={asBits = 6}, flags=65535, objp=0x7fff5fbfd188, propp=0x7fff5fbfd180) at ../jsobj.cpp:4464
#12 0x00000001000dd2b5 in js_GetPropertyHelper (cx=0x100712a30, obj=0x101603000, id={asBits = 6}, getHow=0, vp=0x7fff5fbfd280) at ../jsobj.cpp:4781
#13 0x00000001000ddccf in js_GetProperty (cx=0x100712a30, obj=0x101603000, id={asBits = 6}, vp=0x7fff5fbfd280) at ../jsobj.cpp:4865
#14 0x0000000100165328 in JSObject::getProperty (this=0x101603000, cx=0x100712a30, id={asBits = 6}, vp=0x7fff5fbfd280) at jsobj.h:1042
#15 0x00000001001859cc in js_GetDefaultXMLNamespace (cx=0x100712a30, vp=0x7fff5fbfd330) at ../jsxml.cpp:7232
#16 0x000000010018e159 in QNameHelper (cx=0x100712a30, obj=0x101603550, clasp=0x1003a37e0, argc=0, argv=0x101000158, rval=0x101000148) at ../jsxml.cpp:778
#17 0x000000010018e479 in QName (cx=0x100712a30, argc=0, vp=0x101000148) at ../jsxml.cpp:835
#18 0x00000001000c4810 in js::CallJSNative (cx=0x100712a30, native=0x10018e41a <QName(JSContext*, unsigned int, js::Value*)>, argc=0, vp=0x101000148) at jscntxtinlines.h:657
#19 0x00000001000c48c2 in js::CallJSNativeConstructor (cx=0x100712a30, native=0x10018e41a <QName(JSContext*, unsigned int, js::Value*)>, argc=0, vp=0x101000148) at jscntxtinlines.h:673
#20 0x00000001000c3138 in js::InvokeConstructorWithGivenThis (cx=0x100712a30, thisobj=0x101603550, fval=@0x7fff5fbfd670, argc=0, argv=0x0, rval=0x7fff5fbfd620) at jsinterp.cpp:1059
#21 0x00000001000de4b8 in js_InitClass (cx=0x100712a30, obj=0x101603000, parent_proto=0x1016030a0, clasp=0x1003a37e0, constructor=0x10018e41a <QName(JSContext*, unsigned int, js::Value*)>, nargs=2, ps=0x1003a43e0, fs=0x1003a3ea0, static_ps=0x0, static_fs=0x0) at ../jsobj.cpp:3435

and fail in js_InitClass, where we'll goto bad:

3484	bad:
3485	    if (named) {
3486	        Value rval;
3487	        obj->deleteProperty(cx, ATOM_TO_JSID(atom), &rval);
3488	    }

and that delete will try to recycle the reserved standard slot.
#22 0x0000000100185ba4 in js_InitQNameClass (cx=0x100712a30, obj=0x101603000) at ../jsxml.cpp:7047
#23 0x00000001000d70e9 in js_GetClassObject (cx=0x100712a30, obj=0x101603000, key=JSProto_QName, objp=0x7fff5fbfd7f8) at ../jsobj.cpp:3707
#24 0x00000001000db378 in js_FindClassObject (cx=0x100712a30, start=0x0, protoKey=JSProto_QName, vp=0x7fff5fbfd888, clasp=0x1003a37e0) at ../jsobj.cpp:3772
#25 0x00000001000deaa3 in js_ConstructObject (cx=0x100712a30, clasp=0x1003a37e0, proto=0x0, parent=0x0, argc=1, argv=0x7fff5fbfd9a0) at ../jsobj.cpp:3815
#26 0x0000000100188e99 in js_FindXMLProperty (cx=0x100712a30, nameval=@0x7fff5fbfdd50, objp=0x7fff5fbfdd48, idp=0x7fff5fbfdd40) at ../jsxml.cpp:7425
#27 0x00000001000bbed3 in js::Interpret (cx=0x100712a30, entryFrame=0x1010000e0, inlineCallCount=0, interpFlags=0) at ../jsinterp.cpp:6162

This interpreter activation is handling line 5, the "*" any-name reference error, which before it fails causes all the frames above -- ain't E4X grand?

But really, this could happen for other paths, I think you could do it with any standard class given lazy standard class init.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: fix and test

This all makes sense. I tried using ES5 meta-programming to break other lazy standard class init sequences, but it's hard: you have to block init from binding all names, but leave its goto bad path to delete the class name, then recycle that slot, then reinit. But Object.defineProperty resolves the target name, and the js_Init*Class helpers that bind multiple names (e.g. Number) init too soon.

/be

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 475236 [details] [diff] [review]
fix

OK, I get it now. You can trigger this directly:

    delete Object;  // puts slot 82 on freelist
    x = 12;         // puts x in Object's slot

Those two lines are not enough to make us assert or crash, but maybe you see a way to make things go bad from there (I don't) or maybe dvander can tell us.

The following two lines assert (with a nice short stack) if you type them into the shell, but not if you run them from a file:

    delete Object;
    var x;

So maybe this could be tested via HTML with

   <script>delete Object;</script>
   <script>var x;</script>

Anyway, r=me, but this one really needs a test.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 475384 [details] [diff] [review]
fix and test

Got it! Better test:

  var s = evalcx("");
  delete s.Object;
  evalcx("var x;", s);

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> Comment on attachment 475384 [details] [diff] [review]
> fix and test
> 
> Got it! Better test:
> 
>   var s = evalcx("");
>   delete s.Object;
>   evalcx("var x;", s);

Haha, so much better -- I was changing too little. Thanks, I'll use it instead unless you want two tests, big/over-complicated and this one.

Fuzzer should try this kind of code.

/be

Comment 12

[Jason Orendorff [:jorendorff]]

No strong opinion here either way.

Comment 13

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/cda6465ea42d

/be

Comment 14

[Brendan Eich [:brendan]]

(I did both tests on the theory that more coverage is better than less, and the bigger test exercises lazy standard class init while the small test uses default [eager] sandbox standard class init.)

/be

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/cda6465ea42d

Comment 17

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 18

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-595230-2.js.