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

RESOLVED FIXED

Status

()

--
minor
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: jruderman, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
x86
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
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.

Updated

8 years ago
Assignee: general → gal

Updated

8 years ago
Blocks: 546590
blocking2.0: --- → betaN+

Updated

8 years ago
Whiteboard: [sg:critical]

Updated

8 years ago
Whiteboard: [sg:critical] → [sg:critical][critsmash:investigating]

Comment 1

8 years ago
#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

8 years ago
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.
Group: core-security
Severity: critical → minor
Whiteboard: [sg:critical][critsmash:investigating]

Updated

8 years ago
Attachment #464722 - Flags: review?(jorendorff)

Comment 4

8 years ago
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);
Attachment #464722 - Attachment is private: false
(Assignee)

Comment 5

8 years ago
(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?
(Assignee)

Comment 6

8 years ago
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.
Attachment #464722 - Flags: review?(jorendorff) → review-

Comment 7

8 years ago
Sure, sounds good.
Assignee: gal → jorendorff
(Assignee)

Comment 8

8 years ago
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
Adding fixed-in-tracemonkey per comment 8..
Whiteboard: fixed-in-tracemonkey

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-584578.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.