321522 - The sandbox object's prototype comes from the calling code

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Blake Kaplan (:mrbkap) (inactive)]

moz_bug_r_a4 pointed out in bug 321101, comment 16 that the sandbox object's prototype comes from the creator's context. By definition (since Components.utils can only be accessed from chrome) this means that it's possible to access the protected global object.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Untested potential fix

I need to test to make sure this actually works. I wonder if we shouldn't store the sandbox's context in the sandbox object to avoid recreating it on the sandbox creation and then on each subsequent evalInSandbox.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 206835 [details] [diff] [review]
Untested potential fix

This seems to work and not break things.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 206835 [details] [diff] [review]
Untested potential fix

>-    JSObject *sandbox = JS_NewObject(cx, &SandboxClass, nsnull, nsnull);
>-    if (!sandbox)
>+    JSContext *tempcx = JS_NewContext(JS_GetRuntime(cx), 1024);
>+    if(!tempcx)
>+        return ThrowAndFail(NS_ERROR_OUT_OF_MEMORY, cx, _retval);
>+
>+    JSObject *sandbox = JS_NewObject(tempcx, &SandboxClass, nsnull, nsnull);
>+    if(!sandbox) {
>+        JS_DestroyContextNoGC(tempcx);
>         return ThrowAndFail(NS_ERROR_XPC_UNEXPECTED, cx, _retval);
>+    }

Seems like a hazardous pattern for C++, given NS_ENSURE_* and other return-hiding macros that might crop up.  Isn't there an auto-storage-class helper for managing a local JSContext?  If not, make one, or do the context-conservation-via-private-data thing -- but beware making an uncollectable GC cycle!

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With that addressed

Comment 5

[Brendan Eich [:brendan]]

incomplete patch?

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 206937 [details] [diff] [review]
With that addressed

>+        if (mGCOnFinalization)
>+            JS_DestroyContext(mContext);
>+        else
>+            JS_DestroyContextNoGC(mContext);
>+    }

Literal-minded me would prefer mGCOnDestroy, since Finalization denotes a phase of the JS GC, and you can't GC from within a running GC.

>+    operator JSContext * ()
>+    {return mContext;}

I'm a style nut: why is this better than all on one line?  Just askin'!

>-    JSObject *sandbox = JS_NewObject(cx, &SandboxClass, nsnull, nsnull);
>+    XPCAutoJSContext tempcx(JS_NewContext(JS_GetRuntime(cx), 1024), PR_FALSE);
>+    if (!tempcx)
>+        return ThrowAndFail(NS_ERROR_OUT_OF_MEMORY, cx, _retval);
>+
>+    JSObject *sandbox = JS_NewObject(tempcx, &SandboxClass, nsnull, nsnull);
>     if (!sandbox)
>         return ThrowAndFail(NS_ERROR_XPC_UNEXPECTED, cx, _retval);

Nice and small, thanks to one of C++'s few fully-righteous features (auto-storage-class class ctors and dtors ;-P).

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> I'm a style nut: why is this better than all on one line?  Just askin'!

I think that's what the style of the surrounding code is (xpconnect's style is kind of weird to me).

Comment 8

[Mike Shaver (:shaver emeritus)]

Comment on attachment 206937 [details] [diff] [review]
With that addressed

r=shaver

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk. I don't know what release to nominate this for, trying the obvious ones, though. This doesn't really do anything without a full patch for bug 321101, so 1.8.0.1 might be a bit optitmistic.

Comment 10

[Daniel Veditz [:dveditz]]

1.8.0.2 per comment 9

Comment 11

[Bob Clary [:bc] (inactive)]

note to self: this test will fail until bug 321101 is fixed.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 206937 [details] [diff] [review]
With that addressed

I need this no-risk patch on the branch for bug 265740 and co.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 206937 [details] [diff] [review]
With that addressed

a=me, for the right patch.

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch for the branch

This is the same patch as above, except that it isn't malformed.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branches.

Comment 16

[Bob Clary [:bc] (inactive)]

can not verify on 1.8.0.2 due to bug 328932

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed firefox 1.5.0.2/20060308 using domi on winxp/macosx

Comment 18

[Bob Clary [:bc] (inactive)]

ff2b2 verified fixed 1.8 windows/linux