Bug 1281564 - Fix misuses of free() as the deallocator in PSM. r=keeler
authorCykesiopka <cykesiopka.bmo@gmail.com>
Wed, 22 Jun 2016 15:56:11 -0700
changeset 329675 bd503b1c36f3d07556c305fa1f0e34ed8a6424db
parent 329674 e8609677b8b93d7900cffb29c702123986e9b407
child 329676 a0bc41ba3dc957ceb836ac71ced57ff69af0c594
push idunknown
push userunknown
push dateunknown
reviewerskeeler
bugs1281564
milestone50.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1281564 - Fix misuses of free() as the deallocator in PSM. r=keeler There are a few places in PSM where free() is used to free memory allocated by NSS instead of PORT_Free() (or higher level deallocation functions that end up calling PORT_Free()). In practice, PORT_Free() is just a wrapper around PR_Free(), which is just a wrapper around free() if we don't ask NSPR to use a zone allocator. Gecko explicitly tells NSPR not to use a zone allocator, so the changes here are mainly for making the code more obviously correct. This patch also includes some misc cleanup. MozReview-Commit-ID: 9Ccg5OwlhWR
security/manager/ssl/nsKeygenHandler.cpp
security/manager/ssl/nsNSSCertHelper.cpp
--- a/security/manager/ssl/nsKeygenHandler.cpp
+++ b/security/manager/ssl/nsKeygenHandler.cpp
@@ -454,17 +454,17 @@ nsKeygenFormProcessor::GetPublicKey(cons
                                     const nsAString& aKeyParams)
 {
     nsNSSShutDownPreventionLock locker;
     if (isAlreadyShutDown()) {
       return NS_ERROR_NOT_AVAILABLE;
     }
 
     nsresult rv = NS_ERROR_FAILURE;
-    char *keystring = nullptr;
+    UniquePORTString keystring;
     char *keyparamsString = nullptr;
     uint32_t keyGenMechanism;
     PK11SlotInfo *slot = nullptr;
     PK11RSAGenParams rsaParams;
     SECOidTag algTag;
     int keysize = 0;
     void *params = nullptr;
     SECKEYPrivateKey *privateKey = nullptr;
@@ -664,24 +664,24 @@ nsKeygenFormProcessor::GetPublicKey(cons
                           privateKey, algTag);
     if (srv != SECSuccess) {
         goto loser;
     }
 
     /*
      * Convert the signed public key and challenge into base64/ascii.
      */
-    keystring = BTOA_DataToAscii(signedItem.data, signedItem.len);
+    keystring = UniquePORTString(
+      BTOA_DataToAscii(signedItem.data, signedItem.len));
     if (!keystring) {
         rv = NS_ERROR_OUT_OF_MEMORY;
         goto loser;
     }
 
-    CopyASCIItoUTF16(keystring, aOutPublicKey);
-    free(keystring);
+    CopyASCIItoUTF16(keystring.get(), aOutPublicKey);
 
     rv = NS_OK;
 
     GatherKeygenTelemetry(keyGenMechanism, keysize, keyparamsString);
 loser:
     if (srv != SECSuccess) {
         if ( privateKey ) {
             PK11_DestroyTokenObject(privateKey->pkcs11Slot,privateKey->pkcs11ID);
--- a/security/manager/ssl/nsNSSCertHelper.cpp
+++ b/security/manager/ssl/nsNSSCertHelper.cpp
@@ -2,16 +2,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsNSSCertHelper.h"
 
 #include <algorithm>
 
 #include "mozilla/Casting.h"
+#include "mozilla/NotNull.h"
 #include "mozilla/Snprintf.h"
 #include "mozilla/UniquePtr.h"
 #include "nsCOMPtr.h"
 #include "nsComponentManagerUtils.h"
 #include "nsDateTimeFormatCID.h"
 #include "nsIDateTimeFormat.h"
 #include "nsNSSASN1Object.h"
 #include "nsNSSCertTrust.h"
@@ -64,30 +65,16 @@ static SECOidData more_oids[] = {
     OD( pkixLogotype,
         "Logotype", 
         CKM_INVALID_MECHANISM, INVALID_CERT_EXTENSION ),
 };
 
 static const unsigned int numOids = (sizeof more_oids) / (sizeof more_oids[0]);
 
 static nsresult
-GetIntValue(SECItem *versionItem, 
-            unsigned long *version)
-{
-  SECStatus srv;
-
-  srv = SEC_ASN1DecodeInteger(versionItem,version);
-  if (srv != SECSuccess) {
-    NS_ERROR("Could not decode version of cert");
-    return NS_ERROR_FAILURE;
-  }
-  return NS_OK;
-}
-
-static nsresult
 ProcessVersion(SECItem* versionItem, nsINSSComponent* nssComponent,
                nsIASN1PrintableItem** retItem)
 {
   nsAutoString text;
   nssComponent->GetPIPNSSBundleString("CertDumpVersion", text);
   nsCOMPtr<nsIASN1PrintableItem> printableItem = new nsNSSASN1PrintableItem();
   nsresult rv = printableItem->SetDisplayName(text);
   if (NS_FAILED(rv)) {
@@ -637,71 +624,81 @@ ProcessRawBytes(nsINSSComponent *nssComp
   for (i=0; i<data->len; i++) {
     snprintf_literal(buffer, "%02x ", data->data[i]);
     AppendASCIItoUTF16(buffer, text);
     if ((i+1)%16 == 0) {
       text.AppendLiteral(SEPARATOR);
     }
   }
   return NS_OK;
-}    
+}
+
+/**
+ * Appends a pipnss bundle string to the given string.
+ *
+ * @param nssComponent For accessing the string bundle.
+ * @param bundleKey Key for the string to append.
+ * @param currentText The text to append to, using |SEPARATOR| as the separator.
+ */
+template<size_t N>
+void AppendBundleString(const NotNull<nsINSSComponent*>& nssComponent,
+                        const char (&bundleKey)[N],
+             /*in/out*/ nsAString& currentText)
+{
+  nsAutoString bundleString;
+  nsresult rv = nssComponent->GetPIPNSSBundleString(bundleKey, bundleString);
+  if (NS_FAILED(rv)) {
+    return;
+  }
+
+  currentText.Append(bundleString);
+  currentText.AppendLiteral(SEPARATOR);
+}
 
 static nsresult
 ProcessKeyUsageExtension(SECItem *extData, nsAString &text,
                          nsINSSComponent *nssComponent)
 {
-  nsAutoString local;
-  SECItem decoded;
-  decoded.data = nullptr;
-  decoded.len  = 0;
-  if (SECSuccess != SEC_ASN1DecodeItem(nullptr, &decoded, 
-				SEC_ASN1_GET(SEC_BitStringTemplate), extData)) {
-    nssComponent->GetPIPNSSBundleString("CertDumpExtensionFailure", local);
-    text.Append(local.get());
+  MOZ_ASSERT(extData);
+  MOZ_ASSERT(nssComponent);
+  NS_ENSURE_ARG(extData);
+  NS_ENSURE_ARG(nssComponent);
+
+  NotNull<nsINSSComponent*> wrappedNSSComponent = WrapNotNull(nssComponent);
+  ScopedAutoSECItem decoded;
+  if (SEC_ASN1DecodeItem(nullptr, &decoded, SEC_ASN1_GET(SEC_BitStringTemplate),
+                         extData) != SECSuccess) {
+    AppendBundleString(wrappedNSSComponent, "CertDumpExtensionFailure", text);
     return NS_OK;
   }
   unsigned char keyUsage = 0;
   if (decoded.len) {
     keyUsage = decoded.data[0];
   }
-  free(decoded.data);
+
   if (keyUsage & KU_DIGITAL_SIGNATURE) {
-    nssComponent->GetPIPNSSBundleString("CertDumpKUSign", local);
-    text.Append(local.get());
-    text.AppendLiteral(SEPARATOR);
+    AppendBundleString(wrappedNSSComponent, "CertDumpKUSign", text);
   }
   if (keyUsage & KU_NON_REPUDIATION) {
-    nssComponent->GetPIPNSSBundleString("CertDumpKUNonRep", local);
-    text.Append(local.get());
-    text.AppendLiteral(SEPARATOR);
+    AppendBundleString(wrappedNSSComponent, "CertDumpKUNonRep", text);
   }
   if (keyUsage & KU_KEY_ENCIPHERMENT) {
-    nssComponent->GetPIPNSSBundleString("CertDumpKUEnc", local);
-    text.Append(local.get());
-    text.AppendLiteral(SEPARATOR);
+    AppendBundleString(wrappedNSSComponent, "CertDumpKUEnc", text);
   }
   if (keyUsage & KU_DATA_ENCIPHERMENT) {
-    nssComponent->GetPIPNSSBundleString("CertDumpKUDEnc", local);
-    text.Append(local.get());
-    text.AppendLiteral(SEPARATOR);
+    AppendBundleString(wrappedNSSComponent, "CertDumpKUDEnc", text);
   }
   if (keyUsage & KU_KEY_AGREEMENT) {
-    nssComponent->GetPIPNSSBundleString("CertDumpKUKA", local);
-    text.Append(local.get());
-    text.AppendLiteral(SEPARATOR);
+    AppendBundleString(wrappedNSSComponent, "CertDumpKUKA", text);
   }
   if (keyUsage & KU_KEY_CERT_SIGN) {
-    nssComponent->GetPIPNSSBundleString("CertDumpKUCertSign", local);
-    text.Append(local.get());
-    text.AppendLiteral(SEPARATOR);
+    AppendBundleString(wrappedNSSComponent, "CertDumpKUCertSign", text);
   }
   if (keyUsage & KU_CRL_SIGN) {
-    nssComponent->GetPIPNSSBundleString("CertDumpKUCRLSigner", local);
-    text.Append(local.get());
-    text.AppendLiteral(SEPARATOR);
+    AppendBundleString(wrappedNSSComponent, "CertDumpKUCRLSigner", text);
   }
 
   return NS_OK;
 }
 
 static nsresult
 ProcessBasicConstraints(SECItem  *extData, 
                         nsAString &text,
@@ -874,29 +871,26 @@ ProcessName(CERTName *name, nsINSSCompon
     if (NS_FAILED(rv))
       return rv;
   }
   *value = ToNewUnicode(finalString);    
   return NS_OK;
 }
 
 static nsresult
-ProcessIA5String(SECItem  *extData, 
-		 nsAString &text,
-		 nsINSSComponent *nssComponent)
+ProcessIA5String(const SECItem& extData, /*in/out*/ nsAString& text)
 {
-  SECItem item;
-  nsAutoString local;
-  if (SECSuccess != SEC_ASN1DecodeItem(nullptr, &item, 
-				       SEC_ASN1_GET(SEC_IA5StringTemplate),
-				       extData))
+  ScopedAutoSECItem item;
+  if (SEC_ASN1DecodeItem(nullptr, &item, SEC_ASN1_GET(SEC_IA5StringTemplate),
+                         &extData) != SECSuccess) {
     return NS_ERROR_FAILURE;
-  local.AssignASCII((char*)item.data, item.len);
-  free(item.data);
-  text.Append(local);
+  }
+
+  text.AppendASCII(BitwiseCast<char*, unsigned char*>(item.data),
+                   AssertedCast<uint32_t>(item.len));
   return NS_OK;
 }
 
 static nsresult
 AppendBMPtoUTF16(const UniquePLArenaPool& arena, unsigned char* data,
                  unsigned int len, nsAString& text)
 {
   if (len % 2 != 0) {
@@ -1288,18 +1282,17 @@ ProcessCertificatePolicies(SECItem  *ext
 	case SEC_OID_PKIX_CPS_POINTER_QUALIFIER:
 	  nssComponent->GetPIPNSSBundleString("CertDumpCPSPointer", local);
 	  text.Append(local);
 	  text.Append(':');
 	  text.AppendLiteral(SEPARATOR);
 	  text.AppendLiteral("    ");
 	  /* The CPS pointer ought to be the cPSuri alternative
 	     of the Qualifier choice. */
-	  rv = ProcessIA5String(&policyQualifier->qualifierValue,
-				text, nssComponent);
+          rv = ProcessIA5String(policyQualifier->qualifierValue, text);
           if (NS_FAILED(rv)) {
             return rv;
           }
 	  break;
 	case SEC_OID_PKIX_USER_NOTICE_QUALIFIER:
 	  nssComponent->GetPIPNSSBundleString("CertDumpUserNotice", local);
 	  text.Append(local);
 	  text.AppendLiteral(": ");
@@ -1465,37 +1458,40 @@ ProcessAuthInfoAccess(SECItem  *extData,
   return rv;
 }
 
 static nsresult
 ProcessMSCAVersion(SECItem  *extData, 
 		   nsAString &text,
 		   nsINSSComponent *nssComponent)
 {
-  unsigned long version;
-  nsresult rv;
-  char buf[50];
-  SECItem decoded;
+  MOZ_ASSERT(extData);
+  NS_ENSURE_ARG(extData);
 
-  if (SECSuccess != SEC_ASN1DecodeItem(nullptr, &decoded, 
-				       SEC_ASN1_GET(SEC_IntegerTemplate), 
-				       extData))
+  ScopedAutoSECItem decoded;
+  if (SEC_ASN1DecodeItem(nullptr, &decoded, SEC_ASN1_GET(SEC_IntegerTemplate),
+                         extData) != SECSuccess) {
     /* This extension used to be an Integer when this code
        was written, but apparently isn't anymore. Display
        the raw bytes instead. */
     return ProcessRawBytes(nssComponent, extData, text);
+  }
 
-  rv = GetIntValue(&decoded, &version);
-  free(decoded.data);
-  if (NS_FAILED(rv))
+  unsigned long version;
+  if (SEC_ASN1DecodeInteger(&decoded, &version) != SECSuccess) {
     /* Value out of range, display raw bytes */
     return ProcessRawBytes(nssComponent, extData, text);
+  }
 
   /* Apparently, the encoding is <minor><major>, with 16 bits each */
-  snprintf_literal(buf, "%d.%d", version & 0xFFFF, version >> 16);
+  char buf[50];
+  if (snprintf_literal(buf, "%d.%d", version & 0xFFFF, version >> 16) <= 0) {
+    return NS_ERROR_FAILURE;
+  }
+
   text.AppendASCII(buf);
   return NS_OK;
 }
 
 static nsresult
 ProcessExtensionData(SECOidTag oidTag, SECItem *extData, 
                      nsAString &text, 
                      SECOidTag ev_oid_tag, // SEC_OID_UNKNOWN means: not EV