Bug 160122 - Stop using PR_smprintf in PSM. r=keeler

The (more) modern Mozilla string classes can be used instead, which at the very
least provide built in automatic memory management and performance improvements.

MozReview-Commit-ID: 4l2Er5rkeI0

--HG--
extra : transplant_source : %A1%16%AB%02m%CA%25HfW%40%96Mq%0D%F0%91%9C%99%29

security/certverifier/NSSCertDBTrustDomain.cpp

@@ -16,6 +16,7 @@
 #include "cert.h"
 #include "certdb.h"
 #include "mozilla/UniquePtr.h"
+#include "mozilla/unused.h"
 #include "nsNSSCertificate.h"
 #include "nsServiceManagerUtils.h"
 #include "nss.h"
@@ -1016,16 +1017,16 @@ LoadLoadableRoots(/*optional*/ const char* dir, const char* modNameUTF8)
   int modType;
   SECMOD_DeleteModule(modNameUTF8, &modType);
 
-  UniquePtr<char, void(&)(char*)>
-    pkcs11ModuleSpec(PR_smprintf("name=\"%s\" library=\"%s\"", modNameUTF8,
-                                 escapedFullLibraryPath.get()),
-                     PR_smprintf_free);
-  if (!pkcs11ModuleSpec) {
+  nsAutoCString pkcs11ModuleSpec;
+  pkcs11ModuleSpec.AppendPrintf("name=\"%s\" library=\"%s\"", modNameUTF8,
+                                escapedFullLibraryPath.get());
+  if (pkcs11ModuleSpec.IsEmpty()) {
     return SECFailure;
   }
 
-  UniqueSECMODModule rootsModule(SECMOD_LoadUserModule(pkcs11ModuleSpec.get(),
-                                                       nullptr, false));
+  UniqueSECMODModule rootsModule(
+    SECMOD_LoadUserModule(const_cast<char*>(pkcs11ModuleSpec.get()), nullptr,
+                          false));
   if (!rootsModule) {
     return SECFailure;
   }
@@ -1049,60 +1050,55 @@ UnloadLoadableRoots(const char* modNameUTF8)
   }
 }
 
