Closed Bug 674571 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 675221
mozilla8

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(2 files)

Attached patch fixSplinter Review
Spun off from bug 667535 to handle just the security/manager/ssl case.
Attachment #548817 - Flags: review?
Oops, wrong component.
Component: JavaScript Engine → Networking
QA Contact: general → networking
Component: Networking → Security: PSM
QA Contact: networking → psm
Any ETA for this review?
Comment on attachment 548817 [details] [diff] [review]
fix

Embarrassingly, there is in fact, no assigned reviewer (b/c bugzilla silently ignores r? typos).
Attachment #548817 - Flags: review? → review?(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).
Attachment #548817 - Flags: review?(kaie) → review-
r=kaie if you address my requests, but feel free to rerequest a final review of your new patch. I will rereview quickly.
(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.
Attached patch fix v2Splinter Review
Attachment #552119 - Flags: review?
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
Attachment #552119 - Flags: review? → review+
Thanks!  I agree that it can certainly be readable for many cases.

http://hg.mozilla.org/integration/mozilla-inbound/rev/0cf822d12c64
Whiteboard: [inbound]
Merged:
http://hg.mozilla.org/mozilla-central/rev/0cf822d12c64
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Version: unspecified → Trunk
No longer depends on: 678547
Blocks: 678706
No longer blocks: 678706
Depends on: 678706, 678710
No longer depends on: 678706
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [inbound]
Whiteboard: [inbound]
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 675221
You need to log in before you can comment on or make changes to this bug.