Have fun with lock scopers.
Change-Id: I2697349024769545c2c37173e6ed68640b7d3b78
Reviewed-on: https://boringssl-review.googlesource.com/20805
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index 6c9db80..4c6d93f 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -672,14 +672,13 @@
data.session_id_length = session_id_len;
OPENSSL_memcpy(data.session_id, session_id, session_id_len);
- CRYPTO_MUTEX_lock_read(&ssl->session_ctx->lock);
+ MutexReadLock lock(&ssl->session_ctx->lock);
session.reset(lh_SSL_SESSION_retrieve(ssl->session_ctx->sessions, &data));
if (session) {
// |lh_SSL_SESSION_retrieve| returns a non-owning pointer.
SSL_SESSION_up_ref(session.get());
}
// TODO(davidben): This should probably move it to the front of the list.
- CRYPTO_MUTEX_unlock_read(&ssl->session_ctx->lock);
}
// Fall back to the external cache, if it exists.
@@ -1014,27 +1013,30 @@
// Although |session| is inserted into two structures (a doubly-linked list
// and the hash table), |ctx| only takes one reference.
SSL_SESSION_up_ref(session);
+ UniquePtr<SSL_SESSION> owned_session(session);
SSL_SESSION *old_session;
- CRYPTO_MUTEX_lock_write(&ctx->lock);
+ MutexWriteLock lock(&ctx->lock);
if (!lh_SSL_SESSION_insert(ctx->sessions, &old_session, session)) {
- CRYPTO_MUTEX_unlock_write(&ctx->lock);
- SSL_SESSION_free(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.
- CRYPTO_MUTEX_unlock_write(&ctx->lock);
- SSL_SESSION_free(old_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| must be removed from
- // the linked list and released.
+ // 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_free(old_session);
}
SSL_SESSION_list_add(ctx, session);
@@ -1049,7 +1051,6 @@
}
}
- CRYPTO_MUTEX_unlock_write(&ctx->lock);
return 1;
}
@@ -1128,9 +1129,8 @@
return;
}
tp.time = time;
- CRYPTO_MUTEX_lock_write(&ctx->lock);
+ MutexWriteLock lock(&ctx->lock);
lh_SSL_SESSION_doall_arg(tp.cache, timeout_doall_arg, &tp);
- CRYPTO_MUTEX_unlock_write(&ctx->lock);
}
void SSL_CTX_sess_set_new_cb(SSL_CTX *ctx,
diff --git a/ssl/ssl_x509.cc b/ssl/ssl_x509.cc
index 7ff6a89..2b7ba83 100644
--- a/ssl/ssl_x509.cc
+++ b/ssl/ssl_x509.cc
@@ -835,10 +835,8 @@
X509 *SSL_CTX_get0_certificate(const SSL_CTX *ctx) {
check_ssl_ctx_x509_method(ctx);
- CRYPTO_MUTEX_lock_write((CRYPTO_MUTEX *) &ctx->lock);
- X509 *ret = ssl_cert_get0_leaf(ctx->cert);
- CRYPTO_MUTEX_unlock_write((CRYPTO_MUTEX *) &ctx->lock);
- return ret;
+ MutexWriteLock lock(const_cast<CRYPTO_MUTEX*>(&ctx->lock));
+ return ssl_cert_get0_leaf(ctx->cert);
}
static int ssl_cert_set0_chain(CERT *cert, STACK_OF(X509) *chain) {
@@ -994,11 +992,8 @@
int SSL_CTX_get0_chain_certs(const SSL_CTX *ctx, STACK_OF(X509) **out_chain) {
check_ssl_ctx_x509_method(ctx);
- CRYPTO_MUTEX_lock_write((CRYPTO_MUTEX *) &ctx->lock);
- const int ret = ssl_cert_cache_chain_certs(ctx->cert);
- CRYPTO_MUTEX_unlock_write((CRYPTO_MUTEX *) &ctx->lock);
-
- if (!ret) {
+ MutexWriteLock lock(const_cast<CRYPTO_MUTEX*>(&ctx->lock));
+ if (!ssl_cert_cache_chain_certs(ctx->cert)) {
*out_chain = NULL;
return 0;
}
@@ -1165,11 +1160,10 @@
check_ssl_ctx_x509_method(ctx);
// This is a logically const operation that may be called on multiple threads,
// so it needs to lock around updating |cached_x509_client_CA|.
- CRYPTO_MUTEX_lock_write((CRYPTO_MUTEX *) &ctx->lock);
- STACK_OF(X509_NAME) *ret = buffer_names_to_x509(
- ctx->client_CA, (STACK_OF(X509_NAME) **)&ctx->cached_x509_client_CA);
- CRYPTO_MUTEX_unlock_write((CRYPTO_MUTEX *) &ctx->lock);
- return ret;
+ MutexWriteLock lock(const_cast<CRYPTO_MUTEX *>(&ctx->lock));
+ return buffer_names_to_x509(
+ ctx->client_CA,
+ const_cast<STACK_OF(X509_NAME) **>(&ctx->cached_x509_client_CA));
}
static int add_client_CA(STACK_OF(CRYPTO_BUFFER) **names, X509 *x509,
