764646 - Always initialize the SSL session cache locks lazily

Status: NEW

(NSS :: Libraries, defect, P3)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

In bug 403240, Julien added code to initialize the two SSL session cache
locks (cacheLock in sslnonce.c and symWrapKeysLock in ssl3con.c) lazily
using PR_CallOnce, and also a way for SSL servers to initialize the locks
early in SSL_ConfigServerSessionIDCache etc. to avoid the overhead of
PR_CallOnce.  The assumption is that an SSL server needs to call
SSL_ConfigServerSessionIDCache early in the program, before it starts to
do SSL.

This assumption no longer holds in a client application that is
predominantly an SSL client but may also act as an SSL server in a
peer-to-peer protocol.  See the Chromium bug 131622:
http://code.google.com/p/chromium/issues/detail?id=131622

The cleanest solution is to always initialize the locks lazily.  But we
need to keep the overhead of PR_CallOnce low.  Right now we initialize
the locks lazily right before we use the locks.

After analyzing the code that uses the two locks, I found that the locks
are always used with an sslSocket 'ss', except in SSL3_ShutdownServerCache
and SSL_ClearSessionCache.  This matches our expectation that only SSL
sockets use the SSL session cache.  Therefore we only need to initialize
the locks lazily before we create sslSocket objects.  This is the approach
taken in the attached patch.

You can also review the patch at https://chromiumcodereview.appspot.com/10539144/

Comment 1

[Robert Relyea]

Comment on attachment 632952 [details] [diff] [review]
Proposed patch

r+

One question, is there a strong reason not to do this init in the new ssl_init function which we've already created?

Question 2: I didn't check, but the code is presuming the FreeSessionCacheLocks(); is safe to call if InitSessionCacheLocks() is never called.

bob

Comment 2

[Wan-Teh Chang]

Attachment: Step 1: simply use PR_CallOnce to protect the lock creation, by Adam Langley

I'm splitting my patch into two steps. The first step, done by Adam in this
patch, is easy to verify correct. It simply changes ssl_InitSessionCacheLocks()
to always call PR_CallOnce to protect the lock creation.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/4bb4712d7432

I will create the patch for the second step, which moves the
ssl_InitSessionCacheLocks() call into ssl_Init() to reduce the overhead of
calling PR_CallOnce. The second step is harder to verify correct because we
need to identify the libSSL functions that are considered "entry points" to
the session cache and make sure they all call ssl_Init().

Comment 3

[Kai Engert (:KaiE:)]

changing target milestone to 3.15.4

Comment 4

[Wan-Teh Chang]

Comment on attachment 810131 [details] [diff] [review]
Step 1: simply use PR_CallOnce to protect the lock creation, by Adam Langley

I backed out this patch in https://hg.mozilla.org/projects/nss/rev/07dd289c5111
because selfserv calls SSL_ConfigServerSessionIDCache, which ultimately calls
NSS_RegisterShutdown, before calling NSS_Initialize.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P1'.
:beurdouche, could you have a look please?
For more information, please visit auto_nag documentation.