NSS: fix session cache lock initialisation.

net/third_party/nss/ssl/sslnonce.c

Index: net/third_party/nss/ssl/sslnonce.c
diff --git a/net/third_party/nss/ssl/sslnonce.c b/net/third_party/nss/ssl/sslnonce.c
index 5d8a95407aa09711660be45711464b26769f7207..081fa3ba5cca721184f07c5b56f3a65bf36c0e90 100644
--- a/net/third_party/nss/ssl/sslnonce.c
+++ b/net/third_party/nss/ssl/sslnonce.c
@@ -35,90 +35,59 @@ static PZLock *      cacheLock = NULL;
 #define LOCK_CACHE 	lock_cache()
 #define UNLOCK_CACHE	PZ_Unlock(cacheLock)
 
-static SECStatus
-ssl_InitClientSessionCacheLock(void)
-{
-    cacheLock = PZ_NewLock(nssILockCache);
-    return cacheLock ? SECSuccess : SECFailure;
-}
+static PRCallOnceType lockOnce;
 
-static SECStatus
-ssl_FreeClientSessionCacheLock(void)
-{
-    if (cacheLock) {
-        PZ_DestroyLock(cacheLock);
-        cacheLock = NULL;
-        return SECSuccess;
-    }
-    PORT_SetError(SEC_ERROR_NOT_INITIALIZED);
-    return SECFailure;
-}
-
-static PRBool LocksInitializedEarly = PR_FALSE;
-
-static SECStatus
-FreeSessionCacheLocks()
-{
-    SECStatus rv1, rv2;
-    rv1 = ssl_FreeSymWrapKeysLock();
-    rv2 = ssl_FreeClientSessionCacheLock();
-    if ( (SECSuccess == rv1) && (SECSuccess == rv2) ) {
-        return SECSuccess;
-    }
-    return SECFailure;
-}
-
-static SECStatus
-InitSessionCacheLocks(void)
-{
-    SECStatus rv1, rv2;
-    PRErrorCode rc;
-    rv1 = ssl_InitSymWrapKeysLock();
-    rv2 = ssl_InitClientSessionCacheLock();
-    if ( (SECSuccess == rv1) && (SECSuccess == rv2) ) {
-        return SECSuccess;
-    }
-    rc = PORT_GetError();
-    FreeSessionCacheLocks();
-    PORT_SetError(rc);
-    return SECFailure;
-}
-
-/* free the session cache locks if they were initialized early */
+/* ssl_FreeSessionCacheLocks frees the locks used for session caching and
+ * resets them to their initial state. */
 SECStatus
 ssl_FreeSessionCacheLocks()

[wtc, 2013/09/19 04:37:51]

Nit: add "void" inside the parentheses because this is C code.

[agl, 2013/09/19 17:40:51]

