587409 - pk11ListCertCallback leaks newCert when CERT_AddCertToListTailWithData fails

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[timeless]

2266 	pk11ListCertCallback(NSSCertificate *c, void *arg)
2269 	    CERTCertificate *newCert = NULL;

2303 	    newCert = STAN_GetCERTCertificate(c);

2314 		nickname = STAN_GetCERTCertificateName(certList->arena, c);

2317 		if (newCert->slot && !PK11_IsInternal(newCert->slot)) {
2318 		    CERT_AddCertToListTailWithData(certList,newCert,nickname);
2319 		} else {
2320 		    CERT_AddCertToListHeadWithData(certList,newCert,nickname);

2666 	CERT_AddCertToListTailWithData(CERTCertList *certs, 

2671 	    node = (CERTCertListNode *)PORT_ArenaZAlloc(certs->arena,
2672 							sizeof(CERTCertListNode));
2673 	    if ( node == NULL ) {
2674 		goto loser;

2683 	loser:
2684 	    return(SECFailure);

Comment 1

[timeless]

Attachment: patch

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 466130 [details] [diff] [review]
patch

Review requests for patches to pk11wrap and softoken should go to rrelyea

Comment 3

[Robert Relyea]

Comment on attachment 466130 [details] [diff] [review]
patch

r+ provided the same code is added to the second half of the if (unique) as well.

The leak detection tools is pretty good here. I knows that newCert is reference counted, that CERT_AddCertToListXXX will adopt the reference if it succeeds, but
not if it fails. I doubt this was a run-time detection as CERT_AddCertToListXXX
will only fail if an out of memory situation (at that point worse things usually
happen to the system).

It's interesting it only noted on half of the if.

Comment 4

[Robert Relyea]

taking

Comment 5

[Robert Relyea]

Checking in pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.175; previous revision: 1.174
done

Comment 6

[timeless]

yeah, this is coverity, so it's static. since i'm trying not to reveal what coverity is doing (i'm not sure what the license terms are for the instance i'm using), i tend to collapse things (coverity also requires you to click through to follow function behavior), it's also possible that the other paths were reported in a different "issue" which i hadn't yet reviewed.

i'm not actually certain how smart it was about ref counting. from memory it tends to get things like that mostly wrong and i get to spend time trying to figure out if it got them right or not. generally it just says "1. i see an alloc here. 2. i found a path where i could get it to leak", the rest is my job.