157927 - Fix memory leaks in nsNSSCertificate.cpp

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Kai Engert [:KaiE:]]

nsNSSCertificate.cpp seems to leak.

I see calls to CERT_GetCommonName or CERT_GetOrgName, and there are probably
many others, where the return code has been allocated by NSS, but PSM fails to
free the memory.

Comment 1

[Kai Engert [:KaiE:]]

I think I have fixed most of the leaks with the patch for bug 121916.

Comment 2

[Kai Engert [:KaiE:]]

Wrong bug number in previous statement. Correction:

I think I have fixed most of the leaks with the patch for bug 124037.

Comment 3

[Kai Engert [:KaiE:]]

I looked again, and I actually found more leaks.
In addition, I want to get rid of the compiler warnings PSM produces.
- signed mismatch on comparison
- cleaned up the code in nsNSSCallbacks.cpp to avoid a leak on error
- temporarily #if 0 out more code, sigh. The disabled code was reported as being
defined but unused. There is some code in that file that say "XXX should get
reenabled", but it does not say why. I don't want to remove that code completely
before I have a chance to understand it.
- change a nsXPIDLCString to a nsCString to get rid of an evil looking
"synthesized class for assignment" compiler warning

Comment 4

[Kai Engert [:KaiE:]]

Attachment: Patch v1 ignoring whitespace, please review this one

Comment 5

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 6

[Kai Engert [:KaiE:]]

Javi, can you please review?

Comment 7

[Javier Delgadillo]

Comment on attachment 94215 [details] [diff] [review]
Patch v1 ignoring whitespace, please review this one

r=javi

Is that one chunk in nsNSSIOLayer.cpp that you commented out really not called
from anywhere?

Comment 8

[Kai Engert [:KaiE:]]

No, that block is not called from anywhere. The single entry point to all the
#ifdef'ed functions is CERT_MatchesScopeOfUse. There are two calls to that
function that are already disabled and marked with "XXX fixme".

Comment 9

[Kai Engert [:KaiE:]]

Alec, can you please review?

Comment 10

[jag (Peter Annema)]

Comment on attachment 94215 [details] [diff] [review]
Patch v1 ignoring whitespace, please review this one

>Index: mozilla/security/manager/ssl/src/nsKeygenHandler.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsKeygenHandler.cpp,v
>retrieving revision 1.15
>diff -u -d -w -b -B -r1.15 nsKeygenHandler.cpp
>--- mozilla/security/manager/ssl/src/nsKeygenHandler.cpp	15 May 2002 18:54:55 -0000	1.15
>+++ mozilla/security/manager/ssl/src/nsKeygenHandler.cpp	6 Aug 2002 19:12:20 -0000
>@@ -623,7 +623,6 @@
> 						nsString& aAttribute) 
> { 
>   nsString selectKey;
>-  SECKeySizeChoiceInfo *choice = SECKeySizeChoiceList;
> 
>   selectKey.Assign(NS_LITERAL_STRING("SELECT"));
>   if (Compare(aFormType, NS_LITERAL_STRING("SELECT"), 

It looks like selectKey is unused and can be removed.

You could also change 

  aAttribute.AssignWithConversion(mozKeyGen);

to

  aAttribute.Assign(NS_LITERAL_STRING("-mozilla-keygen"));

sr=jag with those change (and without, but since you seem to be doing some
generic clean-up here anyway)

Comment 11

[Kai Engert [:KaiE:]]

Attachment: Patch v2

This patch includes Jag's suggestions.

Comment 12

[Kai Engert [:KaiE:]]

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

carrying forward reviews

Comment 13

[Kai Engert [:KaiE:]]

fixed on trunk

Comment 14

[John Unruh]

Verified per kaie's comment.