Bug 1287290 - Use ScopedAutoSECItem in PSM more. draft
authorCykesiopka <cykesiopka.bmo@gmail.com>
Mon, 25 Jul 2016 15:06:34 +0800
changeset 392259 106c2743123e088571061afcfbdef9a3990a4ea1
parent 392258 2e6d8283458c202a196842653838ed83384e3cbf
child 526293 e25d5da3dfb26d3494f961a3e4c6bd1e97e7bfb5
push id23983
push usercykesiopka.bmo@gmail.com
push dateMon, 25 Jul 2016 09:04:34 +0000
bugs1287290
milestone50.0a1
Bug 1287290 - Use ScopedAutoSECItem in PSM more. ScopedAutoSECItem is useful for: 1. Removing manual memory management. 2. Getting rid of this pattern: > UniqueSECItem item(SECITEM_AllocItem(nullptr, nullptr, 0)); While this pattern works, ScopedAutoSECItem is slightly superior in that it doesn't unnecessarily cause a SECItem to be allocated from the heap. MozReview-Commit-ID: 8DPD9gtzeru
security/manager/ssl/ContentSignatureVerifier.cpp
security/manager/ssl/nsNSSU2FToken.cpp
security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.cpp
--- a/security/manager/ssl/ContentSignatureVerifier.cpp
+++ b/security/manager/ssl/ContentSignatureVerifier.cpp
@@ -7,16 +7,17 @@
 #include "ContentSignatureVerifier.h"
 
 #include "BRNameMatchingPolicy.h"
 #include "SharedCertVerifier.h"
 #include "cryptohi.h"
 #include "keyhi.h"
 #include "mozilla/Assertions.h"
 #include "mozilla/Casting.h"
+#include "mozilla/unused.h"
 #include "nsCOMPtr.h"
 #include "nsContentUtils.h"
 #include "nsISupportsPriority.h"
 #include "nsIURI.h"
 #include "nsNSSComponent.h"
 #include "nsSecurityHeaderParser.h"
 #include "nsStreamUtils.h"
 #include "nsWhitespaceTokenizer.h"
