Closed Bug 518991 Opened 10 years ago Closed 10 years ago

COWs should only expose certain properties


(Core :: XPConnect, defect)

Not set



Tracking Status
status1.9.2 --- beta1-fixed


(Reporter: mrbkap, Assigned: mrbkap)





(1 file, 1 obsolete file)

We need to be able to only expose certain properties off of COWs for JetPack. Note that there might need to be some additional exceptions for XPConnected objects that are wrapped in COWs (like the FeedWriter and Sidebar objects).
Attached patch Immediate fix for this bug (obsolete) — Splinter Review
This fixes this bug. The one thing it doesn't do is the wrapped native stuff. In order to let existing code continue to work, I'm letting objects that don't specify __exposedProperties__ at all to be totally open (modulo wrapper goodness). I'll file a followup bug on that.
Attachment #403924 - Flags: review?(jst)
diff --git a/js/src/xpconnect/src/XPCChromeObjectWrapper.cpp b/js/src/xpconnect/src/XPCChromeObjectWrapper.cpp
--- a/js/src/xpconnect/src/XPCChromeObjectWrapper.cpp
+++ b/js/src/xpconnect/src/XPCChromeObjectWrapper.cpp
@@ -176,32 +176,38 @@ JSBool
 CanTouchProperty(JSContext *cx, JSObject *wrapperObj, jsid id, JSBool isSet,
                  JSBool *allowedp)
   jsval exposedProps;
   if (!JS_GetReservedSlot(cx, wrapperObj, sExposedPropsSlot, &exposedProps)) {
     return JS_FALSE;
-  if (!JSVAL_IS_OBJECT(exposedProps)) {
+  if (JSVAL_IS_PRIMITIVE(exposedProps)) {
     // TODO For now, if the object doesn't ask for security, provide full
     // access. In the future, we want to default to false here.
-    *allowedp = JS_TRUE;
+    // NB: We differentiate between void (no __exposedProps__ property at all)
+    // and null (__exposedProps__ exists but didn't specify any properties)
+    // here.
+    *allowedp = JSVAL_IS_VOID(exposedProps);
     return JS_TRUE;
   JSObject *hash = JSVAL_TO_OBJECT(exposedProps);
   jsval allowedval;
   if (!JS_LookupPropertyById(cx, hash, id, &allowedval)) {
     return JS_FALSE;
   const PRUint32 wanted = isSet ? sPropIsWritable : sPropIsReadable;
-  *allowedp = (JSVAL_TO_INT(allowedval) & wanted) != 0;
+  // We test JSVAL_IS_INT to protect against unknown ids.
+  *allowedp = JSVAL_IS_INT(allowedval) &&
+              (JSVAL_TO_INT(allowedval) & wanted) != 0;
   return JS_TRUE;
 static JSBool
 XPC_COW_AddProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp);
Attachment #403924 - Attachment is obsolete: true
Attachment #403938 - Flags: review?(jst)
Attachment #403924 - Flags: review?(jst)
Attachment #403938 - Flags: review?(jst) → review+
Closed: 10 years ago
Resolution: --- → FIXED
This also caused some Windows unittest orange -- different clusters of test failures on different tinderbox-columns, but with each column staying consistent in its failures since the checkin.  The various test-failures were:
 - test_bug408328.html (same test from comment 4)
 - 6036895 byte leak
 - geolocation test timeouts: test_allowCurrent.html, test_allowWatch.html, test_manyCurrentSerial.html, test_manyWatchSerial.html
 - Exception... "Security Manager vetoed action" in test_bug427633_no_newfolder_if_noip.xul and test_bug485100-change-case-loses-tag.xul

Failure logs covering all of the above issues:
(In reply to comment #5)
>  - Exception... "Security Manager vetoed action" in
> test_bug427633_no_newfolder_if_noip.xul and
> test_bug485100-change-case-loses-tag.xul

FWIW, this exception happened for test_bug485100-change-case-loses-tag.xul on a Linux tinderbox, too:
Feh, I typo'd && for || and read some unintialized memory in the NewResolve hook (the part that doesn't have unit tests yet). I'll have to re-push tomorrow.
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Attachment #403938 - Flags: approval1.9.2?
Attachment #403938 - Flags: approval1.9.2? → approval1.9.2+
You need to log in before you can comment on or make changes to this bug.