CERT_DestroyCertList does not free the appData fields of the cert list nodes returned by PK11_ListCerts



16 years ago
16 years ago


(Reporter: wtc, Assigned: wtc)


Firefox Tracking Flags

(Not tracked)


(Whiteboard: [3.8.2])


(1 attachment, 1 obsolete attachment)



16 years ago
PK11_ListCerts calls STAN_GetCERTCertificateName to
allocate the appData fields in the cert list nodes
it returns.  STAN_GetCERTCertificateName returns a
nickname whose memory is allocated with PORT_Alloc.
So we need to free the cert list nodes' appData
with PORT_Free.

CERT_DestroyCertList isn't doing that.  This is a
memory leak.

Three possible solutions.

1. Document that the caller of PK11_ListCerts is
responsible for freeing the cert list nodes' appData
with PORT_Free before calling CERT_DestroyCertList.

2. Modify STAN_GetCERTCertificateName so that it
uses the cert list's arena to allocate the nickname.

3. Add an appDataDestructor function pointer to
the CERTCertListStr structure, and modify
CERT_DestroyCertList to invoke appDataDestructor
on the cert list nodes' appData.

Comment 1

16 years ago
Created attachment 131211 [details] [diff] [review]
Proposed patch

This patch implements solution 2: allocate the nicknames
from the cert list's arena.  It improves upon Julien's work.

Note that for clarity this patch is against the tip of NSS
*before* Julien's patch (rev. 1.119 of pk11cert.c).

Comment 2

16 years ago
Comment on attachment 131211 [details] [diff] [review]
Proposed patch

Julien, Ian, please review this patch.

Questions for Ian:
1. Is it okay for STAN_GetCERTCertificateName to take
PRArenaPool (a non-Stan data type) as an argument?
I think it is okay because STAN_GetCERTCertificateName
is not a Stan function.

2. Is 'arenaOpt' your naming convention for an arena
argument that may be NULL, which means allocating from
the heap instead?
Attachment #131211 - Flags: superreview?(bugz)
Attachment #131211 - Flags: review?(jpierre)

Comment 3

16 years ago
Comment on attachment 131211 [details] [diff] [review]
Proposed patch

This looks fine. However, you'll need to undo my other patch (which frees from
the global heap and reallocates into the arena) before you can apply this to
the tip and check it in.
Attachment #131211 - Flags: review?(jpierre) → review+

Comment 4

16 years ago
Created attachment 131355 [details] [diff] [review]
Proposed patch v1.1

The only change in this patch is that I use the more correct
name PLArenaPool rather than PRArenaPool.


16 years ago
Attachment #131211 - Attachment is obsolete: true

Comment 5

16 years ago
Thanks for the review, Julien.  Yes, one needs to
undo your other patch before applying my patch.

I've checked in the patch on the NSS trunk (NSS
3.9) and NSS_3_8_BRANCH (NSS 3.8.2).
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: [3.8.2]
Target Milestone: --- → 3.9
Attachment #131211 - Flags: superreview?(bugz)
You need to log in before you can comment on or make changes to this bug.