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.
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
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.
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.
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().
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.
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.
Re: comment 6 The definition of PORT_Free is such that it not an error to PORT_Free(NULL);
This was fixed in 3.9 .