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

RESOLVED FIXED in 3.9

Status

NSS
Libraries
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [3.8.2])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

15 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.
(Assignee)

Comment 1

15 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).
(Assignee)

Comment 2

15 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

15 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+
(Assignee)

Comment 4

15 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.
(Assignee)

Updated

15 years ago
Attachment #131211 - Attachment is obsolete: true
(Assignee)

Comment 5

15 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).
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Whiteboard: [3.8.2]
Target Milestone: --- → 3.9

Updated

15 years ago
Attachment #131211 - Flags: superreview?(bugz)
You need to log in before you can comment on or make changes to this bug.