337095 - [FIX]Sort out XPCNativeWrapper behavior for calls from C++ to JS

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See comments in bug 336875.  Basic issue is that when calling from C++ to JS we currently use whatever random script is on the stack to determine whether to create an XPCNativeWrapper.  This is wrong.  I propose to condition the current behavior on ccx.GetXPCContext()->CallerTypeIsJavaScript().

I also think I can get at the script filename of the thing we're calling into, with a bit of work, so things that set xpcnativewrappers=no will continue to work right.

The question is whether this is a behavior change we're willing to take on the stable branch.  I think we should, but it's kinda scary...  To avoid breaking adblock we should also make sure that Components.utils.lookupMethod works with XPCNativeWrappers; I don't think it does right now.  Can anyone think of other problems this will cause?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 221376 [details] [diff] [review]
Like so

I'm going to be scarce for a while here and at XTech next week, but:

* factor jsdbgapi.h into minimal pieces: that means just JS_GetScriptFilenameFlags, and the caller has to go from function to script using JS_GetFunctionScript (and JS_GetPrivate if necessary).

* Nits: JS does not use the C++ wrong-headed style of cuddling * with the typename on its left -- * is a declarator mode, it cuddles the declarator name.

/be

Comment 3

[Boris Zbarsky [:bzbarsky]]

> that means just JS_GetScriptFilenameFlags

So the loop over cx->fp should also move into the caller?

I'll make the other changes.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> > that means just JS_GetScriptFilenameFlags
> 
> So the loop over cx->fp should also move into the caller?

I perpetrated JS_GetTopScriptFilenameFlags in an attempt to make an ad-hoc API, instead of factoring fully.  I regret this error :-/.  Is it undo-able?  I think so, especially if we can get the factoring fix into the js1.6 standalone release.

> I'll make the other changes.

Thanks,

/be

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: With Brendan's issues addressed

Comment 6

[Boris Zbarsky [:bzbarsky]]

We should also figure out whether we want this for the 1.8 branches.  It's sorta an API change, a bit....

Comment 7

[Jay Patel [:jay]]

Can't take this for 1.8.0.5 due to API change risks.  If there is a safer fix, we can reconsider.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Jay, the "API change" is that stuff that used to be unsafe is now made safe.  This does change what chrome code sees in some cases, of course -- it no longer sees unsafe stuff.

There's no way to make this "safer" short of not taking the security fix.

Renominating; I'd like us to do an actual evaluation of the security issue and then deciding how much we care about fixing it.

Comment 9

[Daniel Veditz [:dveditz]]

Marking blocking for 1.8.0.5, but with jst and brendan out on vacation right now it'll be tight to get it into the release. We'll probably have to forego our usual requirement to get trunk baking time, so we'll need extra QA attention to this on the branches.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I bet dbradley, shaver, mrbkap would be qualified to review this (though I'd still like jst and brendan to OK the concept and brendan in particular to OK the jsdbgapi.h change...

Not changing the review flags because I'm not sure who all is free, exactly, but I'd love drive-by reviews.

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I don't think this is the patch we want on the 1.8.0 branch, where we should leave the old API in place and just add the new one for this new caller.  (We're already breaking bincompat in 1.8.1, so we can follow the trunk there.)

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 221761 [details] [diff] [review]
With Brendan's issues addressed

r=jst, but I agree with shaver on leaving the old API in place on the branch and just adding the new one.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 221761 [details] [diff] [review]
With Brendan's issues addressed

shaver says sr=him for trunk and 1.8 branch.  I'll work on a 1.8.0 branch patch.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8.0 branch version

Same as the other, but with only API addition; the only changes from the other patch are in jsdbgapi.{h,cpp}, so I figure jseng owner review is enough.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 221761 [details] [diff] [review]
With Brendan's issues addressed

>+ * Get the script filename flags for the script.  If the script
>+ * doesn't have a filename, return JSFILENAME_NULL.

Too many uses of "script" there, lose the first one ("Get the filename flags for the script.").

>+                // else don't create XPCNativeWrappers, I guess.
>+  // No script means we should bypass, I guess?  That's what the old code did.

No guessing, either we like the original design, or we fix it here, or file bugs and cite them in FIXME comments.

Nit: func and funObj are used for the same kind of pointer, unify on funobj or callee to match other code in xpconnect and js?

