Last Comment Bug 189976 - CMMF_CreateCertRepContentFromDER leaks
: CMMF_CreateCertRepContentFromDER leaks
Status: VERIFIED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: 3.7.2
Assigned To: Julien Pierre
: Bishakha Banerjee
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
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 Kai Engert (:kaie) 2003-01-21 12:06:07 PST
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 2003-01-21 17:46:30 PST
Julien, could you take a look at this bug?  Thanks.
Comment 2 Julien Pierre 2003-01-21 18:03:34 PST
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) 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 Julien Pierre 2003-01-28 17:54:25 PST
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 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 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 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 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
partially).
Comment 9 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 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
done

Still waiting for Nelson's review and tinderbox results for check-in into 3.7
branch.
Comment 11 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: 1.6.28.1; previous revision: 1.6
done
Comment 12 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 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.