Bug 1275197 - Ensure nsNSSU2FToken.cpp GetSymKeyByNickname() does not cause leaks. r=keeler
authorCykesiopka <cykesiopka.bmo@gmail.com>
Wed, 01 Jun 2016 22:43:37 -0700
changeset 341172 d048e177ab2685617f4cadad190314c2452d324e
parent 341171 94c374f96f84710e07fd4678fb56dc923c38252a
child 341173 f795d90a2debcabf78d6cce0d446990e08bd0ba6
push id1183
push userraliiev@mozilla.com
push dateMon, 05 Sep 2016 20:01:49 +0000
treeherdermozilla-release@3148731bed45 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1275197
milestone49.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 1275197 - Ensure nsNSSU2FToken.cpp GetSymKeyByNickname() does not cause leaks. r=keeler Prior to these changes, GetSymKeyByNickname() could theoretically leak. This should not happen in practice, so the changes here just ensure that the code doesn't cause leaks. MozReview-Commit-ID: LWtqLmsBPV2
security/manager/ssl/nsNSSU2FToken.cpp
--- a/security/manager/ssl/nsNSSU2FToken.cpp
+++ b/security/manager/ssl/nsNSSU2FToken.cpp
@@ -79,48 +79,59 @@ nsNSSU2FToken::virtualDestroyNSSReferenc
 }
 
 void
 nsNSSU2FToken::destructorSafeDestroyNSSReference()
 {
   mWrappingKey = nullptr;
 }
 
+/**
+ * Gets the first key with the given nickname from the given slot. Any other
+ * keys found are not returned.
+ * PK11_GetNextSymKey() should not be called on the returned key.
+ *
+ * @param aSlot Slot to search.
+ * @param aNickname Nickname the key should have.
+ * @return The first key found. nullptr if no key could be found.
+ */
 static UniquePK11SymKey
 GetSymKeyByNickname(const UniquePK11SlotInfo& aSlot,
-                    nsCString aNickname,
+                    const nsCString& aNickname,
                     const nsNSSShutDownPreventionLock&)
 {
   MOZ_ASSERT(aSlot);
   if (!aSlot) {
     return nullptr;
   }
 
   MOZ_LOG(gNSSTokenLog, LogLevel::Debug,
           ("Searching for a symmetric key named %s", aNickname.get()));
 
-  PK11SymKey* keyList;
-  keyList = PK11_ListFixedKeysInSlot(aSlot.get(),
-                                     const_cast<char*>(aNickname.get()),
-                                     /* wincx */ nullptr);
-  while (keyList) {
-    UniquePK11SymKey freeKey(keyList);
-
-    UniquePORTString freeKeyName(PK11_GetSymKeyNickname(freeKey.get()));
-
-    if (aNickname == freeKeyName.get()) {
-      MOZ_LOG(gNSSTokenLog, LogLevel::Debug, ("Symmetric key found!"));
-      return freeKey;
-    }
-
-    keyList = PK11_GetNextSymKey(keyList);
+  UniquePK11SymKey keyListHead(
+    PK11_ListFixedKeysInSlot(aSlot.get(), const_cast<char*>(aNickname.get()),
+                             /* wincx */ nullptr));
+  if (!keyListHead) {
+    MOZ_LOG(gNSSTokenLog, LogLevel::Debug, ("Symmetric key not found."));
+    return nullptr;
   }
 
-  MOZ_LOG(gNSSTokenLog, LogLevel::Debug, ("Symmetric key not found."));
-  return nullptr;
+  // Sanity check PK11_ListFixedKeysInSlot() only returns keys with the correct
+  // nickname.
+  MOZ_ASSERT(aNickname ==
+               UniquePORTString(PK11_GetSymKeyNickname(keyListHead.get())).get());
+  MOZ_LOG(gNSSTokenLog, LogLevel::Debug, ("Symmetric key found!"));
+
+  // Free any remaining keys in the key list.
+  UniquePK11SymKey freeKey(PK11_GetNextSymKey(keyListHead.get()));
+  while (freeKey) {
+    freeKey = UniquePK11SymKey(PK11_GetNextSymKey(freeKey.get()));
+  }
+
+  return keyListHead;
 }
 
 static nsresult
 GenEcKeypair(const UniquePK11SlotInfo& aSlot,
      /*out*/ UniqueSECKEYPrivateKey& aPrivKey,
      /*out*/ UniqueSECKEYPublicKey& aPubKey,
              const nsNSSShutDownPreventionLock&)
 {