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

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

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

[Wan-Teh Chang]

Attachment: 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

[Wan-Teh Chang]

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?

Comment 3

[Julien Pierre]

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.

Comment 4

[Wan-Teh Chang]

Attachment: Proposed patch v1.1

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

Comment 5

[Wan-Teh Chang]

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).