Bug 1287290 - Use ScopedAutoSECItem in PSM more. r=dkeeler
authorCykesiopka <cykesiopka.bmo@gmail.com>
Mon, 25 Jul 2016 15:06:34 +0800
changeset 348938 f8a4ca6608d94a78e2b0a6cfdc22406057ed4e2a
parent 348937 e16a16e8146afab78e8a43b7a0334770520c8f86
child 348939 e723ec6aafabfaf97786dd31e534d35fdc3a2354
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdkeeler
bugs1287290
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 1287290 - Use ScopedAutoSECItem in PSM more. r=dkeeler 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;