Closed Bug 486404 Opened 11 years ago Closed 11 years ago
XPCOM allocator mismatches in PSM
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.
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.
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+
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
Closed: 11 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
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 359209
You need to log in before you can comment on or make changes to this bug.