The proxy code tries to protect against potential Proxy.fix calls that can mutate proxy into non-proxy unexpectedly. But currently such protection is incomplete. For example, JSProxyHandler::enumerateOwn contains: if (!getOwnPropertyNames(cx, proxy, idap)) return false; ... for (size_t n = 0; n < ida.length(); ++n) { ... if (!getOwnPropertyDescriptor(cx, proxy, vector[n], &desc)) return false; } This assumes that if getOwnPropertyNames or getOwnPropertyDescriptor return successfully then the proxy was not fixed and its class continue to be Proxy. For that the methods wraps the Trap method invocation into TryHandlerTrap. But they miss that the proxy could be fixed during later call to ArrayToJSIDArray or ParsePropertyDescriptorObject. Those methods use generic getProperty call which could run a scripted getter and fix the proxy.

Looking.

ArrayToJSIDArray and ParsePropertyDescriptorObject use a generic API to access the object that was returned. No matter what that object is (proxy or not), it will work fine. ArrayToJSIDArray is the last operation in each trap and doesn't touch the original proxy object any more, so no matter in what state the proxy is in (fixed or not), all is fine. Similarly, ParsePropertyDescriptorObject doesn't touch the proxy object and always happens last, except for the defineProperty case. The attached patch catches that case.

(In reply to comment #2) > ArrayToJSIDArray and ParsePropertyDescriptorObject use a generic API to access > the object that was returned. No matter what that object is (proxy or not), it > will work fine. ArrayToJSIDArray is the last operation in each trap and doesn't > touch the original proxy object any more, so no matter in what state the proxy > is in (fixed or not), all is fine. It is the last operation in the trap but it is not the last operation in JSProxyHandler::enumerateOwn: bool JSProxyHandler::enumerateOwn(JSContext *cx, JSObject *proxy, JSIdArray **idap) { if (!getOwnPropertyNames(cx, proxy, idap)) return false; AutoIdArray ida(cx, *idap); size_t w = 0; jsid *vector = (*idap)->vector; AutoDescriptor desc(cx); for (size_t n = 0; n < ida.length(); ++n) { JS_ASSERT(n >= w); vector[w] = vector[n]; if (!getOwnPropertyDescriptor(cx, proxy, vector[n], &desc)) return false; if (desc.obj && (desc.attrs & JSPROP_ENUMERATE)) ++w; } (*idap)->length = w; ida.steal(); return true; } getOwnPropertyDescriptor assumes that the proxy is a Proxy. Yet what prevents in the above loop the parser of the descriptor to fix the proxy?

also handle enumerateOwn

Another fragment: static JSBool proxy_SetAttributes(JSContext *cx, JSObject *obj, jsid id, JSProperty *prop, uintN *attrsp) { /* Lookup the current property descriptor so we have setter/getter/value. */ AutoDescriptor desc(cx); if (!JSProxy::getOwnPropertyDescriptor(cx, obj, id, &desc)) return false; desc.attrs = (*attrsp & (~JSPROP_SHORTID)); return JSProxy::defineProperty(cx, obj, id, &desc); } Here again getOwnPropertyDescriptor could fix the proxy during parsing.

JSProxyHandler::set has the same problem due to the usage of getOwnPropertyDescriptor.

Guard all handler functions for unexpected proxy fixing.

Comment on attachment 448657 [details] [diff] [review] patch Redirect to local expert on the problem at hand ;-) /be

