Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 14 years ago

var x = wrap.call(x, Function);
if (x.__proto__ = x) {
    print(x);
}

crashes js opt shell on TM tip without -j at js_LookupPropertyWithFlags and crashes js debug shell on TM tip without -j at JSAutoResolveFlags

Turning s-s due to scary memory address (we seem to be calling from a scary address).

===

Program received signal SIGSEGV, Segmentation fault.
0x080b40de in js_LookupPropertyWithFlags ()
(gdb) bt
#0  0x080b40de in js_LookupPropertyWithFlags ()
#1  0x00000000 in ?? ()
(gdb) x/i $eip
=> 0x80b40de <js_LookupPropertyWithFlags+14>:	call   0x804f22e <__i686.get_pc_thunk.bx>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   42437:4dd9be00049c
user:        Andreas Gal
date:        Tue May 18 19:21:43 2010 -0700
summary:     Implement ES Harmony Proxies (bug 546590, r=mrbkap).

	Andreas Gal :gal Assignee
	Comment 1 • 14 years ago

stack overflow. wrap() is not in the browser, only in the shell. probably not critical for the browser. looking.

	Andreas Gal :gal Assignee
	Comment 2 • 14 years ago

We make a wrapper, set __proto__ on it, and then recurse to death in the lookup. Lovely. I wonder why the proto set isn't disallowed.

var x = wrap.call(x, {});
x.__proto__ = x;
print(x);

	Andreas Gal :gal Assignee
	Comment 3 • 14 years ago

This is kinda cute. So we do x.__proto__ = x, but x is a wrapper, so it forwards the proto set to the underlying object and that one gets x (the wrapper) as proto. Freaky. My gut feeling is we should censor __proto__ for wrappers, or maybe even for all proxies. Paging jorendorff and mrbkap for 2nd and 3rd opinions.

	Andreas Gal :gal Assignee
	Comment 4 • 14 years ago

Can be done with script, most likely, so its a dos for 1.9.3. I can easily fix the recursion, but I would rather just block the whole vector.

	Brendan Eich [:brendan]
	Comment 6 • 14 years ago

Why does the underlying object get "x (the wrapper) as proto"? Please diagnose that before censoring __proto__.

/be

	Andreas Gal :gal Assignee
	Comment 7 • 14 years ago

x.__proto__ = x; thats why

	Jason Orendorff [:jorendorff]
	Comment 8 • 14 years ago

Comment on attachment 450302 [details] [diff] [review]
patch

I'm feeling cynical about this fix. Andreas, are you convinced __proto__ is the only way to trigger runaway recursion using proxies? JSProxy and JSScriptedProxyHandler seem to offer plenty of opportunities. Dynamic dispatch makes it hard to believe there are any real guarantees.

Please convince me ...or just add a JS_CHECK_RECURSION call in each proxy op.

	Andreas Gal :gal Assignee
	Comment 9 • 14 years ago

There is a JS_CHECK_RECURSION in every handler trap (see Trap). This is not a recursion in the proxy code. Its a recursion in LookupPropertyWithFlags, based on the result returned by proxy handlers. We expect proto walk to be cycle free, but in this particular case its not.

	Jason Orendorff [:jorendorff]
	Comment 11 • 14 years ago

The recursion path is:

  JSObject::lookupProperty
  -> proxy_LookupProperty
  -> js::JSProxy::has
  -> js::JSWrapper::has
  -> JS_HasPropertyById
  -> LookupPropertyById
  -> JSObject::lookupProperty
  -> js_LookupProperty
  -> js_LookupPropertyWithFlags
  -> JSObject::lookupProperty

So moving the JS_CHECK_RECURSION to JSProxy::has from Trap will fix this. Trap is too easy to miss, that's all.

	Jason Orendorff [:jorendorff]
	Comment 12 • 14 years ago

Comment on attachment 450381 [details] [diff] [review]
patch

> JSString *
> JSProxy::obj_toString(JSContext *cx, JSObject *proxy)
> {
>+    JS_CHECK_RECURSION(cx, return false);
>     AutoPendingProxyOperation pending(cx, proxy);
>     return proxy->getProxyHandler()->obj_toString(cx, proxy);
> }
> 
> JSString *
> JSProxy::fun_toString(JSContext *cx, JSObject *proxy, uintN indent)
> {
>+    JS_CHECK_RECURSION(cx, return false);
>     AutoPendingProxyOperation pending(cx, proxy);
>     return proxy->getProxyHandler()->fun_toString(cx, proxy, indent);
> }

'return NULL' for these last two. r=me with that nit picked.

	Jason Orendorff [:jorendorff]
	Comment 13 • 14 years ago

This needs to be landed.

	Sheila Mooney
	Comment 14 • 14 years ago

Looks like this just needs to be checked in? Is there some other reason it's been sitting here since the end of July?

	David Mandelin [:dmandelin]
	Comment 15 • 14 years ago

The patch is pretty seriously bitrotted.

	Andreas Gal :gal Assignee
	Comment 17 • 14 years ago

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

	Robert Sayre
	Comment 18 • 14 years ago

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

	Christian Holler (:decoder)
	Comment 19 • 12 years ago

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929