Some PSM files mix and match the XPCOM and both the NSPR allocators. nsNSSCallbacks.cpp's HandshakeCallback uses status->mCipherName.Adopt(cipherName); cipherName is allocated by PORT_strdup however Adopt should use NS_Alloc (in fact cipherName is not freed on failure of do_GetService) nsNSSCertificate::GetSerialNumber, GetSha1Fingerprint and GetMd5Fingerprint use nsXPIDLCString fpStr; fpStr.Adopt(CERT_Hexify(&fpItem, 1)); CERT_Hexify's return value is allocated by PORT_Alloc so should be PORT_Free()d nsNSSIOLayer.cpp's nsNSSSocketInfo's hostName propety uses mHostName.Adopt(host ? nsCRT::strdup(host) : 0); *host = (mHostName) ? nsCRT::strdup(mHostName) : nsnull; Both the Adopt and the outparam should use an XPCOM allocator. (You could alternatively change mHostName to a char* and retain that allocator.) nsNSSIOLayer.cpp also has two calls to SSL_RevealURL which return values allocated by PL_strdup so they should be freed with PL_strfree but just to be confusing charCleaner is also (correctly) used with the return value from CERT_GetCommonName. I can't see an easy fix for this one though. nsSSLStatus::GetCipherName uses PL_strdup when it should be an XPCOM allocator.
Created attachment 370595 [details] [diff] [review] Proposed patch The only other alternative I could think of was to revert to manual string deallocation thus eliminating charCleaner entirely.
Assignee: kaie → neil
Status: NEW → ASSIGNED
Attachment #370595 - Flags: review?(honzab.moz)
Comment on attachment 370595 [details] [diff] [review] Proposed patch > /* Get CN and O of the subject and O of the issuer */ > char *ccn = CERT_GetCommonName(&serverCert->subject); >- charCleaner ccnCleaner(ccn); >+ void *v = ccn; >+ voidCleaner ccnCleaner(v); > NS_ConvertUTF8toUTF16 cn(ccn); voidCleaner? http://mxr.mozilla.org/mozilla/search?string=voidCleaner says > No results found
Comment on attachment 370595 [details] [diff] [review] Proposed patch >-NSSCleanupAutoPtrClass(char, PR_FREEIF) >+NSSCleanupAutoPtrClass(char, PL_strfree) >+NSSCleanupAutoPtrClass(void, PR_FREEIF) The name of the cleaner class is created using C++ preprocessor magic. The cleaner class passes a memory pointer to the deallocator. Thus PL_strfree needs to be called from the charCleaner because it's expecting a char* but with PR_FREEIF I can use any suitable type because it doesn't care, it's just most convenient for me to use a void* pointer as I can easily cast to it.
Comment on attachment 370595 [details] [diff] [review] Proposed patch Looks good to me. I did not check any other places of PSM for mismatch.
Attachment #370595 - Flags: review?(honzab.moz) → review+
As far as I know this patch doesn't need any more reviews but I don't have checkin access to the security module so requesting checkin-needed.
> I don't have checkin access to the security module The parts of the security module that are in NSS and NSPR are indeed constrained, so that only peers may checkin. but I was not aware that PSM was under any such checkin restrictions.
Of course. Silly me. I even have eight previous checkins to security/manager.
Created attachment 372209 [details] [diff] [review] Missed two cases I hadn't set up my debugging correctly and a rerun discovered two more files with mismatches, nsCertOverrideService.cpp and nsSDR.cpp; the other changes are the same as the previous patch.
Bah, stupid keyword "autocomplete" :-(
Comment on attachment 372209 [details] [diff] [review] Missed two cases You should use PR_DELETE in nsSDR::DecryptString for freeing 'decoded'. It's allocated with PR_MALLOC in PL_Base64Decode. Same for call to PL_Base64Encode. However, quick look to mxr shows that combination of all nsMemory:Free, PR_Free, NS_Free is used to free result of these functions, so you are probably OK with the patch as is :)
Attachment #372209 - Flags: review?(honzab.moz) → review+
Created attachment 372222 [details] [diff] [review] With PR_DELETE You're right, although at least the PR_DELETE/PR_Free difference is not so much of a concern as the NS_Free/PL_strfree difference.
Attachment #372209 - Attachment is obsolete: true
Fix checked in to CVS.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
This is burning trees on 1.9.0.
This needed approval before checkin. Please back out ASAP.
PSM != NSS PSM != NSS PSM != NSS PSM != NSS PSM != NSS PSM != NSS PSM != NSS PSM != NSS PSM != NSS PSM != NSS PSM != NSS
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed changeset c7ee4c717dd3 to mozilla-central.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.