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/handshake_client.cc b/ssl/handshake_client.cc
index 0fba737..d5ccafc 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -1801,10 +1801,8 @@
// Note TLS 1.2 resumptions with ticket renewal have both |ssl->session| (the
// resumed session) and |hs->new_session| (the session with the new ticket).
- if (hs->new_session == nullptr) {
- assert(ssl->session != nullptr);
- ssl->s3->established_session = UpRef(ssl->session);
- } else {
+ bool has_new_session = hs->new_session != nullptr;
+ if (has_new_session) {
// When False Start is enabled, the handshake reports completion early. The
// caller may then have passed the (then unresuable) |hs->new_session| to
// another thread via |SSL_get0_session| for resumption. To avoid potential
@@ -1821,11 +1819,16 @@
}
hs->new_session.reset();
+ } else {
+ assert(ssl->session != nullptr);
+ ssl->s3->established_session = UpRef(ssl->session);
}
hs->handshake_finalized = true;
ssl->s3->initial_handshake_complete = true;
- ssl_update_cache(hs, SSL_SESS_CACHE_CLIENT);
+ if (has_new_session) {
+ ssl_update_cache(ssl);
+ }
hs->state = state_done;
return ssl_hs_ok;
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index ec5c8b9..74ac133 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -1791,18 +1791,21 @@
ssl->ctx->x509_method->session_clear(hs->new_session.get());
}
- if (hs->new_session == nullptr) {
- assert(ssl->session != nullptr);
- ssl->s3->established_session = UpRef(ssl->session);
- } else {
+ bool has_new_session = hs->new_session != nullptr;
+ if (has_new_session) {
assert(ssl->session == nullptr);
ssl->s3->established_session = std::move(hs->new_session);
ssl->s3->established_session->not_resumable = false;
+ } else {
+ assert(ssl->session != nullptr);
+ ssl->s3->established_session = UpRef(ssl->session);
}
hs->handshake_finalized = true;
ssl->s3->initial_handshake_complete = true;
- ssl_update_cache(hs, SSL_SESS_CACHE_SERVER);
+ if (has_new_session) {
+ ssl_update_cache(ssl);
+ }
hs->state = state12_done;
return ssl_hs_ok;
diff --git a/ssl/internal.h b/ssl/internal.h
index ad199aa..ba1dde3 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -3147,7 +3147,7 @@
void ssl_session_renew_timeout(SSL *ssl, SSL_SESSION *session,
uint32_t timeout);
-void ssl_update_cache(SSL_HANDSHAKE *hs, int mode);
+void ssl_update_cache(SSL *ssl);
void ssl_send_alert(SSL *ssl, int level, int desc);
int ssl_send_alert_impl(SSL *ssl, int level, int desc);
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index ae9883d..3c9fc90 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -272,50 +272,6 @@
return ret;
}
-void ssl_update_cache(SSL_HANDSHAKE *hs, int mode) {
- SSL *const ssl = hs->ssl;
- SSL_CTX *ctx = ssl->session_ctx.get();
- if (!SSL_SESSION_is_resumable(ssl->s3->established_session.get()) ||
- (ctx->session_cache_mode & mode) != mode) {
- return;
- }
-
- // Clients never use the internal session cache.
- int use_internal_cache = ssl->server && !(ctx->session_cache_mode &
- SSL_SESS_CACHE_NO_INTERNAL_STORE);
- if (ssl->s3->established_session.get() != ssl->session.get()) {
- if (use_internal_cache) {
- SSL_CTX_add_session(ctx, ssl->s3->established_session.get());
- }
- if (ctx->new_session_cb != NULL) {
- UniquePtr<SSL_SESSION> ref = UpRef(ssl->s3->established_session);
- if (ctx->new_session_cb(ssl, ref.get())) {
- // |new_session_cb|'s return value signals whether it took ownership.
- ref.release();
- }
- }
- }
-
- if (use_internal_cache &&
- !(ctx->session_cache_mode & SSL_SESS_CACHE_NO_AUTO_CLEAR)) {
- // Automatically flush the internal session cache every 255 connections.
- int flush_cache = 0;
- CRYPTO_MUTEX_lock_write(&ctx->lock);
- ctx->handshakes_since_cache_flush++;
- if (ctx->handshakes_since_cache_flush >= 255) {
- flush_cache = 1;
- ctx->handshakes_since_cache_flush = 0;
- }
- CRYPTO_MUTEX_unlock_write(&ctx->lock);
-
- if (flush_cache) {
- struct OPENSSL_timeval now;
- ssl_get_current_time(ssl, &now);
- SSL_CTX_flush_sessions(ctx, now.tv_sec);
- }
- }
-}
-
static bool cbb_add_hex(CBB *cbb, Span<const uint8_t> in) {
static const char hextable[] = "0123456789abcdef";
uint8_t *out;
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);
}
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index d6edcfe..4bb9c32 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -5427,7 +5427,8 @@
}
}
- // Hit the maximum session cache size across multiple threads
+ // Hit the maximum session cache size across multiple threads, to test the
+ // size enforcement logic.
size_t limit = SSL_CTX_sess_number(server_ctx_.get()) + 2;
SSL_CTX_sess_set_cache_size(server_ctx_.get(), limit);
{
@@ -5443,6 +5444,59 @@
}
EXPECT_EQ(SSL_CTX_sess_number(server_ctx_.get()), limit);
}
+
+ // Reset the session cache, this time with a mock clock.
+ ASSERT_NO_FATAL_FAILURE(ResetContexts());
+ SSL_CTX_set_options(server_ctx_.get(), SSL_OP_NO_TICKET);
+ SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH);
+ SSL_CTX_set_session_cache_mode(server_ctx_.get(), SSL_SESS_CACHE_BOTH);
+ SSL_CTX_set_current_time_cb(server_ctx_.get(), CurrentTimeCallback);
+
+ // Make some sessions at an arbitrary start time. Then expire them.
+ g_current_time.tv_sec = 1000;
+ bssl::UniquePtr<SSL_SESSION> expired_session1 =
+ CreateClientSession(client_ctx_.get(), server_ctx_.get());
+ ASSERT_TRUE(expired_session1);
+ bssl::UniquePtr<SSL_SESSION> expired_session2 =
+ CreateClientSession(client_ctx_.get(), server_ctx_.get());
+ ASSERT_TRUE(expired_session2);
+ g_current_time.tv_sec += 100 * SSL_DEFAULT_SESSION_TIMEOUT;
+
+ session1 = CreateClientSession(client_ctx_.get(), server_ctx_.get());
+ ASSERT_TRUE(session1);
+
+ // Every 256 connections, we flush stale sessions from the session cache. Test
+ // this logic is correctly synchronized with other connection attempts.
+ static const int kNumConnections = 256;
+ {
+ std::vector<std::thread> threads;
+ threads.emplace_back([&] {
+ for (int i = 0; i < kNumConnections; i++) {
+ connect_with_session(nullptr);
+ }
+ });
+ threads.emplace_back([&] {
+ for (int i = 0; i < kNumConnections; i++) {
+ connect_with_session(nullptr);
+ }
+ });
+ threads.emplace_back([&] {
+ // Never connect with |expired_session2|. The session cache eagerly
+ // removes expired sessions when it sees them. Leaving |expired_session2|
+ // untouched ensures it is instead cleared by periodic flushing.
+ for (int i = 0; i < kNumConnections; i++) {
+ connect_with_session(expired_session1.get());
+ }
+ });
+ threads.emplace_back([&] {
+ for (int i = 0; i < kNumConnections; i++) {
+ connect_with_session(session1.get());
+ }
+ });
+ for (auto &thread : threads) {
+ thread.join();
+ }
+ }
}
TEST_P(SSLVersionTest, SessionTicketThreads) {