Main suggestion: why not add mFunObj or mCallee to XPCCallContext and avoid the need to add yet another parameter in lots of places?

/be

Comment 16

[Boris Zbarsky [:bzbarsky]]

> or file bugs and cite them in FIXME comments.

I'll file bugs to investigate that stuff as needed.

I can unify on funObj, sure.

> why not add mFunObj or mCallee to XPCCallContext

I want something provably correct that I won't mind landing on the branches.  I don't have a good feel for when such members of an XPCCallContext could get clobbered (e.g. whether any of this code can be reentered and when); to feel confident about it I'd need to understand a lot more about control flow through XPConnect than I do now or will in time for any of the 1.8.0 security releases I've wanted to target this at.

If someone can prove to me that this would be a safe change, I'm willing to do it, I guess.  Or I can file a followup bug on trunk to do it; whichever folks prefer.

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> 
> I can unify on funObj, sure.

"funobj" all lower-case, a grep in xpconnect/src shows, is the canonical spelling.

> > why not add mFunObj or mCallee to XPCCallContext
> 
> I want something provably correct that I won't mind landing on the branches.  I
> don't have a good feel for when such members of an XPCCallContext could get
> clobbered (e.g. whether any of this code can be reentered and when);

XPCCallContext is storage class auto, LIFO allocated on the thread stack, not subject to clobbering.  Adding a member to it is equivalent to adding a local variable that can be referenced outside of C++ function scope, which is why it wins for this case -- only some calls need to pass the funobj through.

/be

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with an mCallee member

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with an mCallee member for real

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8.0.x branch version

The only difference here is the jsdbgapi code.

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 225545 [details] [diff] [review]
Now with an mCallee member for real

>+  // Check what the script calling us looks like
>+  JSScript *script = nsnull;
>+  JSStackFrame *fp = cx->fp;
>+  while(!script && fp)
>+  {
>+    script = fp->script;
>+    fp = fp->down;
>+  }

Nit: prevailing style in XPCNativeWrapper.cpp is jst-ish/dom-like: opening brace on same line as control keyword and condition, not on its own line.

sr=me with that -- and I am happy to branch-approve too, but if you want to wait till jst is back, I won't object.

/be

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 225546 [details] [diff] [review]
1.8.0.x branch version

Did we resolve to break ABI (not that anyone cares about jsdbgapi.h stuff added only for XPCNativeWrapper usage) in 1.8.1?  Ok by me, just checking.  I was considering restoring the lost JSFunctionSpec ABI compat for JS 1.7 in Gecko 1.8.1.

If we want to keep compat, then this is good for both branches, obviously.

/be

Comment 23

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 225546 [details] [diff] [review]
1.8.0.x branch version

ABI is broken by the JSFunctionSpec changes that will come as part of the JS1.7 -> branch landing, no?  I thought we were already committed to that.  If I'm mistaken and we're not, then we should probably take the 1.8.0 patch on 1.8.1 as well.

(Sort of a shame that we're breaking ABI in 1.8 rather than 1.9, but I think we'll probably live.  Especially if we reach out early to extension and plugin authors, which I guess is my job!)

r=shaver on the assumption (!) that I'm not mistaken above about JSFunctionSpec.

Comment 24

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 225546 [details] [diff] [review]
1.8.0.x branch version

Dammit, I need to read better.  Sorry, yeah, if we're restoring the JSFunctionSpec stuff, we should take the 1.8.0 patch on 1.8.  So let's just take it anyway, rather than have to change it later.

181=shaver

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk patch updated to all comments.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8/1.8.0 branch patch updated to all comments

Comment 27

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk and 1.8.1 branch.  Let the baking begin!

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 225637 [details] [diff] [review]
1.8/1.8.0 branch patch updated to all comments

approved for 1.8.0 branch, a=dveditz for drivers

Comment 29

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.8.0.5.

Comment 30

[Jay Patel [:jay]]

Is there a way to test these changes on the 1.8.0 branch to verify the fix?

Comment 31

[Boris Zbarsky [:bzbarsky]]

I tested by installing adblock, then editing the code it installed to output what node it actually got from our C++.  You might be able to reproduce with the "minimal testcase" from bug 336875 and some changes to the code to see whether you're actually getting an XPCNativeWrapper.