The default bug view has changed. See this FAQ.

CMMF_CreateCertRepContentFromDER leaks

VERIFIED FIXED in 3.7.2

Status

NSS
Libraries
P1
normal
VERIFIED FIXED
14 years ago
14 years ago

People

(Reporter: kaie, Assigned: Julien Pierre)

Tracking

unspecified
3.7.2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.75 KB, patch
Wan-Teh Chang
: review+
Nelson Bolyard (seldom reads bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
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

14 years ago
Julien, could you take a look at this bug?  Thanks.
Assignee: wtc → jpierre
Priority: -- → P1
Target Milestone: --- → 3.8
(Assignee)

Comment 2

14 years ago
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.
(Reporter)

Comment 3

14 years ago
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?
(Assignee)

Comment 4

14 years ago
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.
(Assignee)

Comment 5

14 years ago
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
(Reporter)

Comment 6

14 years ago
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

14 years ago
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.
Target Milestone: 3.8 → 3.7.2
(Assignee)

Comment 8

14 years ago
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).
Attachment #112937 - Attachment is obsolete: true

Comment 9

14 years ago
Comment on attachment 113046 [details] [diff] [review]
better patch

r=wtc. Nelson, could you review this patch too?
Attachment #113046 - Flags: superreview?(nelsonb)
Attachment #113046 - Flags: review+
(Assignee)

Comment 10

14 years ago
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.
Attachment #113046 - Flags: superreview?(nelsonb) → superreview+
(Assignee)

Comment 11

14 years ago
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
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 12

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

Comment 13

14 years ago
I confirm the second patch fixes the problem, too.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.