Comment on attachment 448657 [details] [diff] [review] patch There is another pattern that exists at some call sites even with the patch. When the proxy is fixed, its handler becomes unrooted and could be GC-ed before the use. Consider, for example, > bool > JSScriptedProxyHandler::has(JSContext *cx, JSObject *proxy, jsid id, bool *bp) > { >- JSObject *handler = JSVAL_TO_OBJECT(proxy->getProxyHandler()); >+ JSObject *handler = GetHandlerObject(cx, proxy); >+ if (!handler) >+ return false; > AutoValueRooter tvr(cx); > if (!DerivedTrap(cx, handler, ATOM(has), tvr.addr())) > return false; > if (!js_IsCallable(tvr.value())) > return JSProxyHandler::has(cx, proxy, id, bp); > return TryHandlerTrap(cx, proxy, Trap1(cx, handler, tvr.value(), id, tvr.addr())) && > ValueToBool(cx, tvr.value(), bp); Here DerivedTrap can fix the property in the getter. When the getter returns, the handler becomes unrooted if it was only referenced via the proxy. It can be GC-ed in the ValueToString call that Trap1 uses if the latter triggers the last-ditch GC when allocating a string for the numeric id. AFAICS these problems could fixed if any call that potentially can run a script will check that the proxy stays proxy. That is, GetTrap, Trap and array id and property descriptor parsers should do such checks.

(In reply to comment #11) > Created an attachment (id=448702) [details] > patch This looks rather lengthy compared with the direct checks at the places of a potential script invocations. Any reasons for that? IMO a blanket ban on fixing a proxy while any operation its operation is running would be better than a selective black listing.

I am not sure what you mean and how you want to do that.

(In reply to comment #11) > Created an attachment (id=448702) [details] > patch The patch also need to check in FixProxy that JSProxy::fix has not fixed the proxy already.

(In reply to comment #13) > I am not sure what you mean and how you want to do that. The proxy can be unexpectedely fixed at the following places: 1. During the Trap call. 2. During GetTrap call. 3. During ParsePropertyDescriptorObject. 4. During ArrayToJSIdArray. I suggest to check that a proxy still the Proxy at the end of these calls rather than chase all the callers.

How is that different from what the patch does? The patch in the bug fixes the problem for all cases and is easy to read and symmetric. Furthermore, we will soon remove about half the lines when we have conservative stack scanning, and the rest when I am done with my ObjectOps makeover. What exactly is #15 trying to optimize for?

(In reply to comment #16) > How is that different from what the patch does? The patch in the bug fixes the > problem for all cases and is easy to read and symmetric. Regarding "all cases" see the comment 14. > What exactly is #15 trying > to optimize for? The source of problems here is a script invocation that can unexpectedly fix a proxy. If all such invocations would be protected with isProxy check then we can be sure that there is no bug here. Otherwise some cases could still be missed.

(In reply to comment #18) > Created an attachment (id=448705) [details] > patch It looks like the patch covers all the known cases. I will try to look for unknown ones one more time today ;)

js_PopulateObject call in FixProxy right before proxy->swap can also fix the proxy. But this does not look like a hazard.

proxy_SetAttributes contains: if (!JSProxy::getOwnPropertyDescriptor(cx, obj, id, &desc)) return false; desc.attrs = (*attrsp & (~JSPROP_SHORTID)); return JSProxy::defineProperty(cx, obj, id, &desc); where JSProxy::defineProperty starts with: jsval handler = proxy->getProxyHandler(); if (JSVAL_IS_OBJECT(handler)) Here getOwnPropertyDescriptor can return non-proxy and JSProxy::defineProperty does not check for that.

The patch does not protect against fixing the property in JSNoopProxyHandler. For example, AFAICS JS_GetPropertyDescriptorById that JSNoopProxyHandler::getPropertyDescriptor calls can run an arbitrary script via, for example, making another proxy a prototype of the proxy-wrapped object.

(In reply to comment #21) > where JSProxy::defineProperty starts with: > > jsval handler = proxy->getProxyHandler(); > if (JSVAL_IS_OBJECT(handler)) Not to waste time any father I suggest to make proxy->getProxyHandler to check that the proxy is still a proxy. That would really close the hole.

New attempt. Disallow fixing while any other operation is pending on a proxy.

(In reply to comment #24) > Created an attachment (id=448868) [details] > patch Yes, this is much better. The only remaining issue AFAICS is a call to js_PopulateObject in FixProxy right before proxy->swap. That call can also fix the proxy. It seems harmless but lets close this hole as well with a AutoNoFix right after the OperationInProgress check in FixProxy. I assume that C++ proxy handlers cannot be called directly and will always stay behind the JSProxy:: , right? But even if so it would be nice to assert that given that the added AutoNoFix allow to do that trivially via JS_ASSERT(OperationInProgress(cx, proxy)). That also suggests to rename AutoNoFix into AutoOperationCheck or something, but I would not insist on that. r+ with that addressed.

Yet one more issue: for consistency I think the patch should use AutoNoFix in proxy_Call and proxy_Construct when those calls js_InternalCall. Although these are harmless, it would allow to enforce a simple rule that proxy cannot be fixed during *any* operation.

- protecting js_PopulateObject against fixing now - protecting the call/construct hooks now, this was actually a bug since we reach into fslots after the ::get in case of construct - assert that code is only entered via the monitor class (JSProxy) - renamed auto object to AutoPendingProxyOperation

Comment on attachment 449116 [details] [diff] [review] patch >diff --git a/js/src/jsproxy.cpp b/js/src/jsproxy.cpp >+static bool >+OperationInProgress(JSContext *cx, JSObject *proxy) >+{ >+ JSThreadData *data = JS_THREAD_DATA(cx); >+ JSPendingProxyOperation *op = data->pendingProxyOperation; A single nit for a nice patch: the extra data intermediate local does not add clarity and could be removed.

I dropped the !OperationInProgress checks from JSNoopProxyHandler which is now JSWrapper and lives in a separate file. Its just one possible C++ handler implementation. Not all handlers check this anyway. All the jsproxy.cpp internal checks remain. I fixed #30 too.

(In reply to comment #31) > I dropped the !OperationInProgress checks from JSNoopProxyHandler which is now > JSWrapper and lives in a separate file. Its just one possible C++ handler > implementation. Not all handlers check this anyway. A better solution would be to have non-virtual public methods in the base class containing the necessary asserts calling private virtual implementation. It would be harder to abuse that.

Does this apply to the branches? Isn't the proxy stuff only on trunk?

This is not on any branch, but it is on trunk.

Why did this move to beta2? This is fixed.