399022 - Leak-prone code in nsCertTree

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 327181 comment 134 and bug 327181 comment 135.

I'm not quite sure how this code got review, and more importantly how it got sr.  Then again, as far as I can see it in fact did not get sr.

Comment 1

[Kai Engert (:KaiE:)]

Copying over the comments:

> So is there a reason the nsCertTree code doesn't use alread_AddRefed<> for the
> addrefed pointers it returns?  Then you wouldn't need to sprinkle
> getter_AddRefs all over where it's used....

> I'm also a little confused as to why we're manually addreffing/releasing stuff
> before putting it in mDispInfo.

Comment 2

[Kai Engert (:KaiE:)]

Attachment: Patch v1

(In reply to comment #1)
> So is there a reason the nsCertTree code doesn't use alread_AddRefed<> for the
> addrefed pointers it returns?  Then you wouldn't need to sprinkle
> getter_AddRefs all over where it's used....


The reason is, I didn't think of it :-)
Thanks for your proposal.
I guess you're referring to the two private functions, and I'll change them.

Regarding the interface functions, IIUC we can't use already_AddRefed.


> I'm also a little confused as to why we're manually addreffing/releasing stuff
> before putting it in mDispInfo.

Earlier versions of the patch used an array of raw pointers, I converted it to an array of nsRefPtr late in the game. I agree when using nsRefPtr the manual addref/release is no longer required.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 284028 [details] [diff] [review]
Patch v1

r=bzbarsky.  Thanks for the quick fix!

Comment 4

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Sorry, the previous patch didn't compile.
I had to add one more change:

@@ -868,7 +864,7 @@ NS_IMETHODIMP
 nsCertTree::GetCert(PRUint32 aIndex, nsIX509Cert **_cert)
 {
   NS_ENSURE_ARG(_cert);
-  *_cert = GetCertAtIndex(aIndex);
+  *_cert = GetCertAtIndex(aIndex).get();
   return NS_OK;
 }


This should be ok.
The get() function doesn't do anything, it just returns the raw pointer (which already has its reference counter set).

Comment 5

[Robert Relyea]

I'm not sure I'm the best person to sr code based on the mozilla reference model.

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 284045 [details] [diff] [review]
Patch v2

Boris, do you think we'll need another review?
As this patch is really about addressing your review comments from bug 327181, I'd say it's not necessary.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I don't know what the current review regime in PSM is.  If only one review is needed, I think we'refine.

Comment 8

[Kai Engert (:KaiE:)]

Comment on attachment 284045 [details] [diff] [review]
Patch v2

(In reply to comment #7)
> I don't know what the current review regime in PSM is.  If only one review is
> needed, I think we'refine.

If a module-owner/peer created the patch, review from the module-owner/peer is sufficient.

As this is a correctness fix, not changing any PSM logic, I think your review was really a superreview and is sufficient for check in.

Comment 9

[Kai Engert (:KaiE:)]

checked in, marking fixed.

Comment 10

[Kai Engert (:KaiE:)]

Reopening bug. All patches that got checked in to trunk yesterday are being backed out, because it's unclear which patch has caused a performance regression.

Comment 11

[Kai Engert (:KaiE:)]

checked in again, marking fixed.