584578 - "Assertion failure: watchpoint access check failed" with proxy, watch

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

TM branch, debug shell, no JITs.

var x = Proxy.create( {get:function(r,name){return {}[name]}} );
x.watch('e', function(){});

Assertion failure: watchpoint access check failed, at ../jsobj.cpp:1328

This assertion was added in rev c5e31473b1a6 (bug 583850).  Since it's a security-related assertion, and I don't know whether it affects older versions, I'm filing it as security-sensitive.

Comment 1

[Andreas Gal :gal]

#0  0x0000000100167795 in JS_Assert (s=0x10020b4c0 "watchpoint access check failed", file=0x10020a11b "../jsobj.cpp", ln=1326) at ../jsutil.cpp:80
#1  0x00000001000d83b8 in obj_watch (cx=0x1005126b0, argc=2, vp=0x1010001b8) at ../jsobj.cpp:1326
#2  0x00000001000a5ba0 in js::Interpret (cx=0x1005126b0) at ../jsinterp.cpp:4709
#3  0x00000001000b97a3 in js::Execute (cx=0x1005126b0, chain=0x101503000, script=0x100514680, down=0x0, flags=0, result=0x0) at jsinterp.cpp:907
#4  0x00000001000160df in JS_ExecuteScript (cx=0x1005126b0, obj=0x101503000, script=0x100514680, rval=0x0) at ../jsapi.cpp:4730
#5  0x000000010000a3e5 in Process (cx=0x1005126b0, obj=0x101503000, filename=0x7fff5fbffa95 "x.js", forceTTY=0) at ../../shell/js.cpp:440
#6  0x000000010000b01b in ProcessArgs (cx=0x1005126b0, obj=0x101503000, argv=0x7fff5fbff948, argc=1) at ../../shell/js.cpp:854
#7  0x000000010000b103 in shell (cx=0x1005126b0, argc=1, argv=0x7fff5fbff948, envp=0x7fff5fbff958) at ../../shell/js.cpp:5047
#8  0x000000010000b1ff in main (argc=1, argv=0x7fff5fbff948, envp=0x7fff5fbff958) at ../../shell/js.cpp:5134
(gdb)

Comment 2

[Andreas Gal :gal]

This is a bogus assert. CheckAccess can fail (the proxy handler throws a regular exception here). We can't intercept proxies here, because the failure could also come from the proto chain. I think this can already fail in other cases too.

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Andreas Gal :gal]

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

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>--- a/js/src/jsobj.cpp
>+++ b/js/src/jsobj.cpp
>@@ -1320,20 +1320,18 @@ obj_watch(JSContext *cx, uintN argc, Val
> 
>     JSObject *obj = ComputeThisFromVp(cx, vp);
>     if (!obj)
>         return false;
> 
>     /* Legacy security check. This can't fail. See bug 583850. */
>     Value tmp;
>     uintN attrs;
>-    if (!CheckAccess(cx, obj, propid, JSACC_WATCH, &tmp, &attrs)) {
>-        JS_NOT_REACHED("watchpoint access check failed");
>+    if (!CheckAccess(cx, obj, propid, JSACC_WATCH, &tmp, &attrs))
>         return false;
>-    }
> 
>     vp->setUndefined();
> 
>     if (attrs & JSPROP_READONLY)
>         return true;
>     if (obj->isDenseArray() && !obj->makeDenseArraySlow(cx))
>         return false;
>     return JS_SetWatchPoint(cx, obj, propid, obj_watch_handler, callable);

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to comment #2)
> This is a bogus assert. CheckAccess can fail (the proxy handler throws a
> regular exception here). We can't intercept proxies here, because the failure
> could also come from the proto chain.

OK, but doesn't this basically mean the whole patch in bug 583850 should be backed out and we should try again later with a different approach?

Comment 6

[Jason Orendorff [:jorendorff]]

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

Actually I think this is the wrong kind of fix -- let me give it a shot real quick.

Comment 7

[Andreas Gal :gal]

Sure, sounds good.

Comment 8

[Jason Orendorff [:jorendorff]]

It turns out the assertions were discovering places where we really do need the security checks.

So the assertions are wrong. Backed them out (Friday morning).

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

Pushed the test just now.

http://hg.mozilla.org/tracemonkey/rev/9fb00422e957

Comment 9

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

Adding fixed-in-tracemonkey per comment 8..

Comment 10

[Christian Holler (:decoder)]

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