@@ -94,35 +95,34 @@ ReadChainIntoCertList(const nsACString& 
     if (token.IsEmpty()) {
       continue;
     }
     if (inBlock) {
       if (token.Equals(footer)) {
         inBlock = false;
         certFound = true;
         // base64 decode data, make certs, append to chain
-        UniqueSECItem der(::SECITEM_AllocItem(nullptr, nullptr, 0));
-        if (!der || !NSSBase64_DecodeBuffer(nullptr, der.get(),
-                                            blockData.BeginReading(),
-                                            blockData.Length())) {
+        ScopedAutoSECItem der;
+        if (!NSSBase64_DecodeBuffer(nullptr, &der, blockData.BeginReading(),
+                                    blockData.Length())) {
           CSVerifier_LOG(("CSVerifier: decoding the signature failed\n"));
           return NS_ERROR_FAILURE;
         }
-        CERTCertificate* tmpCert =
-          CERT_NewTempCertificate(CERT_GetDefaultCertDB(), der.get(), nullptr,
-                                  false, true);
+        UniqueCERTCertificate tmpCert(
+          CERT_NewTempCertificate(CERT_GetDefaultCertDB(), &der, nullptr, false,
+                                  true));
         if (!tmpCert) {
           return NS_ERROR_FAILURE;
         }
         // if adding tmpCert succeeds, tmpCert will now be owned by aCertList
-        SECStatus res = CERT_AddCertToListTail(aCertList, tmpCert);
+        SECStatus res = CERT_AddCertToListTail(aCertList, tmpCert.get());
         if (res != SECSuccess) {
-          CERT_DestroyCertificate(tmpCert);
           return MapSECStatus(res);
         }
+        Unused << tmpCert.release();
       } else {
         blockData.Append(token);
       }
     } else if (token.Equals(header)) {
       inBlock = true;
       blockData = "";
     }
   }
@@ -209,48 +209,43 @@ ContentSignatureVerifier::CreateContextI
 
   // in case we were not able to extract a key
   if (!mKey) {
     CSVerifier_LOG(("CSVerifier: unable to extract a key\n"));
     return NS_ERROR_INVALID_SIGNATURE;
   }
 
   // Base 64 decode the signature
-  UniqueSECItem rawSignatureItem(::SECITEM_AllocItem(nullptr, nullptr, 0));
-  if (!rawSignatureItem ||
-      !NSSBase64_DecodeBuffer(nullptr, rawSignatureItem.get(),
-                              mSignature.get(), mSignature.Length())) {
+  ScopedAutoSECItem rawSignatureItem;
+  if (!NSSBase64_DecodeBuffer(nullptr, &rawSignatureItem, mSignature.get(),
+                              mSignature.Length())) {
     CSVerifier_LOG(("CSVerifier: decoding the signature failed\n"));
     return NS_ERROR_FAILURE;
   }
 
   // get signature object
-  UniqueSECItem signatureItem(::SECITEM_AllocItem(nullptr, nullptr, 0));
-  if (!signatureItem) {
-    return NS_ERROR_FAILURE;
-  }
+  ScopedAutoSECItem signatureItem;
   // We have a raw ecdsa signature r||s so we have to DER-encode it first
   // Note that we have to check rawSignatureItem->len % 2 here as
   // DSAU_EncodeDerSigWithLen asserts this
-  if (rawSignatureItem->len == 0 || rawSignatureItem->len % 2 != 0) {
+  if (rawSignatureItem.len == 0 || rawSignatureItem.len % 2 != 0) {
     CSVerifier_LOG(("CSVerifier: signature length is bad\n"));
     return NS_ERROR_FAILURE;
   }
-  if (DSAU_EncodeDerSigWithLen(signatureItem.get(), rawSignatureItem.get(),
-                               rawSignatureItem->len) != SECSuccess) {
+  if (DSAU_EncodeDerSigWithLen(&signatureItem, &rawSignatureItem,
+                               rawSignatureItem.len) != SECSuccess) {
     CSVerifier_LOG(("CSVerifier: encoding the signature failed\n"));
     return NS_ERROR_FAILURE;
   }
 
   // this is the only OID we support for now
   SECOidTag oid = SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE;
 
   mCx = UniqueVFYContext(
-    VFY_CreateContext(mKey.get(), signatureItem.get(), oid, nullptr));
-
+    VFY_CreateContext(mKey.get(), &signatureItem, oid, nullptr));
   if (!mCx) {
     return NS_ERROR_INVALID_SIGNATURE;
   }
 
   if (VFY_Begin(mCx.get()) != SECSuccess) {
     return NS_ERROR_INVALID_SIGNATURE;
   }
 
--- a/security/manager/ssl/nsNSSU2FToken.cpp
+++ b/security/manager/ssl/nsNSSU2FToken.cpp
@@ -598,44 +598,39 @@ nsNSSU2FToken::Register(uint8_t* aApplic
   // It's OK to ignore the return values here because we're writing into
   // pre-allocated space
   signedDataBuf.AppendElement(0x00, mozilla::fallible);
   signedDataBuf.AppendElements(aApplication, aApplicationLen, mozilla::fallible);
   signedDataBuf.AppendElements(aChallenge, aChallengeLen, mozilla::fallible);
   signedDataBuf.AppendSECItem(keyHandleItem.get());
   signedDataBuf.AppendSECItem(pubKey->u.ec.publicValue);
 
-  UniqueSECItem signatureItem(::SECITEM_AllocItem(/* default arena */ nullptr,
-                                                  /* no buffer */ nullptr, 0));
-  if (!signatureItem) {
-    return NS_ERROR_OUT_OF_MEMORY;
-  }
-
-  SECStatus srv = SEC_SignData(signatureItem.get(), signedDataBuf.Elements(),
+  ScopedAutoSECItem signatureItem;
+  SECStatus srv = SEC_SignData(&signatureItem, signedDataBuf.Elements(),
                                signedDataBuf.Length(), attestPrivKey.get(),
                                SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE);
   if (srv != SECSuccess) {
     MOZ_LOG(gNSSTokenLog, LogLevel::Warning,
             ("Signature failure: %d", PORT_GetError()));
     return NS_ERROR_FAILURE;
   }
 
   // Serialize the registration data
   mozilla::dom::CryptoBuffer registrationBuf;
   if (!registrationBuf.SetCapacity(1 + kPublicKeyLen + 1 + keyHandleItem->len +
                                    attestCert.get()->derCert.len +
-                                   signatureItem->len, mozilla::fallible)) {
+                                   signatureItem.len, mozilla::fallible)) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
   registrationBuf.AppendElement(0x05, mozilla::fallible);
   registrationBuf.AppendSECItem(pubKey->u.ec.publicValue);
   registrationBuf.AppendElement(keyHandleItem->len, mozilla::fallible);
   registrationBuf.AppendSECItem(keyHandleItem.get());
   registrationBuf.AppendSECItem(attestCert.get()->derCert);
-  registrationBuf.AppendSECItem(signatureItem.get());
+  registrationBuf.AppendSECItem(signatureItem);
   if (!registrationBuf.ToNewUnsignedBuffer(aRegistration, aRegistrationLen)) {
     return NS_ERROR_FAILURE;
   }
 
   return NS_OK;
 }
 
 // A U2F Sign operation creates a signature over the "param" arguments (plus
@@ -722,41 +717,36 @@ nsNSSU2FToken::Sign(uint8_t* aApplicatio
 
   // It's OK to ignore the return values here because we're writing into
   // pre-allocated space
   signedDataBuf.AppendElements(aApplication, aApplicationLen, mozilla::fallible);
   signedDataBuf.AppendElement(0x01, mozilla::fallible);
   signedDataBuf.AppendSECItem(counterItem);
   signedDataBuf.AppendElements(aChallenge, aChallengeLen, mozilla::fallible);
 
-  UniqueSECItem signatureItem(::SECITEM_AllocItem(/* default arena */ nullptr,
-                                                  /* no buffer */ nullptr, 0));
-  if (!signatureItem) {
-    return NS_ERROR_OUT_OF_MEMORY;
-  }
-
-  SECStatus srv = SEC_SignData(signatureItem.get(), signedDataBuf.Elements(),
+  ScopedAutoSECItem signatureItem;
+  SECStatus srv = SEC_SignData(&signatureItem, signedDataBuf.Elements(),
                                signedDataBuf.Length(), privKey.get(),
                                SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE);
   if (srv != SECSuccess) {
     MOZ_LOG(gNSSTokenLog, LogLevel::Warning,
             ("Signature failure: %d", PORT_GetError()));
     return NS_ERROR_FAILURE;
   }
 
   // Assemble the signature data into a buffer for return
   mozilla::dom::CryptoBuffer signatureBuf;
-  if (!signatureBuf.SetCapacity(1 + counterItem.len + signatureItem->len,
+  if (!signatureBuf.SetCapacity(1 + counterItem.len + signatureItem.len,
                                 mozilla::fallible)) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
   // It's OK to ignore the return values here because we're writing into
   // pre-allocated space
   signatureBuf.AppendElement(0x01, mozilla::fallible);
   signatureBuf.AppendSECItem(counterItem);
-  signatureBuf.AppendSECItem(signatureItem.get());
+  signatureBuf.AppendSECItem(signatureItem);
 
   if (!signatureBuf.ToNewUnsignedBuffer(aSignature, aSignatureLen)) {
     return NS_ERROR_FAILURE;
   }
   return NS_OK;
 }
--- a/security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.cpp
+++ b/security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.cpp
@@ -178,26 +178,25 @@ DecodeCertCallback(void* arg, SECItem** 
 SECStatus
 AddCertificateFromFile(const char* basePath, const char* filename)
 {
   char buf[16384] = { 0 };
   SECStatus rv = ReadFileToBuffer(basePath, filename, buf);
   if (rv != SECSuccess) {
     return rv;
   }
-  SECItem certDER;
+  ScopedAutoSECItem certDER;
   rv = CERT_DecodeCertPackage(buf, strlen(buf), DecodeCertCallback, &certDER);
   if (rv != SECSuccess) {
     PrintPRError("CERT_DecodeCertPackage failed");
     return rv;
   }
   UniqueCERTCertificate cert(CERT_NewTempCertificate(CERT_GetDefaultCertDB(),
                                                      &certDER, nullptr, false,
                                                      true));
-  PORT_Free(certDER.data);
   if (!cert) {
     PrintPRError("CERT_NewTempCertificate failed");
     return SECFailure;
   }
   UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
   if (!slot) {
     PrintPRError("PK11_GetInternalKeySlot failed");
     return SECFailure;