If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

XPCOM allocator mismatches in PSM

RESOLVED FIXED

Status

()

Core
Security: PSM
RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
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
(Assignee)

Comment 3

9 years ago
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+
(Assignee)

Comment 5

9 years ago
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.
Keywords: 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.
(Assignee)

Comment 7

9 years ago
Of course. Silly me. I even have eight previous checkins to security/manager.
(Assignee)

Comment 8

9 years ago
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.
Attachment #370595 - Attachment is obsolete: true
Attachment #372209 - Flags: review?(honzab.moz)
(Assignee)

Updated

9 years ago
Keywords: checkin-needed → 4xp
(Assignee)

Comment 9

9 years ago
Bah, stupid keyword "autocomplete" :-(
Keywords: 4xp
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+
(Assignee)

Comment 11

9 years ago
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
(Assignee)

Comment 12

9 years ago
Fix checked in to CVS.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
This is burning trees on 1.9.0.
This needed approval before checkin. Please back out ASAP.
(Assignee)

Comment 15

9 years ago
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 → ---
(Assignee)

Comment 16

9 years ago
Pushed changeset c7ee4c717dd3 to mozilla-central.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 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.