-char*
-DefaultServerNicknameForCert(CERTCertificate* cert)
+nsresult
+DefaultServerNicknameForCert(const CERTCertificate* cert,
+                     /*out*/ nsCString& nickname)
 {
-  char* nickname = nullptr;
-  int count;
-  bool conflict;
-  char* servername = nullptr;
+  MOZ_ASSERT(cert);
+  NS_ENSURE_ARG_POINTER(cert);
 
-  servername = CERT_GetCommonName(&cert->subject);
-  if (!servername) {
-    // Certs without common names are strange, but they do exist...
-    // Let's try to use another string for the nickname
-    servername = CERT_GetOrgUnitName(&cert->subject);
-    if (!servername) {
-      servername = CERT_GetOrgName(&cert->subject);
-      if (!servername) {
-        servername = CERT_GetLocalityName(&cert->subject);
-        if (!servername) {
-          servername = CERT_GetStateName(&cert->subject);
-          if (!servername) {
-            servername = CERT_GetCountryName(&cert->subject);
-            if (!servername) {
-              // We tried hard, there is nothing more we can do.
-              // A cert without any names doesn't really make sense.
-              return nullptr;
-            }
-          }
-        }
-      }
-    }
+  UniquePORTString baseName(CERT_GetCommonName(&cert->subject));
+  if (!baseName) {
+    baseName = UniquePORTString(CERT_GetOrgUnitName(&cert->subject));
+  }
+  if (!baseName) {
+    baseName = UniquePORTString(CERT_GetOrgName(&cert->subject));
+  }
+  if (!baseName) {
+    baseName = UniquePORTString(CERT_GetLocalityName(&cert->subject));
+  }
+  if (!baseName) {
+    baseName = UniquePORTString(CERT_GetStateName(&cert->subject));
+  }
+  if (!baseName) {
+    baseName = UniquePORTString(CERT_GetCountryName(&cert->subject));
+  }
+  if (!baseName) {
+    return NS_ERROR_FAILURE;
   }
 
-  count = 1;
-  while (1) {
-    if (count == 1) {
-      nickname = PR_smprintf("%s", servername);
+  // This function is only used in contexts where a failure to find a suitable
+  // nickname does not block the overall task from succeeding.
+  // As such, we use an arbitrary limit to prevent this nickname searching
+  // process from taking forever.
+  static const uint32_t ARBITRARY_LIMIT = 500;
+  for (uint32_t count = 1; count < ARBITRARY_LIMIT; count++) {
+    nickname = baseName.get();
+    if (count != 1) {
+      nickname.AppendPrintf(" #%u", count);
     }
-    else {
-      nickname = PR_smprintf("%s #%d", servername, count);
-    }
-    if (!nickname) {
-      break;
+    if (nickname.IsEmpty()) {
+      return NS_ERROR_FAILURE;
     }
 
-    conflict = SEC_CertNicknameConflict(nickname, &cert->derSubject,
-                                        cert->dbhandle);
+    bool conflict = SEC_CertNicknameConflict(nickname.get(), &cert->derSubject,
+                                             cert->dbhandle);
     if (!conflict) {
-      break;
+      return NS_OK;
     }
-    PR_Free(nickname);
-    count++;
   }
-  PR_FREEIF(servername);
-  return nickname;
+
+  return NS_ERROR_FAILURE;
 }
 
 void
@@ -1112,6 +1108,11 @@ SaveIntermediateCerts(const UniqueCERTCertList& certList)
     return;
   }
 
+  UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
+  if (!slot) {
+    return;
+  }
+
   bool isEndEntity = true;
   for (CERTCertListNode* node = CERT_LIST_HEAD(certList);
         !CERT_LIST_END(node, certList);
@@ -1133,15 +1134,19 @@ SaveIntermediateCerts(const UniqueCERTCertList& certList)
     }
 
     // We have found a signer cert that we want to remember.
-    char* nickname = DefaultServerNicknameForCert(node->cert);
-    if (nickname && *nickname) {
-      UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
-      if (slot) {
-        PK11_ImportCert(slot.get(), node->cert, CK_INVALID_HANDLE,
-                        nickname, false);
-      }
+    nsAutoCString nickname;
+    nsresult rv = DefaultServerNicknameForCert(node->cert, nickname);
+    if (NS_FAILED(rv)) {
+      continue;
     }
-    PR_FREEIF(nickname);
+
+    // Saving valid intermediate certs to the database is a compatibility hack
+    // to work around unknown issuer errors for incorrectly configured servers
+    // that fail to send the necessary intermediate certs. As such, we ignore
+    // the return value of PK11_ImportCert(), since it doesn't really matter if
+    // it fails.
+    Unused << PK11_ImportCert(slot.get(), node->cert, CK_INVALID_HANDLE,
+                              nickname.get(), false);
   }
 }
 

security/certverifier/NSSCertDBTrustDomain.h

@@ -10,6 +10,7 @@
 #include "CertVerifier.h"
 #include "ScopedNSSTypes.h"
 #include "nsICertBlocklist.h"
+#include "nsString.h"
 #include "pkix/pkixtypes.h"
 #include "secmodt.h"
 
@@ -37,8 +38,8 @@ SECStatus LoadLoadableRoots(/*optional*/ const char* dir,
 
 void UnloadLoadableRoots(const char* modNameUTF8);
 
-// Caller must free the result with PR_Free
-char* DefaultServerNicknameForCert(CERTCertificate* cert);
+nsresult DefaultServerNicknameForCert(const CERTCertificate* cert,
+                              /*out*/ nsCString& nickname);
 
 void SaveIntermediateCerts(const UniqueCERTCertList& certList);
 

security/manager/ssl/nsCertOverrideService.cpp

@@ -373,27 +373,24 @@ nsCertOverrideService::RememberValidityOverride(const nsACString& aHostName,
     return NS_ERROR_FAILURE;
   }
 
-  char* nickname = DefaultServerNicknameForCert(nsscert.get());
-  if (!aTemporary && nickname && *nickname)
-  {
+  nsAutoCString nickname;
+  nsresult rv = DefaultServerNicknameForCert(nsscert.get(), nickname);
+  if (!aTemporary && NS_SUCCEEDED(rv)) {
     UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
     if (!slot) {
-      PR_Free(nickname);
       return NS_ERROR_FAILURE;
     }
 
     SECStatus srv = PK11_ImportCert(slot.get(), nsscert.get(), CK_INVALID_HANDLE,
-                                    nickname, false);
+                                    nickname.get(), false);
     if (srv != SECSuccess) {
-      PR_Free(nickname);
       return NS_ERROR_FAILURE;
     }
   }
-  PR_FREEIF(nickname);
 
   nsAutoCString fpStr;
-  nsresult rv = GetCertFingerprintByOidTag(nsscert.get(),
-                  mOidTagForStoringNewHashes, fpStr);
+  rv = GetCertFingerprintByOidTag(nsscert.get(), mOidTagForStoringNewHashes,
+                                  fpStr);
   if (NS_FAILED(rv))
     return rv;
 

security/manager/ssl/nsNSSCertificateDB.cpp

@@ -1289,13 +1289,9 @@ nsNSSCertificateDB::get_default_nickname(CERTCertificate *cert,
   NS_ConvertUTF16toUTF8 nickFmt(tmpNickFmt);
 
   nsAutoCString baseName;
-  char *temp_nn = PR_smprintf(nickFmt.get(), username.get(), caname.get());
-  if (!temp_nn) {
+  baseName.AppendPrintf(nickFmt.get(), username.get(), caname.get());
+  if (baseName.IsEmpty()) {
     return;
-  } else {
-    baseName = temp_nn;
-    PR_smprintf_free(temp_nn);
-    temp_nn = nullptr;
   }
 
   nickname = baseName;
@@ -1310,28 +1306,26 @@ nsNSSCertificateDB::get_default_nickname(CERTCertificate *cert,
     return;
 
   if (!PK11_IsInternal(slot.get())) {
-    char* tmp = PR_smprintf("%s:%s", PK11_GetTokenName(slot.get()),
-                            baseName.get());
-    if (!tmp) {
+    nsAutoCString tmp;
+    tmp.AppendPrintf("%s:%s", PK11_GetTokenName(slot.get()), baseName.get());
+    if (tmp.IsEmpty()) {
       nickname.Truncate();
       return;
     }
     baseName = tmp;
-    PR_smprintf_free(tmp);
-
     nickname = baseName;
   }
 
   int count = 1;
   while (true) {
     if ( count > 1 ) {
-      char *tmp = PR_smprintf("%s #%d", baseName.get(), count);
-      if (!tmp) {
+      nsAutoCString tmp;
+      tmp.AppendPrintf("%s #%d", baseName.get(), count);
+      if (tmp.IsEmpty()) {
         nickname.Truncate();
         return;
       }
       nickname = tmp;
-      PR_smprintf_free(tmp);
     }
 
     UniqueCERTCertificate dummycert;

security/manager/ssl/nsNSSCertificateDB.h

@@ -5,13 +5,14 @@
 #ifndef nsNSSCertificateDB_h
 #define nsNSSCertificateDB_h
 
+#include "ScopedNSSTypes.h"
 #include "certt.h"
 #include "mozilla/Mutex.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/UniquePtr.h"
 #include "nsIX509CertDB.h"
 #include "nsNSSShutDown.h"
-#include "ScopedNSSTypes.h"
+#include "nsString.h"
 
 class nsCString;
 class nsIArray;

security/manager/ssl/nsPKCS12Blob.cpp

@@ -645,10 +645,9 @@ nsPKCS12Blob::nickname_collision(SECItem *oldNick, PRBool *cancel, void *wincx)
     // without a corresponding cert.
     //  XXX If a user imports *many* certs without the 'friendly name'
     //      attribute, then this may take a long time.  :(
+    nickname = nickFromPropC;
     if (count > 1) {
-      nickname.Adopt(PR_smprintf("%s #%d", nickFromPropC.get(), count));
-    } else {
-      nickname = nickFromPropC;
+      nickname.AppendPrintf(" #%d", count);
     }
     CERTCertificate *cert = CERT_FindCertByNickname(CERT_GetDefaultCertDB(),
                                            const_cast<char*>(nickname.get()));