139325 - Leaking CERTCertificate references from nsNSSCertificate::GetChain

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Brian Ryner (not reading)]

The following code leaks:

  while (cert) {
    nsCOMPtr<nsIX509Cert> pipCert = new nsNSSCertificate(cert);
    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("adding %s to chain\n", cert->nickname));
    array->AppendElement(pipCert);
    if (SECITEM_CompareItem(&cert->derIssuer, &cert->derSubject) == SECEqual)
      break;
    cert = CERT_FindCertIssuer(cert, PR_Now(), certUsageSSLClient);
  }


because CERT_FindCertIssuer will addref the certificate before handing it back.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Suggested fix

Comment 2

[Kai Engert [:KaiE:]]

Javi, can you please review?

Comment 3

[Kai Engert [:KaiE:]]

Attachment: Better fix

Shame on me, the previous patch introduced a double free for the first cert.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: Fixed goto init crossing

The previous patch didn't compile everywhere, because the existing goto crossed
the init location.

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 80491 [details] [diff] [review]
Fixed goto init crossing

|cert| doesn't strictly need to be initialized to nsnull; remove it if you want
to.  r=bryner.

Comment 6

[Brian Ryner (not reading)]

With this patch applied, and the patch in bug 133584 applied, NSS shuts down
cleanly (verified by setting NSS_STRICT_SHUTDOWN) after bringing up the cert
manager and viewing a certificate.

Comment 7

[Kai Engert [:KaiE:]]

Brian, you also had the fix from 139349 in your tree, right?

Alec, can you please sr= ?

Comment 8

[Javier Delgadillo]

Comment on attachment 80491 [details] [diff] [review]
Fixed goto init crossing

r=javi

Comment 9

[Brian Ryner (not reading)]

kaie, yes, I have that fix in my tree as well.

Comment 10

[Alec Flett]

Comment on attachment 80491 [details] [diff] [review]
Fixed goto init crossing

sr=alecf

Comment 11

[Stephane Saux]

Update for Scott Putterman.
This bug is indeed related to the profile switching work that is taking place.
See comment #6
Note also that this bug blocks bug 125561
If the strategy is to try and get NSS to shutdown cleanly always, or at least
most of the time then this bug is needed.

Comment 12

[Kai Engert [:KaiE:]]

Checked in to trunk.

Comment 13

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin on the 1.0 branch. Pls check
this in today, then add the fixed1.0.0 keyword.

Comment 14

[Charles Rosendahl]

Verified - through 125561 on Win and Linux

Comment 15

[Scott Collins]

Comment on attachment 80491 [details] [diff] [review]
Fixed goto init crossing

a=scc for checkin to the mozilla 1.0 branch

Comment 16

[Kai Engert [:KaiE:]]

Checked in to branch

Comment 17

[John Unruh]

verified1.0.0