189976 - CMMF_CreateCertRepContentFromDER leaks

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kai Engert [:KaiE:]]

Call 
  CMMFCertRepContent * c =
    CMMF_CreateCertRepContentFromDER(CERT_GetDefaultCertDB(), data, len);
  if (c)
    CMMF_DestroyCertRepContent(c);

After this sequence, shutting down and reinitializing NSS fails.
Always reproducible.

Comment 1

[Wan-Teh Chang]

Julien, could you take a look at this bug?  Thanks.

Comment 2

[Julien Pierre]

Kai,

It would be useful if you had some sample data to feed to this function that
reproduced the leak, or some programmatic way to generate that data.

I took a look at the code and I have a suspicion of where the leak happens. I
believe it is in crmf/asn1cmf.c, in cmmf_DecodeDERCertificate , in the
CERT_NewTempCertificate call. I don't see where that cert is getting freed.

Comment 3

[Kai Engert [:KaiE:]]

Julien, I changed PSM to dump the argument passed on to
CMMF_CreateCertRepContentFromDER to a file. I have sent you personal mail with
such a file attached, that was created using the procdure explained in bug 189974.

Does this help?

Comment 4

[Julien Pierre]

Kai,

Thanks for the file.
I created a test program that called this function to create the context, and
then immediately destroyed it. I confirmed using Solaris dbx showleaks that
there is a leak. It is in fact occurring in the function I mentioned in comment #2. 
I will create a patch shortly.

Comment 5

[Julien Pierre]

Attachment: Fix memory leak

- Correct erroneous test for isDecoded . Because of this mistake, all the cert
freeing code was dead code
- remove the erroneous freeing of caPubs . This was allocated in an arena
through the ASN.1 decoder and is freed by the PORT_FreeArena call

Comment 6

[Kai Engert [:KaiE:]]

Thanks for the patch, Julien. I just tested it, and it appears to work.

I can confirm, when applying both patches from this and bug 189974, profile
switching does work correctly when using the testcase described in bug 189974.


While I think this is not required for Mozilla 1.3 beta, it would be great if
this fix could be included in the version of NSS that will be supplied for
Mozilla 1.3 final.

Comment 7

[Wan-Teh Chang]

Let's target this for NSS_3_7_BRANCH (3.7.2).
Please get two code reviews.  Thank you for
tracking down this memory leak, Julien.

Comment 8

[Julien Pierre]

Attachment: better patch

The check for isDecoded was actually unnecessary, since the memory is
Zallocated . So we always try to free, even if decoding failed (potentially
partially).

Comment 9

[Wan-Teh Chang]

Comment on attachment 113046 [details] [diff] [review]
better patch

r=wtc. Nelson, could you review this patch too?

Comment 10

[Julien Pierre]

Checked in to NSS 3.8 (tip) .
Checking in respcmn.c;
/cvsroot/mozilla/security/nss/lib/crmf/respcmn.c,v  <--  respcmn.c
new revision: 1.7; previous revision: 1.6
done

Still waiting for Nelson's review and tinderbox results for check-in into 3.7
branch.

Comment 11

[Julien Pierre]

Thanks. Checked in to 3.7 branch as well.

cvs commit: Examining .
Checking in respcmn.c;
/cvsroot/mozilla/security/nss/lib/crmf/respcmn.c,v  <--  respcmn.c
new revision: 1.6.28.1; previous revision: 1.6
done

Comment 12

[Wan-Teh Chang]

Kai, could you verify Julien's new patch (attachment 113046 [details] [diff] [review])?  Thanks.

Comment 13

[Kai Engert [:KaiE:]]

I confirm the second patch fixes the problem, too.