Reduce bouncing on the cache lock in ssl_update_cache.
ssl_update_cache takes the cache lock to add to the session cache,
releases it, and then immediately takes and releases the lock to
increment handshakes_since_cache_flush. Then, in 1/255 connections, does
the same thing again to flush stale sessions.
Merge the first two into one lock. In doing so, move ssl_update_cache to
ssl_session.cc, so it can access a newly-extracted add_session_lock.
Also remove the mode parameter (the SSL knows if it's a client or
server), and move the established_session != session check to the
caller, which more directly knows whether there was a new session.
Also add some TSan coverage for this path in the tests. In an earlier
iteration of this patch, I managed to introduce a double-locking bug
because we weren't testing it at all. Confirmed this test catches both
double-locking and insufficient locking. (It doesn't seem able to catch
using a read lock instead of a write lock in SSL_CTX_flush_sessions,
however. I suspect the hash table is distributing the cells each thread
touches.)
Update-Note: This reshuffles some locks around the session cache.
(Hopefully for the better.)
Change-Id: I78dca53fda74e036b90110cca7fbcc306a5c8ebe
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48133
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index b0f0892..76e6dc4 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -163,7 +163,6 @@
static void SSL_SESSION_list_remove(SSL_CTX *ctx, SSL_SESSION *session);
static void SSL_SESSION_list_add(SSL_CTX *ctx, SSL_SESSION *session);
-static int remove_session_lock(SSL_CTX *ctx, SSL_SESSION *session, int lock);
UniquePtr<SSL_SESSION> ssl_session_new(const SSL_X509_METHOD *x509_method) {
return MakeUnique<SSL_SESSION>(x509_method);
@@ -754,34 +753,36 @@
return ssl_hs_ok;
}
-static int remove_session_lock(SSL_CTX *ctx, SSL_SESSION *session, int lock) {
- int ret = 0;
-
- if (session != NULL && session->session_id_length != 0) {
- if (lock) {
- CRYPTO_MUTEX_lock_write(&ctx->lock);
- }
- SSL_SESSION *found_session = lh_SSL_SESSION_retrieve(ctx->sessions,
- session);
- if (found_session == session) {
- ret = 1;
- found_session = lh_SSL_SESSION_delete(ctx->sessions, session);
- SSL_SESSION_list_remove(ctx, session);
- }
-
- if (lock) {
- CRYPTO_MUTEX_unlock_write(&ctx->lock);
- }
-
- if (ret) {
- if (ctx->remove_session_cb != NULL) {
- ctx->remove_session_cb(ctx, found_session);
- }
- SSL_SESSION_free(found_session);
- }
+static bool remove_session(SSL_CTX *ctx, SSL_SESSION *session, bool lock) {
+ if (session == nullptr || session->session_id_length == 0) {
+ return false;
}
- return ret;
+ if (lock) {
+ CRYPTO_MUTEX_lock_write(&ctx->lock);
+ }
+
+ SSL_SESSION *found_session = lh_SSL_SESSION_retrieve(ctx->sessions, session);
+ bool found = found_session == session;
+ if (found) {
+ found_session = lh_SSL_SESSION_delete(ctx->sessions, session);
+ SSL_SESSION_list_remove(ctx, session);
+ }
+
+ if (lock) {
+ CRYPTO_MUTEX_unlock_write(&ctx->lock);
+ }
+
+ if (found) {
+ // TODO(https://crbug.com/boringssl/251): Callbacks should not be called
+ // under a lock.
+ if (ctx->remove_session_cb != nullptr) {
+ ctx->remove_session_cb(ctx, found_session);
+ }
+ SSL_SESSION_free(found_session);
+ }
+
+ return found;
}
void ssl_set_session(SSL *ssl, SSL_SESSION *session) {
@@ -839,6 +840,98 @@
}
}
+static bool add_session_locked(SSL_CTX *ctx, UniquePtr<SSL_SESSION> session) {
+ SSL_SESSION *new_session = session.get();
+ SSL_SESSION *old_session;
+ if (!lh_SSL_SESSION_insert(ctx->sessions, &old_session, new_session)) {
+ return false;
+ }
+ // |ctx->sessions| took ownership of |new_session| and gave us back a
+ // reference to |old_session|. (|old_session| may be the same as
+ // |new_session|, in which case we traded identical references with
+ // |ctx->sessions|.)
+ session.release();
+ session.reset(old_session);
+
+ if (old_session != nullptr) {
+ if (old_session == new_session) {
+ // |session| was already in the cache. There are no linked list pointers
+ // to update.
+ return false;
+ }
+
+ // There was a session ID collision. |old_session| was replaced with
+ // |session| in the hash table, so |old_session| must be removed from the
+ // linked list to match.
+ SSL_SESSION_list_remove(ctx, old_session);
+ }
+
+ // This does not increment the reference count. Although |session| is inserted
+ // into two structures (a doubly-linked list and the hash table), |ctx| only
+ // takes one reference.
+ SSL_SESSION_list_add(ctx, new_session);
+
+ // Enforce any cache size limits.
+ if (SSL_CTX_sess_get_cache_size(ctx) > 0) {
+ while (lh_SSL_SESSION_num_items(ctx->sessions) >
+ SSL_CTX_sess_get_cache_size(ctx)) {
+ if (!remove_session(ctx, ctx->session_cache_tail,
+ /*lock=*/false)) {
+ break;
+ }
+ }
+ }
+
+ return true;
+}
+
+void ssl_update_cache(SSL *ssl) {
+ SSL_CTX *ctx = ssl->session_ctx.get();
+ SSL_SESSION *session = ssl->s3->established_session.get();
+ int mode = SSL_is_server(ssl) ? SSL_SESS_CACHE_SERVER : SSL_SESS_CACHE_CLIENT;
+ if (!SSL_SESSION_is_resumable(session) ||
+ (ctx->session_cache_mode & mode) != mode) {
+ return;
+ }
+
+ // Clients never use the internal session cache.
+ if (ssl->server &&
+ !(ctx->session_cache_mode & SSL_SESS_CACHE_NO_INTERNAL_STORE)) {
+ UniquePtr<SSL_SESSION> ref = UpRef(session);
+ bool remove_expired_sessions = false;
+ {
+ MutexWriteLock lock(&ctx->lock);
+ add_session_locked(ctx, std::move(ref));
+
+ if (!(ctx->session_cache_mode & SSL_SESS_CACHE_NO_AUTO_CLEAR)) {
+ // Automatically flush the internal session cache every 255 connections.
+ ctx->handshakes_since_cache_flush++;
+ if (ctx->handshakes_since_cache_flush >= 255) {
+ remove_expired_sessions = true;
+ ctx->handshakes_since_cache_flush = 0;
+ }
+ }
+ }
+
+ if (remove_expired_sessions) {
+ // |SSL_CTX_flush_sessions| takes the lock we just released. We could
+ // merge the critical sections, but we'd then call user code under a
+ // lock, or compute |now| earlier, even when not flushing.
+ OPENSSL_timeval now;
+ ssl_get_current_time(ssl, &now);
+ SSL_CTX_flush_sessions(ctx, now.tv_sec);
+ }
+ }
+
+ if (ctx->new_session_cb != nullptr) {
+ UniquePtr<SSL_SESSION> ref = UpRef(session);
+ if (ctx->new_session_cb(ssl, ref.get())) {
+ // |new_session_cb|'s return value signals whether it took ownership.
+ ref.release();
+ }
+ }
+}
+
BSSL_NAMESPACE_END
using namespace bssl;
@@ -1121,51 +1214,13 @@
}
int SSL_CTX_add_session(SSL_CTX *ctx, SSL_SESSION *session) {
- // Although |session| is inserted into two structures (a doubly-linked list
- // and the hash table), |ctx| only takes one reference.
UniquePtr<SSL_SESSION> owned_session = UpRef(session);
-
- SSL_SESSION *old_session;
MutexWriteLock lock(&ctx->lock);
- if (!lh_SSL_SESSION_insert(ctx->sessions, &old_session, session)) {
- return 0;
- }
- // |ctx->sessions| took ownership of |session| and gave us back a reference to
- // |old_session|. (|old_session| may be the same as |session|, in which case
- // we traded identical references with |ctx->sessions|.)
- owned_session.release();
- owned_session.reset(old_session);
-
- if (old_session != NULL) {
- if (old_session == session) {
- // |session| was already in the cache. There are no linked list pointers
- // to update.
- return 0;
- }
-
- // There was a session ID collision. |old_session| was replaced with
- // |session| in the hash table, so |old_session| must be removed from the
- // linked list to match.
- SSL_SESSION_list_remove(ctx, old_session);
- }
-
- SSL_SESSION_list_add(ctx, session);
-
- // Enforce any cache size limits.
- if (SSL_CTX_sess_get_cache_size(ctx) > 0) {
- while (lh_SSL_SESSION_num_items(ctx->sessions) >
- SSL_CTX_sess_get_cache_size(ctx)) {
- if (!remove_session_lock(ctx, ctx->session_cache_tail, 0)) {
- break;
- }
- }
- }
-
- return 1;
+ return add_session_locked(ctx, std::move(owned_session));
}
int SSL_CTX_remove_session(SSL_CTX *ctx, SSL_SESSION *session) {
- return remove_session_lock(ctx, session, 1);
+ return remove_session(ctx, session, /*lock=*/true);
}
int SSL_set_session(SSL *ssl, SSL_SESSION *session) {
@@ -1219,10 +1274,11 @@
if (param->time == 0 ||
session->time + session->timeout < session->time ||
param->time > (session->time + session->timeout)) {
- // The reason we don't call SSL_CTX_remove_session() is to
- // save on locking overhead
+ // TODO(davidben): This can probably just call |remove_session|.
(void) lh_SSL_SESSION_delete(param->cache, session);
SSL_SESSION_list_remove(param->ctx, session);
+ // TODO(https://crbug.com/boringssl/251): Callbacks should not be called
+ // under a lock.
if (param->ctx->remove_session_cb != NULL) {
param->ctx->remove_session_cb(param->ctx, session);
}
