674571 - xpcom proxy use in security/manager/ssl can lead to scripted QI off main thread

Status: RESOLVED DUPLICATE of bug 675221

(Core :: Security: PSM, defect)

Description

[Luke Wagner [:luke]]

Attachment: fix

Spun off from bug 667535 to handle just the security/manager/ssl case.

Comment 1

[Luke Wagner [:luke]]

Oops, wrong component.

Comment 2

[Luke Wagner [:luke]]

Any ETA for this review?

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 548817 [details] [diff] [review]
fix

Embarrassingly, there is in fact, no assigned reviewer (b/c bugzilla silently ignores r? typos).

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 548817 [details] [diff] [review]
fix

Thanks for your patch.

I think your patch looks fine in general, but I'd like to request some improvements.


(a) Must change:

Can you please explain, in one or two sentences, 
why the use of runnable/dispatch is better than
using a proxy to the main thread?

Also mention that you use SYNC,
because you want to wait for the result.

Please include that as a comment in the code,
at the first place where you use ->Dispatch.


(b) Must change:

You unnecessarily add the mainThread variable twice.
I believe the second one isn't necessary.
You didn't get a compiler warning, because your second one
is in a nested sub-block.


(c)
Your implementation of nsNotifyCertProblemRunnable
is straightforward and looks good to me.


(d) Optional, but recommended changes:

>+class nsIsStsHostRunnable : public nsIRunnable
>+{
>+ public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIRUNNABLE
>+
>+  nsIsStsHostRunnable(nsCOMPtr<nsIStrictTransportSecurityService> stss)

This will trigger an nsCOMPtr copy constructor, which is unnecessary.
Make that "nsCOMPtr<...> & stss".


>+    : stss(stss), nsrv(NS_ERROR_UNEXPECTED)
>+  {}

I would be good style to add
  ,stsEnabled(PR_FALSE)
in order to avoid random contents of your bool.


This:
>+  nsXPIDLCString &GetHostNameRef() { return hostName; }
and this:
>+  nsrv = infoObject->GetHostName(getter_Copies(runnable->GetHostNameRef()));

wasn't obvious immediately.

You're calling two functions, which are both named "Get", but essentially
this operation is a "Set".

At the very least, please add a comment like
  // Inject hostname into runnable.


I personally would have done this differently,
to make it easier to understand.

Given that you allow anyone direct access to the hostName variable anyway,
there is no point to make that member variable private.

I'd change "hostName" into a public member, 
remove the GetHostNameRef function,
and change the GetHostName call to refer to runnable.hostName directly.

In my opinion, this would be much easier to read.


>+  nsresult GetResult(PRBool *b) const { *b = stsEnabled; return nsrv; }

In my opinion, you shouldn't mix pointers and references in your interface.

For consistency, I'd change that into
  nsresult GetResult(PRBool &b) const { b = stsEnabled; return nsrv; }
(which also avoids the need for parameter null checking).

Comment 5

[Kai Engert (:KaiE:)]

r=kaie if you address my requests, but feel free to rerequest a final review of your new patch. I will rereview quickly.

Comment 6

[Luke Wagner [:luke]]

(In reply to Kai Engert (:kaie) from comment #4)
> Can you please explain, in one or two sentences, 
> why the use of runnable/dispatch is better than
> using a proxy to the main thread?

I would be happy to but the comment would just say "cross-thread xpcom/proxies may not be used for scripted classes" but this comment will (in theory) be when bsmedberg removes xpcom/proxies altogether (bug 667535 comment 11).  Thus, not-using-proxies is just the right thing to do.  Still want a comment?

> This:
> >+  nsXPIDLCString &GetHostNameRef() { return hostName; }
> and this:
> >+  nsrv = infoObject->GetHostName(getter_Copies(runnable->GetHostNameRef()));
> 
> wasn't obvious immediately.

For this I was just copying the existing:

-  // now grab the host name to pass to the STS Service
-  nsXPIDLCString hostName;
-  nsrv = infoObject->GetHostName(getter_Copies(hostName));

but I did fail to preserve the comment.  I'll add it back.

> >+  nsresult GetResult(PRBool *b) const { *b = stsEnabled; return nsrv; }
> 
> In my opinion, you shouldn't mix pointers and references in your interface.

In SM, we resolved to *always* use pointers as outparams because it aids readability at the call site since you can syntactically see in foo(&bar) that bar is an outparam whereas, with mutable &, you just see foo(bar) and need to go look at the signature.  We also don't null check unnecessarily when non-null is part of the calling precondition.  Your code, though, so I'll make the change.

Comment 7

[Luke Wagner [:luke]]

Attachment: fix v2

Comment 8

[Kai Engert (:KaiE:)]

Comment on attachment 552119 [details] [diff] [review]
fix v2

(In reply to Luke Wagner [:luke] from comment #6)
> 
> In SM, we resolved to *always* use pointers as outparams because it aids
> readability at the call site since you can syntactically see in foo(&bar)
> that bar is an outparam whereas, with mutable &, you just see foo(bar) and
> need to go look at the signature.  We also don't null check unnecessarily
> when non-null is part of the calling precondition. 

Yeah, this is debatable. I'd argue, it's still readable, because the function is named Get*

Thanks a lot for addressing my proposals!

r=kaie

Comment 9

[Luke Wagner [:luke]]

Thanks!  I agree that it can certainly be readable for many cases.

http://hg.mozilla.org/integration/mozilla-inbound/rev/0cf822d12c64

Comment 10

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/0cf822d12c64

Comment 11

[Luke Wagner [:luke]]

Backed out to fix bug 678440 and a few other intermittent crashes:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5f0596a0b81e

We suspect that subtle difference between XPCOM proxies and Dispatch(SYNC) is causing the crashes.  This should be fixed by either bug 675221 or bug 679140.  I'll leave this bug open to track the goal which blocks other bigger patches.