Leaks of the return value of CERT_NameToAscii

RESOLVED FIXED in 3.9

Status

NSS
Tools
P2
normal
RESOLVED FIXED
14 years ago
10 years ago

People

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

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

14 years ago
The return value of CERT_NameToAscii needs to be freed
with a PORT_Free call.

Tej Arora reported that some callers of CERT_NameToAscii
are not freeing the return value and therefore leaking
memory.  Specifically, SECU_PrintName and sv_PrintName.
(Assignee)

Comment 1

14 years ago
Created attachment 135277 [details] [diff] [review]
Partial patch

This patch fixes all the leaks of the return value of
CERT_NameToAscii, except for two files:
- mozilla/security/nss/cmd/crmf-cgi/crmfcgi.c
- mozilla/security/nss/cmd/swfort/instinit/instinit.c
(Assignee)

Comment 2

14 years ago
Comment on attachment 135277 [details] [diff] [review]
Partial patch

Reviewers: please first verify that PORT_Free is the right function
for freeing the return value of CERT_NameToAscii, and then review the
patch.	Thanks.
Attachment #135277 - Flags: superreview?(rrelyea0264)
Attachment #135277 - Flags: review?(jpierre)

Comment 3

14 years ago
Comment on attachment 135277 [details] [diff] [review]
Partial patch

The patch is OK.
PORT_Free is indeed the correct function to use, since the memory returned by
CER_NameToAscii is allocated in AppendStr in alg1485.c which uses PORT_Alloc
and PORT_Realloc.

My only comment is about SECU_PrintName. The use of the freeStr variable to
prevent freeing const memory makes the code a little bit unclean. I would
probably have used PORT_Strdup on the error string to keep the code simpler,
but that's less efficient at runtime.
Attachment #135277 - Flags: review?(jpierre) → review+
(Assignee)

Updated

14 years ago
Attachment #135277 - Flags: superreview?(rrelyea0264)
(Assignee)

Comment 4

14 years ago
Created attachment 135405 [details] [diff] [review]
Partial patch v1.1 (checked in)

Thank you for the code review, Julien, and I agree with your
comment on the use of freeStr in SECU_PrintName.  I made the
following two changes before I checked the patch in.
1. I used a different fix for SECU_PrintName.  Hopefully you
will find this fix more clear.
2. I added a comment to cert.h documenting that the string
returned by CERT_NameToAscii must be freed with PORT_Free().
(Assignee)

Updated

14 years ago
Attachment #135277 - Attachment is obsolete: true
(Assignee)

Comment 5

14 years ago
Comment on attachment 135405 [details] [diff] [review]
Partial patch v1.1 (checked in)

You just need to review my new changes to secutil.c and cert.h.
Attachment #135405 - Flags: review?(jpierre)

Comment 6

14 years ago
Comment on attachment 135405 [details] [diff] [review]
Partial patch v1.1 (checked in)

It seems this new fix in SECU_PrintName may end up calling PORT_Free on a NULL
pointer if nameStr is NULL.
You should check that nameStr is non-NULL before freeing it.
Attachment #135405 - Flags: review?(jpierre) → review-
Re: comment 6
The definition of PORT_Free is such that it not an error to PORT_Free(NULL);
QA Contact: bishakhabanerjee → jason.m.reid
QA Contact: jason.m.reid → tools

Comment 8

10 years ago
This was fixed in 3.9 .
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → 3.9
You need to log in before you can comment on or make changes to this bug.