Done.

 {
-    PORT_Assert(PR_TRUE == LocksInitializedEarly);
-    if (!LocksInitializedEarly) {
+    SECStatus rv;
+
+    if (cacheLock) {
+        PZ_DestroyLock(cacheLock);
+        cacheLock = NULL;
+        return SECSuccess;

[wtc, 2013/09/19 04:37:51]

BUG: remove this return statement.

It may be better to rewrite this if-else statement as follows:

    if (!cacheLock) {
        PORT_SetError(SEC_ERROR_NOT_INITIALIZED);
        return SECFailure;
    }
    PZ_DestroyLock(cacheLock);
    cacheLock = NULL;

[agl, 2013/09/19 17:40:51]

Done.

+    } else {
         PORT_SetError(SEC_ERROR_NOT_INITIALIZED);
         return SECFailure;
     }
-    FreeSessionCacheLocks();
-    LocksInitializedEarly = PR_FALSE;
-    return SECSuccess;
-}
 
-static PRCallOnceType lockOnce;
-
-/* free the session cache locks if they were initialized lazily */
-static SECStatus ssl_ShutdownLocks(void* appData, void* nssData)
-{
-    PORT_Assert(PR_FALSE == LocksInitializedEarly);
-    if (LocksInitializedEarly) {
-        PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
-        return SECFailure;
+    rv = ssl_FreeSymWrapKeysLock();
+    if (rv != SECSuccess) {
+        return rv;
     }
-    FreeSessionCacheLocks();
+
     memset(&lockOnce, 0, sizeof(lockOnce));

[Ryan Sleevi, 2013/09/19 01:14:13]

Normal NSS style is to keep a 'static const PRCallOnceType' to assign to. I note
this is equivalent, just a style nit.

lockOnce = pristineCallOnce;

[agl, 2013/09/19 17:40:51]

Done.

-    return SECSuccess;
+    return SECFailure;

[wtc, 2013/09/19 04:37:51]

BUG: this should return SECSuccess.

[agl, 2013/09/19 17:40:51]

Done.

 }
 
-static PRStatus initSessionCacheLocksLazily(void)
+/* ssl_ShutdownLocks is a callback from NSS_RegisterShutdown which destroys the
+ * session cache locks on shutdown. */
+static SECStatus ssl_ShutdownLocks(void* appData, void* nssData)

[wtc, 2013/09/19 04:37:51]

Nit: NSS style folds this line as follows:

static SECStatus
ssl_ShutdownLocks(void* appData, void* nssData)

[agl, 2013/09/19 17:40:51]

Done.

 {
-    SECStatus rv = InitSessionCacheLocks();
-    if (SECSuccess != rv) {
+    return ssl_FreeSessionCacheLocks();
+}
+
+/* initSessionCacheLocks is called, protected by lockOnce, to create the

[wtc, 2013/09/19 04:37:51]

Nit: capitalize this function name "InitSessionCacheLocks".

[agl, 2013/09/19 17:40:51]

Done.

+ * session cache locks. */
+static PRStatus initSessionCacheLocks(void)

[wtc, 2013/09/19 04:37:51]

Fold this line as follows:

static PRStatus
initSessionCacheLocks(void)

[agl, 2013/09/19 17:40:51]

Done.

+{
+    SECStatus rv;
+
+    cacheLock = PZ_NewLock(nssILockCache);
+    if (cacheLock == NULL) {
         return PR_FAILURE;
     }
+    rv = ssl_InitSymWrapKeysLock();
+    if (rv != SECSuccess) {
+        PRErrorCode rc = PORT_GetError();

[wtc, 2013/09/19 04:37:51]

Nit: name this variable |error|.

[agl, 2013/09/19 17:40:51]

Done.

+        PZ_DestroyLock(cacheLock);
+        cacheLock = NULL;
+        PORT_SetError(rc);
+        return PR_FAILURE;
+    }
+
     rv = NSS_RegisterShutdown(ssl_ShutdownLocks, NULL);
     PORT_Assert(SECSuccess == rv);
     if (SECSuccess != rv) {
@@ -127,34 +96,18 @@ static PRStatus initSessionCacheLocksLazily(void)
     return PR_SUCCESS;
 }
 
-/* lazyInit means that the call is not happening during a 1-time
- * initialization function, but rather during dynamic, lazy initialization
- */
 SECStatus
-ssl_InitSessionCacheLocks(PRBool lazyInit)
+ssl_InitSessionCacheLocks()
 {
-    if (LocksInitializedEarly) {
-        return SECSuccess;
-    }
-
-    if (lazyInit) {
-        return (PR_SUCCESS ==
-                PR_CallOnce(&lockOnce, initSessionCacheLocksLazily)) ?
-               SECSuccess : SECFailure;
-    }
-     
-    if (SECSuccess == InitSessionCacheLocks()) {
-        LocksInitializedEarly = PR_TRUE;
-        return SECSuccess;
-    }
-
-    return SECFailure;
+    return (PR_SUCCESS ==
+            PR_CallOnce(&lockOnce, initSessionCacheLocks)) ?
+           SECSuccess : SECFailure;
 }
 
-static void 
+static void
 lock_cache(void)
 {
-    ssl_InitSessionCacheLocks(PR_TRUE);
+    ssl_InitSessionCacheLocks();
     PZ_Lock(cacheLock);
 }