Last Comment Bug 189976 - CMMF_CreateCertRepContentFromDER leaks
: CMMF_CreateCertRepContentFromDER leaks
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: unspecified
: All All
P1 normal (vote)
: 3.7.2
Assigned To: Julien Pierre
: Bishakha Banerjee
Depends on:
Blocks: 189974
  Show dependency treegraph
Reported: 2003-01-21 12:06 PST by Kai Engert (:kaie)
Modified: 2003-01-30 20:03 PST (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Fix memory leak (993 bytes, patch)
2003-01-28 19:00 PST, Julien Pierre
no flags Details | Diff | Splinter Review
better patch (1.75 KB, patch)
2003-01-29 17:40 PST, Julien Pierre
wtc: review+
nelson: superreview+
Details | Diff | Splinter Review

Description User image Kai Engert (:kaie) 2003-01-21 12:06:07 PST
  CMMFCertRepContent * c =
    CMMF_CreateCertRepContentFromDER(CERT_GetDefaultCertDB(), data, len);
  if (c)

After this sequence, shutting down and reinitializing NSS fails.
Always reproducible.
Comment 1 User image Wan-Teh Chang 2003-01-21 17:46:30 PST
Julien, could you take a look at this bug?  Thanks.
Comment 2 User image Julien Pierre 2003-01-21 18:03:34 PST

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 User image Kai Engert (:kaie) 2003-01-27 11:26:46 PST
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 User image Julien Pierre 2003-01-28 17:54:25 PST

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 User image Julien Pierre 2003-01-28 19:00:15 PST
Created attachment 112937 [details] [diff] [review]
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 User image Kai Engert (:kaie) 2003-01-28 19:38:27 PST
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 User image Wan-Teh Chang 2003-01-28 19:51:22 PST
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 User image Julien Pierre 2003-01-29 17:40:15 PST
Created attachment 113046 [details] [diff] [review]
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
Comment 9 User image Wan-Teh Chang 2003-01-29 18:57:37 PST
Comment on attachment 113046 [details] [diff] [review]
better patch

r=wtc. Nelson, could you review this patch too?
Comment 10 User image Julien Pierre 2003-01-29 19:00:20 PST
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

Still waiting for Nelson's review and tinderbox results for check-in into 3.7
Comment 11 User image Julien Pierre 2003-01-30 14:59:43 PST
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:; previous revision: 1.6
Comment 12 User image Wan-Teh Chang 2003-01-30 15:07:22 PST
Kai, could you verify Julien's new patch (attachment 113046 [details] [diff] [review])?  Thanks.
Comment 13 User image Kai Engert (:kaie) 2003-01-30 20:03:00 PST
I confirm the second patch fixes the problem, too.

Note You need to log in before you can comment on or make changes to this bug.